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

[Cédric Le Goater]

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.

> 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." 

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. 

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

Thanks,

C.