On Thu, 14 Sep 2017 22:31:26 +1000 David Gibson <[email protected]> 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 <[email protected]> > > --- > > 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 }, > > { } > > >
pgpQ4cirkN1W9.pgp
Description: OpenPGP digital signature
