Re: [Qemu-devel] [PATCH 1/4] spapr: remove irq_hint parameter from spapr_irq_alloc()

[Cédric Le Goater]

On 06/13/2018 06:22 AM, David Gibson wrote:
> On Tue, Jun 05, 2018 at 08:41:13AM +0200, Cédric Le Goater wrote:
>> On 06/05/2018 05:34 AM, David Gibson wrote:
>>> On Mon, May 28, 2018 at 09:06:12AM +0200, Cédric Le Goater wrote:
>>>> On 05/28/2018 08:17 AM, Thomas Huth wrote:
>>>>> On 25.05.2018 16:02, Greg Kurz wrote:
>>>>>> On Fri, 18 May 2018 18:44:02 +0200
>>>>>> Cédric Le Goater <address@hidden> wrote:
>>>>>>
>>>>>>> This IRQ number hint can possibly be used by the VIO devices if the
>>>>>>> "irq" property is defined on the command line but it seems it is never
>>>>>>> the case. It is not used in libvirt for instance. So, let's remove it
>>>>>>> to simplify future changes.
>>>>>>>
>>>>>>
>>>>>> Setting an irq manually looks a bit anachronistic. I doubt anyone would
>>>>>> do that nowadays, and the patch does a nice cleanup. So this looks like
>>>>>> a good idea.
>>>>> [...]
>>>>>>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>>>>>>> index 472dd6f33a96..cc064f64fccf 100644
>>>>>>> --- a/hw/ppc/spapr_vio.c
>>>>>>> +++ b/hw/ppc/spapr_vio.c
>>>>>>> @@ -455,7 +455,7 @@ static void spapr_vio_busdev_realize(DeviceState 
>>>>>>> *qdev, Error **errp)
>>>>>>>          dev->qdev.id = id;
>>>>>>>      }
>>>>>>>  
>>>>>>> -    dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
>>>>>>> +    dev->irq = spapr_irq_alloc(spapr, false, &local_err);
>>>>>>
>>>>>> Silently breaking "irq" like this looks wrong. I'd rather officially 
>>>>>> remove
>>>>>> it first (ie, kill spapr_vio_props, -5 lines in spapr_vio.c).
>>>>>>
>>>>>> Of course, this raises the question of interface deprecation, and it 
>>>>>> should
>>>>>> theoretically follow the process described at:
>>>>>>
>>>>>> https://wiki.qemu.org/Features/LegacyRemoval#Rules_for_removing_an_interface
>>>>>>
>>>>>> Cc'ing Thomas, our Chief Deprecation Officer, for insights :)
>>>>>
>>>>> The property is a public interface. Just because it's not used by
>>>>> libvirt does not mean that nobody is using it. So yes, please follow the
>>>>> rules and mark it as deprecated first for two release, before you really
>>>>> remove it.
>>>>
>>>> This "irq" property is a problem to introduce a new static layout of IRQ 
>>>> numbers. It is in complete opposition. 
>>>>
>>>> Can we keep it as it is for old pseries machine (settable) and ignore it 
>>>> for newer ? Would that be fine ?
>>>
>>> So, Thomas is right that we need to keep the interface while we go
>>> through the deprecation process, even though it's a bit of a pain
>>> (like you, I seriously doubt anyone ever used it).
>>
>> That's OK. The patch is simple. But it means that we have to keep the 
>> irq_hint parameter for 2 QEMU versions.
> 
> No.. the suggestion below is designed to avoid that..
> 
>>> But, I think there's a way to avoid that getting in the way of your
>>> cleanups too much.
>>>
>>> A bunch of the current problems are caused because spapr_irq_alloc()
>>> conflates two meanings of "allocate": 1) finding a free irq to use for
>>> this device and 2) assigning that irq exclusively to this device.
>>>
>>> I think the first thing to do is to split those two parts.  (1) will
>>> never take an irq parameter, (2) will always take an irq parameter.
>>> To implement the (to be deprecated) "irq" property on vio devices you
>>> should skip (1) and just call (2) with the given irq number.
>>
>> well, we need to call both because if "irq" is zero then when we 
>> fallback to "1) finding a free irq to use."
> 
> No, basically in the VIO code itself you'd have:
>       irq = <irq property value>;
>       if (!irq)
>               irq = find_irq()
>       claim_irq(irq);
>
> find_irq() never takes a hint, claim_irq() always does (except it's
> not really a hint).

ok. I add something like that in mind : 
 
    if (dev->irq) {
        spapr_irq_assign(spapr, SPAPR_IRQ_VIO, dev->irq, &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }
    } else {
        dev->irq = spapr_irq_alloc(spapr, SPAPR_IRQ_VIO, vio_index++,  
                                   &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

and spapr_irq_assign() would die when the vio "irq" property does.

>> But we can move the exclusive IRQ assignment (2) under the VIO model 
>> which is the only one using it and start deprecating the property.
> 
> No.. the exclusive claim would be global - everything would use that.

Yes, I see the model. I am not sure it's useful to have two routines
in the long term.

>>> The point of this series is to basically get rid of (1), but this
>>> first step means we don't need to worry about the hint parameter as we
>>> gradually remove it.
>>
>> OK. I think I got what you are asking for. (2) means adding an extra 
>> handler to the sPAPR IRQ interface, which would always fail in the
>> new XICS sPAPR IRQ backend using static numbers.
> 
> No.. (2), "claim_irq()" as I called it above, would _always_ be used.
> find_irq() would only be used to implement the legacy allocation.
> In various places we'll have code like this:
> 
>       if (legacy) {
>               irq = find_irq();
>       } else {
>               irq = <fixed value or formula>;
>       }
>       claim_irq(irq);

I rather hide all this behind a class machine operation doing the 
allocation, it will give us a clear view of the IRQ number space usage 
instead of spreading the definitions in the code. 

we will need something for XIVE any how.

> Where that fixed value could be something like:
>       irq = PCI_LSI_BASE + phb->index*4 + pin#;
> 

If you use a different class machine operation for allocation claim_irq() 
is not needed at all. The only case to handle is the VIO "irq" property 
which requires and extra operation. 

C.