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

[Greg Kurz <groug@kaod.org>]

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;