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

[Cédric Le Goater]

On 06/14/2018 05:46 AM, David Gibson wrote:
> On Wed, Jun 13, 2018 at 09:24:12AM +0200, Cédric Le Goater wrote:
>> 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.
> 
> Right, but this obscures the fact that the function of assign/claim is
> also performed at the end of the alloc function.
> 
>>>> 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.
> 
> Well, we won't, in a sense, since we want to phase out the find()
> part.
> 
>>>>> 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,
> 
> I see that, but I think it's a bad idea.  We have need for variants of
> both the find() and claim() operations, but they vary based on
> different things.  Combining them into one abstraction point just
> muddies that.
> 
> In particular, we're aiming to allow runtime switching of xics
> vs. xive, for which we require the same irq allocations in both.
> Combining the "claim" backend with the allocation makes it much less
> obvious that we really do have the same allocations in both cases.
> 
>> it will give us a clear view of the IRQ number space usage 
>> instead of spreading the definitions in the code. 
> 
> Yeah.. I really think centralizing the management of the IRQ space is
> an anti goal here.  Well, other than comments and #defines for the
> sub-range bases.

OK. so, we agree on these, at least.

> Kind of the whole point of the fixed allocation scheme is so that we
> don't need any centralized management, each device can just figure out
> its own irq based only on static, read-only information.

How do we know which IRQ number is being used or not ? The IRQ controller
needs to know that. 

> To put the allocation back into a common routine requires explicit
> enumeration of of the various subsystems, to be passed from the
> subsystem into the central allocator.  

yes.

> Thus we mirror the overall structure of the code in the allocator - 

hmm, kind of, yes. You need to define the ranges somewhere anyhow.

> I think that's a worse
> duplication that just setting the irq number in various places.

but we need to do more than just that. The XICS, legacy and old, and 
the XIVE interrupt controller need to declare those IRQ numbers in 
their *status* tables or/and in their *routing* tables. I mean how
would the KVM device work for instance if it didn't know anything 
about the layout ? 

The machine IRQs and its device IRQs are linked to the IRQ controller, 
we can not just ignore that. I really don't understand.

Please take a look at the resulting three backends :
 
https://github.com/legoater/qemu/blob/xive-3.0/hw/ppc/spapr_irq.c

They do much more, they

 - initialize the IRQ controller (we could even imagine having a 
   new backend when KVM is in use.)
 - define the ranges
 - alloc/free IRQ numbers *from* an IRQ controller POV
 - return the QEMU irq object
 - handle 'info pic'
 - populate the interrupt DT node
 - allocated the CPU interrupt presenter
 - handle IRQ migration quirks
 - define IRQ OV5 bits

This needs a low latency discussion.

C.