| Message ID | 20180518164405.11804-2-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: generic IRQ frontend | expand |
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. > Nevertheless, this is a compatibbility breakage that will be addressed ^^ typo > by the subsequent patches which introduce IRQ number allocator > handlers per machine version. > Indeed, this silently breaks the "irq" property of VIO devices, and I don't quite see how the other patches address this specific breakage. > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/spapr.h | 3 +-- > hw/ppc/spapr.c | 21 ++++++--------------- > hw/ppc/spapr_events.c | 7 +++---- > hw/ppc/spapr_vio.c | 2 +- > 4 files changed, 11 insertions(+), 22 deletions(-) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index d60b7c6d7a8b..2cfdfdd67eaf 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -773,8 +773,7 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu); > void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp); > PowerPCCPU *spapr_find_cpu(int vcpu_id); > > -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, > - Error **errp); > +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp); > int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi, > bool align, Error **errp); > void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 32ab3c43b6c0..05a924a5f2da 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi) > ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi); > } > > -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, > - Error **errp) > +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp) > { > ICSState *ics = spapr->ics; > int irq; > > assert(ics); > > - if (irq_hint) { > - if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) { > - error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint); > - return -1; > - } > - irq = irq_hint; > - } else { > - irq = ics_find_free_block(ics, 1, 1); > - if (irq < 0) { > - error_setg(errp, "can't allocate IRQ: no IRQ left"); > - return -1; > - } > - irq += ics->offset; > + irq = ics_find_free_block(ics, 1, 1); > + if (irq < 0) { > + error_setg(errp, "can't allocate IRQ: no IRQ left"); > + return -1; > } > + irq += ics->offset; > > spapr_irq_set_lsi(spapr, irq, lsi); > trace_spapr_irq_alloc(irq); > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 86836f0626dc..64a67439beac 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr) > spapr->event_sources = spapr_event_sources_new(); > > spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW, > - spapr_irq_alloc(spapr, 0, false, > - &error_fatal)); > + spapr_irq_alloc(spapr, false, &error_fatal)); > > /* NOTE: if machine supports modern/dedicated hotplug event source, > * we add it to the device-tree unconditionally. This means we may > @@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr) > */ > if (spapr->use_hotplug_event_source) { > spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG, > - spapr_irq_alloc(spapr, 0, false, > - &error_fatal)); > + spapr_irq_alloc(spapr, false, > + &error_fatal)); > } > > spapr->epow_notifier.notify = spapr_powerdown_req; > 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 :) > if (local_err) { > error_propagate(errp, local_err); > return;
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. Thomas
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 ? Thanks, C.
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 <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 ? 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? Thomas
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 <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 ? > > 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. C.
On Mon, 28 May 2018 11:20:36 +0200 Cédric Le Goater <clg@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 <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 ? > > > > 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. > C. >
On 05/28/2018 02:09 PM, Greg Kurz wrote: > On Mon, 28 May 2018 11:20:36 +0200 > Cédric Le Goater <clg@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 <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 ? >>> >>> 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.
On 05/18/2018 06:44 PM, Cédric Le Goater 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. > > Nevertheless, this is a compatibbility breakage that will be addressed > by the subsequent patches which introduce IRQ number allocator > handlers per machine version. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/spapr.h | 3 +-- > hw/ppc/spapr.c | 21 ++++++--------------- > hw/ppc/spapr_events.c | 7 +++---- > hw/ppc/spapr_vio.c | 2 +- > 4 files changed, 11 insertions(+), 22 deletions(-) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index d60b7c6d7a8b..2cfdfdd67eaf 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -773,8 +773,7 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu); > void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp); > PowerPCCPU *spapr_find_cpu(int vcpu_id); > > -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, > - Error **errp); > +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp); > int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi, > bool align, Error **errp); > void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num); > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 32ab3c43b6c0..05a924a5f2da 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi) > ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi); > } > > -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, > - Error **errp) > +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp) > { > ICSState *ics = spapr->ics; > int irq; > > assert(ics); > > - if (irq_hint) { > - if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) { Side note, this line is a promise for a nice segv if irq_hint < ics->offset. I think it is time to deprecate the 'irq' property of the sPAPR VIO devices. C. > - error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint); > - return -1; > - } > - irq = irq_hint; > - } else { > - irq = ics_find_free_block(ics, 1, 1); > - if (irq < 0) { > - error_setg(errp, "can't allocate IRQ: no IRQ left"); > - return -1; > - } > - irq += ics->offset; > + irq = ics_find_free_block(ics, 1, 1); > + if (irq < 0) { > + error_setg(errp, "can't allocate IRQ: no IRQ left"); > + return -1; > } > + irq += ics->offset; > > spapr_irq_set_lsi(spapr, irq, lsi); > trace_spapr_irq_alloc(irq); > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 86836f0626dc..64a67439beac 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr) > spapr->event_sources = spapr_event_sources_new(); > > spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW, > - spapr_irq_alloc(spapr, 0, false, > - &error_fatal)); > + spapr_irq_alloc(spapr, false, &error_fatal)); > > /* NOTE: if machine supports modern/dedicated hotplug event source, > * we add it to the device-tree unconditionally. This means we may > @@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr) > */ > if (spapr->use_hotplug_event_source) { > spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG, > - spapr_irq_alloc(spapr, 0, false, > - &error_fatal)); > + spapr_irq_alloc(spapr, false, > + &error_fatal)); > } > > spapr->epow_notifier.notify = spapr_powerdown_req; > 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); > if (local_err) { > error_propagate(errp, local_err); > return; >
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. I don't see a section for 'Deprecated' properties in the qemu-doc files. C.
>> 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. > > I don't see a section for 'Deprecated' properties in the qemu-doc files. So I added a new one, which gives us : B.7 Device options B.7.1 Block device options B.7.1.1 "backing": "" (since 2.12.0) B.7.2 vio-spapr-device device options B.7.2.1 "irq": "" (since 2.13.0) Cheers, C.
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.
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. > 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." 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. > 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. Thanks, C.
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#;
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 <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). 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.
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 <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). > > 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. >
On 06/14/2018 05:46 AM, David Gibson wrote: > 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 <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). >> >> 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. OK. so, we agree on these, at least. > 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. How do we know which IRQ number is being used or not ? The IRQ controller needs to know that. > 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. yes. > Thus we mirror the overall structure of the code in the allocator - hmm, kind of, yes. You need to define the ranges somewhere anyhow. > I think that's a worse > duplication that just setting the irq number in various places. but we need to do more than just that. The XICS, legacy and old, and the XIVE interrupt controller need to declare those IRQ numbers in their *status* tables or/and in their *routing* tables. I mean how would the KVM device work for instance if it didn't know anything about the layout ? The machine IRQs and its device IRQs are linked to the IRQ controller, we can not just ignore that. I really don't understand. Please take a look at the resulting three backends : https://github.com/legoater/qemu/blob/xive-3.0/hw/ppc/spapr_irq.c They do much more, they - initialize the IRQ controller (we could even imagine having a new backend when KVM is in use.) - define the ranges - alloc/free IRQ numbers *from* an IRQ controller POV - return the QEMU irq object - handle 'info pic' - populate the interrupt DT node - allocated the CPU interrupt presenter - handle IRQ migration quirks - define IRQ OV5 bits This needs a low latency discussion. C.
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index d60b7c6d7a8b..2cfdfdd67eaf 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -773,8 +773,7 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu); void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp); PowerPCCPU *spapr_find_cpu(int vcpu_id); -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, - Error **errp); +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp); int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi, bool align, Error **errp); void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 32ab3c43b6c0..05a924a5f2da 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3798,28 +3798,19 @@ static void spapr_irq_set_lsi(sPAPRMachineState *spapr, int irq, bool lsi) ics_set_irq_type(spapr->ics, irq - spapr->ics->offset, lsi); } -int spapr_irq_alloc(sPAPRMachineState *spapr, int irq_hint, bool lsi, - Error **errp) +int spapr_irq_alloc(sPAPRMachineState *spapr, bool lsi, Error **errp) { ICSState *ics = spapr->ics; int irq; assert(ics); - if (irq_hint) { - if (!ICS_IRQ_FREE(ics, irq_hint - ics->offset)) { - error_setg(errp, "can't allocate IRQ %d: already in use", irq_hint); - return -1; - } - irq = irq_hint; - } else { - irq = ics_find_free_block(ics, 1, 1); - if (irq < 0) { - error_setg(errp, "can't allocate IRQ: no IRQ left"); - return -1; - } - irq += ics->offset; + irq = ics_find_free_block(ics, 1, 1); + if (irq < 0) { + error_setg(errp, "can't allocate IRQ: no IRQ left"); + return -1; } + irq += ics->offset; spapr_irq_set_lsi(spapr, irq, lsi); trace_spapr_irq_alloc(irq); diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 86836f0626dc..64a67439beac 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -712,8 +712,7 @@ void spapr_events_init(sPAPRMachineState *spapr) spapr->event_sources = spapr_event_sources_new(); spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW, - spapr_irq_alloc(spapr, 0, false, - &error_fatal)); + spapr_irq_alloc(spapr, false, &error_fatal)); /* NOTE: if machine supports modern/dedicated hotplug event source, * we add it to the device-tree unconditionally. This means we may @@ -725,8 +724,8 @@ void spapr_events_init(sPAPRMachineState *spapr) */ if (spapr->use_hotplug_event_source) { spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_HOT_PLUG, - spapr_irq_alloc(spapr, 0, false, - &error_fatal)); + spapr_irq_alloc(spapr, false, + &error_fatal)); } spapr->epow_notifier.notify = spapr_powerdown_req; 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); if (local_err) { error_propagate(errp, local_err); return;
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. Nevertheless, this is a compatibbility breakage that will be addressed by the subsequent patches which introduce IRQ number allocator handlers per machine version. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/spapr.h | 3 +-- hw/ppc/spapr.c | 21 ++++++--------------- hw/ppc/spapr_events.c | 7 +++---- hw/ppc/spapr_vio.c | 2 +- 4 files changed, 11 insertions(+), 22 deletions(-)