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

[David Gibson]

On Tue, Sep 19, 2017 at 04:03:57PM +0200, Greg Kurz wrote:
> On Tue, 19 Sep 2017 09:03:58 +1000
> David Gibson <address@hidden> wrote:
> 
> > On Thu, Sep 14, 2017 at 04:14:44PM +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  
> > 
> > So, this is true, but it's kind of missing the point; we could fix
> > those things if we wanted.  The real point here is that the non-index
> > option is more trouble than it's worth: it's difficult to use, keeps
> > requiring (potentially incompatible) changes when some new parameter
> > needs adding, and is awkward to check for collisions.
> > 
> 
> Ok, I'll rewrite the changelog accordingly in the next spin.
> 
> > > 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>
> > > ---
> > > v1->v2: - error out if mem64_win_pciaddr is set but mem64_win_size
> > >           isn't
> > >         - set mem64_win_addr to -1 for old configuration with 32-bit
> > >           window below 2G in spapr_phb_realize()
> > >         - drop instance init function
> > > 
> > > RFC->v1: - as suggested dy David, updated the changelog to explicitely
> > >            mention that we intentionally break backwards compat.
> > > ---
> > >  hw/ppc/spapr_pci.c |   53 
> > > +++++++++++-----------------------------------------
> > >  1 file changed, 11 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index cf54160526fa..024638e18b53 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -1523,16 +1523,6 @@ static void spapr_phb_realize(DeviceState *dev, 
> > > Error **errp)
> > >          sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> > >          Error *local_err = NULL;
> > >  
> > > -        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn[0] != 
> > > (uint32_t)-1)
> > > -            || (sphb->dma_liobn[1] != (uint32_t)-1 && windows_supported 
> > > == 2)
> > > -            || (sphb->mem_win_addr != (hwaddr)-1)
> > > -            || (sphb->mem64_win_addr != (hwaddr)-1)
> > > -            || (sphb->io_win_addr != (hwaddr)-1)) {
> > > -            error_setg(errp, "Either \"index\" or other parameters must"
> > > -                       " be specified for PAPR PHB, not both");
> > > -            return;
> > > -        }
> > > -
> > >          smc->phb_placement(spapr, sphb->index,
> > >                             &sphb->buid, &sphb->io_win_addr,
> > >                             &sphb->mem_win_addr, &sphb->mem64_win_addr,
> > > @@ -1541,36 +1531,12 @@ static void spapr_phb_realize(DeviceState *dev, 
> > > Error **errp)
> > >              error_propagate(errp, local_err);
> > >              return;
> > >          }
> > > -    }
> > > -
> > > -    if (sphb->buid == (uint64_t)-1) {
> > > -        error_setg(errp, "BUID not specified for PHB");
> > > -        return;
> > > -    }
> > > -
> > > -    if ((sphb->dma_liobn[0] == (uint32_t)-1) ||
> > > -        ((sphb->dma_liobn[1] == (uint32_t)-1) && (windows_supported > 
> > > 1))) {
> > > -        error_setg(errp, "LIOBN(s) not specified for PHB");
> > > -        return;
> > > -    }
> > > -
> > > -    if (sphb->mem_win_addr == (hwaddr)-1) {
> > > -        error_setg(errp, "Memory window address not specified for PHB");
> > > -        return;
> > > -    }
> > > -
> > > -    if (sphb->io_win_addr == (hwaddr)-1) {
> > > -        error_setg(errp, "IO window address not specified for PHB");
> > > +    } else {
> > > +        error_setg(errp, "\"index\" for PAPR PHB is mandatory");
> > >          return;
> > >      }
> > >  
> > >      if (sphb->mem64_win_size != 0) {
> > > -        if (sphb->mem64_win_addr == (hwaddr)-1) {
> > > -            error_setg(errp,
> > > -                       "64-bit memory window address not specified for 
> > > PHB");
> > > -            return;
> > > -        }
> > > -
> > >          if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
> > >              error_setg(errp, "32-bit memory window of size 
> > > 0x%"HWADDR_PRIx
> > >                         " (max 2 GiB)", sphb->mem_win_size);  
> > 
> > Some of these checks could probably just become assert()s now, where
> > they're things determined by the placement code and no longer have
> > parameters, a problem indicates a bug in the machine rather than a
> > user error.
> > 
> 
> All the *_size properties are still user settable, as before... should we
> make them internal and have the placement hook initialize them instead ?

Ah, good point.  I guess that still makes sense.

> > > @@ -1581,6 +1547,9 @@ static void spapr_phb_realize(DeviceState *dev, 
> > > Error **errp)
> > >              /* 64-bit window defaults to identity mapping */
> > >              sphb->mem64_win_pciaddr = sphb->mem64_win_addr;
> > >          }
> > > +    } else if (sphb->mem64_win_pciaddr != (hwaddr) -1) {
> > > +        error_setg(errp, "64-bit memory window requires 
> > > \"mem64_win_size\"");
> > > +        return;  
> > 
> > Likewise this new one.
> > 
> 
> mem64_win_pciaddr is still user settable as well...

That doesn't sound like a good idea.  If we're making index mandatory,
we might has well have it set *all* the windows.

> 
> > >      } else if (sphb->mem_win_size > SPAPR_PCI_MEM32_WIN_SIZE) {
> > >          /*
> > >           * For compatibility with old configuration, if no 64-bit MMIO
> > > @@ -1594,6 +1563,12 @@ static void spapr_phb_realize(DeviceState *dev, 
> > > Error **errp)
> > >          sphb->mem64_win_pciaddr =
> > >              SPAPR_PCI_MEM_WIN_BUS_OFFSET + SPAPR_PCI_MEM32_WIN_SIZE;
> > >          sphb->mem_win_size = SPAPR_PCI_MEM32_WIN_SIZE;
> > > +    } else {
> > > +        /* Old configuration with the 32-bit MMIO window <= 2GiB don't 
> > > need a
> > > +         * 64-bit MMIO window.
> > > +         */  
> > 
> > Wouldn't it be easier to just change the test below to check for size
> > != 0 instead of checking for addr != -1 as it does now.
> > 
> 
> Yeah indeed.
> 
> > > +        sphb->mem64_win_addr = (hwaddr) -1;
> > > +        sphb->mem64_win_pciaddr = (hwaddr) -1;
> > >      }
> > >  
> > >      if (spapr_pci_find_phb(spapr, sphb->buid)) {
> > > @@ -1789,18 +1764,12 @@ static void spapr_phb_reset(DeviceState *qdev)
> > >  
> > >  static Property spapr_phb_properties[] = {
> > >      DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
> > > -    DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
> > > -    DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn[0], -1),
> > > -    DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
> > > -    DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
> > >      DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
> > >                         SPAPR_PCI_MEM32_WIN_SIZE),
> > > -    DEFINE_PROP_UINT64("mem64_win_addr", sPAPRPHBState, mem64_win_addr, 
> > > -1),
> > >      DEFINE_PROP_UINT64("mem64_win_size", sPAPRPHBState, mem64_win_size,
> > >                         SPAPR_PCI_MEM64_WIN_SIZE),
> > >      DEFINE_PROP_UINT64("mem64_win_pciaddr", sPAPRPHBState, 
> > > mem64_win_pciaddr,
> > >                         -1),
> > > -    DEFINE_PROP_UINT64("io_win_addr", sPAPRPHBState, io_win_addr, -1),
> > >      DEFINE_PROP_UINT64("io_win_size", sPAPRPHBState, io_win_size,
> > >                         SPAPR_PCI_IO_WIN_SIZE),
> > >      DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, 
> > > dr_enabled,
> > >   
> > 
> 
> 
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature