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

[Cédric Le Goater]

On 06/13/2018 06:27 AM, David Gibson wrote:
> 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 <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.
> 
> 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.

To be honest, this is very much discouraging. 

C.