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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4867 bytes --]

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 <clg@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).

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

> > 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);

Where that fixed value could be something like:
	irq = PCI_LSI_BASE + phb->index*4 + pin#;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]