[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] spapr_pci: make index property mandatory
From: |
Greg Kurz |
Subject: |
Re: [Qemu-ppc] [PATCH] spapr_pci: make index property mandatory |
Date: |
Thu, 14 Sep 2017 15:36:32 +0200 |
On Thu, 14 Sep 2017 22:31:26 +1000
David Gibson <address@hidden> wrote:
> On Thu, Sep 14, 2017 at 09:03:15AM +0200, Greg Kurz wrote:
> > Creating several PHBs without index property confuses the DRC code
> > and causes issues:
> > - only the first index-less PHB is functional, the other ones will
> > silently ignore hotplugging of PCI devices
> > - QEMU will even terminate if these PHBs have cold-plugged devices
> >
> > qemu-system-ppc64: -device virtio-net,bus=pci2.0: an attached device
> > is still awaiting release
> >
> > This happens because DR connectors for child PCI devices are created
> > with a DRC index that is derived from the PHB's index property. If the
> > PHBs are created without index, then the same value of -1 is used to
> > compute the DRC indexes for both PHBs, hence causing the collision.
> >
> > Also, the index property is used to compute the placement of the PHB's
> > memory regions. It is limited to 31 or 255, depending on the machine
> > type version. This fits well with the requirements of DRC indexes, which
> > need the PHB index to be a 16-bit value.
> >
> > This patch hence makes the index property mandatory. As a consequence,
> > the PHB's memory regions and BUID are now always configured according
> > to the index, and it is no longer possible to set them from the command
> > line. We have to introduce a PHB instance init function to initialize
> > the 64-bit window address to -1 because pseries-2.7 and older machines
> > don't set it.
> >
> > This DOES BREAK backwards compat, but we don't think the non-index
> > PHB feature was used in practice (at least libvirt doesn't) and the
> > simplification is worth it.
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> > RFC->v1: - as suggested dy David, updated the changelog to explicitely
> > mention that we intentionally break backwards compat.
> > ---
[...snip...]
> > +static void spapr_phb_instance_init(Object *obj)
> > +{
> > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(obj);
> > +
> > + sphb->mem64_win_addr = (hwaddr)-1;
> > +}
> > +
>
> Why do we need to initialize this field especially?
>
It is *somehow* explained in the commit log:
We have to introduce a PHB instance init function to initialize the
64-bit window address to -1 because pseries-2.7 and older machines
don't set it. [in the phb_placement hook]
/*
* We don't set the 64-bit MMIO window, relying on the PHB's
* fallback behaviour of automatically splitting a large "32-bit"
* window into contiguous 32-bit and 64-bit windows
*/
and spapr_phb_realize() doesn't set it either unless
sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE...
But thinking again, I guess I should add an else block in spapr_phb_realize()
instead.
I'll send a v2 (and I'll send the checkpatch fix along it if you don't
mind).
> > static void spapr_phb_class_init(ObjectClass *klass, void *data)
> > {
> > PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> > @@ -1960,6 +1927,7 @@ static const TypeInfo spapr_phb_info = {
> > .parent = TYPE_PCI_HOST_BRIDGE,
> > .instance_size = sizeof(sPAPRPHBState),
> > .class_init = spapr_phb_class_init,
> > + .instance_init = spapr_phb_instance_init,
> > .interfaces = (InterfaceInfo[]) {
> > { TYPE_HOTPLUG_HANDLER },
> > { }
> >
>
pgprXuycgBUvM.pgp
Description: OpenPGP digital signature