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

[Cédric Le Goater]

On 05/28/2018 02:09 PM, Greg Kurz wrote:
> On Mon, 28 May 2018 11:20:36 +0200
> Cédric Le Goater <address@hidden> wrote:
> 
>> 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.
>>
> 
> If we keep "irq" but we ignore it with newer machine types, we should at
> least print a warning then IMHO.

May be we can deprecate at the same time. I will take a look.

Thanks,

C.