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: 4575 bytes --]

On Tue, Jun 05, 2018 at 08:31:49AM +0200, Cédric Le Goater wrote:
> On 06/05/2018 05:44 AM, David Gibson wrote:
> > 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.
> 
> Given that : 
> 
>  - irq_hint is "unused"
>  - all IRQs are allocated sequentially at machine init,  
>  - spapr_pci is the only model using the block allocation for MSIs, 
>    potentially fragmenting more the IRQ number space but done at 
>    guest runtime. 
>  - the PHB LSI are the allocated at realize time doing the loop above, 
>  - we don't support PHB hotplug 
>  - we do support PHB coldplug but then the IRQ allocation is done
>    at machine time,
> 
> it seems highly improbable that the IRQ number space is fragmented
> to a point which would not allow the loop above to return four 
> contiguous IRQ numbers, always.

Well, assuming irq_hint really is unused, that's right.  But we can't
assume that - that's the whole point of the deprecation thing.

Given that, AIUI, just one vio device with irq= set to a value that
would be within an LSI block allocated under the old scheme would
result in the new scheme returning a non-contiguous set of LSIs -
i.e. a different result from what we used to have.

> That is why I felt confident changing the loop to a single block 
> allocation. 
> 
> > 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.
> 
> It pollutes the new sPAPR IRQ interface API with an extra parameter 
> to support both underlying backend and it complexifies the code 
> to handle block allocation of a single IRQ (like above) within an 
> IRQ range (the PCI LSIs).
> 
> So you end up having a family, a device index, a count, an alignment,
> and an index within the range. pffut.
> 
> Also, could we kill the alignment ?

Since we sometimes pass 'true', no, we can't, without changing the
existing pattern of allocations, which we can't do.
> 

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