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: 3254 bytes --]

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

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.

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.

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