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

[Cédric Le Goater]

On 05/28/2018 09:18 AM, Thomas Huth wrote:
> On 28.05.2018 09:06, 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 ?
> 
> I think that would be fine, too. You likely need to keep the settable
> IRQs around for the old machines anyway, to make sure that migration of
> the old machine types still works, right?

Yes, that is the goal of patch 3. It introduces a common sPAPR IRQ frontend,
the first backend being the current one.

C.