Re: [Qemu-devel] [PATCH] spapr_pci: make index property mandatory

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

On Thu, 14 Sep 2017 22:31:26 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org>
> > ---
> > 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 },
> >          { }
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 195 bytes --]