Re: [Qemu-devel] [PATCH 2/4] sparp_pci: simplify how the PCI LSIs are allocated

[David Gibson <david@gibson.dropbear.id.au>]

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

On Sat, May 26, 2018 at 11:40:23AM +0200, Greg Kurz wrote:
> On Fri, 18 May 2018 18:44:03 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > PCI LSIs are today allocated one by one using the IRQ alloc_block
> > routine. Change the code sequence to first allocate a PCI_NUM_PINS
> > block. It will help us providing a generic IRQ framework to the
> > machine.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  hw/ppc/spapr_pci.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 39a14980d397..4fd97ffe4c6e 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1546,6 +1546,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >      sPAPRTCETable *tcet;
> >      const unsigned windows_supported =
> >          sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> > +    uint32_t irq;
> > +    Error *local_err = NULL;
> >  
> >      if (!spapr) {
> >          error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries machine");
> > @@ -1694,18 +1696,15 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
> >      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
> >  
> >      /* Initialize the LSI table */
> > -    for (i = 0; i < PCI_NUM_PINS; i++) {
> > -        uint32_t irq;
> > -        Error *local_err = NULL;
> > -
> > -        irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
> > -        if (local_err) {
> > -            error_propagate(errp, local_err);
> > -            error_prepend(errp, "can't allocate LSIs: ");
> > -            return;
> > -        }
> > +    irq = spapr_irq_alloc_block(spapr, PCI_NUM_PINS, true, false, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        error_prepend(errp, "can't allocate LSIs: ");
> > +        return;
> > +    }
> >  
> 
> It isn't strictly equivalent. The current code would be happy with
> sparse IRQ numbers, while the proposed one wouldn't... Anyway, this
> cannot happen since we don't have PHB hotplug.

This makes me pretty nervous, because it's not obvious it will come up
with the same numbers in all circumstances, which we have to do for
existing machine types.  It's also not obvious to me why it's useful
to go via this step before going straight to static allocation of the
irq numbers.

If you can convince me this will (in practice) return the same numbers
as the existing code for all valid setups, and that it's a useful
intermediate step, then I'll apply it.

> 
> > -        sphb->lsi_table[i].irq = irq;
> > +    for (i = 0; i < PCI_NUM_PINS; i++) {
> > +        sphb->lsi_table[i].irq = irq + i;
> >      }
> >  
> >      /* allocate connectors for child PCI devices */
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]