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

[Cédric Le Goater]

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 <address@hidden> 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 <address@hidden>
>>> ---
>>>  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. 

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 ?

C.  

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