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 <c...@kaod.org> 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.
