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

[David Gibson]

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.

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.

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.  Thus we mirror the overall
structure of the code in the allocator - I think that's a worse
duplication that just setting the irq number in various places.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature