On 05/28/2018 02:09 PM, Greg Kurz wrote: > On Mon, 28 May 2018 11:20:36 +0200 > Cédric Le Goater <c...@kaod.org> 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 <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 ? >>> >>> 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.
