Re: [PATCH 13/20] spapr: Eliminate SpaprIrq:get_nodename method

[Greg Kurz]

On Wed, 25 Sep 2019 16:45:27 +1000
David Gibson <address@hidden> wrote:

> This method is used to determine the name of the irq backend's node in the
> device tree, so that we can find its phandle (after SLOF may have modified
> it from the phandle we initially gave it).
> 
> But, in the two cases the only difference between the node name is the
> presence of a unit address.  Searching for a node name without considering
> unit address is standard practice for the device tree, and
> fdt_subnode_offset() will do exactly that.
> 
> So, the method is unnecessary.
> 

So is the XICS_NODENAME macro which was introduced by the same
commit 743ed566c1d80, and it seems that "interrupt-controller"
is a well-known string that is used everywhere:

[greg@bahia qemu-spapr]$ git grep -E \"interrupt-controller\"
hw/arm/virt.c:    qemu_fdt_setprop(vms->fdt, nodename, "interrupt-controller", 
NULL, 0);
hw/arm/xlnx-versal-virt.c:    qemu_fdt_setprop(s->fdt, nodename, 
"interrupt-controller", NULL, 0);
hw/intc/sh_intc.c:                          "interrupt-controller", 
0x100000000ULL);
hw/intc/spapr_xive.c:    _FDT(fdt_setprop(fdt, node, "interrupt-controller", 
NULL, 0));
hw/intc/xics_spapr.c:    _FDT(fdt_setprop(fdt, node, "interrupt-controller", 
NULL, 0));
hw/pci/pci.c:    { 0x0800, "Interrupt controller", "interrupt-controller"},
hw/ppc/e500.c:    qemu_fdt_setprop(fdt, mpic, "interrupt-controller", NULL, 0);
hw/ppc/pnv.c:    _FDT((fdt_setprop(fdt, offset, "interrupt-controller", NULL, 
0)));
hw/ppc/spapr_events.c:    _FDT((fdt_setprop(fdt, event_sources, 
"interrupt-controller", NULL, 0)));
hw/ppc/spapr_irq.c:    const char *nodename = "interrupt-controller";
hw/ppc/spapr_pci.c:    { PCI_CLASS_SYSTEM_PIC, "interrupt-controller", 
pic_iface },
hw/ppc/spapr_vio.c:    _FDT(fdt_setprop(fdt, node, "interrupt-controller", 
NULL, 0));
hw/riscv/sifive_u.c:        qemu_fdt_setprop(fdt, intc, "interrupt-controller", 
NULL, 0);
hw/riscv/sifive_u.c:    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", 
NULL, 0);
hw/riscv/spike.c:        qemu_fdt_setprop(fdt, intc, "interrupt-controller", 
NULL, 0);
hw/riscv/virt.c:        qemu_fdt_setprop(fdt, intc, "interrupt-controller", 
NULL, 0);
hw/riscv/virt.c:    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", 
NULL, 0);
include/hw/ppc/spapr.h: * "interrupt-controller" node has its 
"#interrupt-cells" property set to 2 (ie,
include/hw/ppc/xics_spapr.h:#define XICS_NODENAME "interrupt-controller"

Maybe drop XICS_NODENAME as well while here ?

With or without that,

Reviewed-by: Greg Kurz <address@hidden>

> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr_irq.c         | 25 +++----------------------
>  include/hw/ppc/spapr_irq.h |  1 -
>  2 files changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 038bfffff4..79167ccc68 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -211,11 +211,6 @@ static void spapr_irq_reset_xics(SpaprMachineState 
> *spapr, Error **errp)
>      }
>  }
>  
> -static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
> -{
> -    return XICS_NODENAME;
> -}
> -
>  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> @@ -237,7 +232,6 @@ SpaprIrq spapr_irq_xics = {
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
>      .set_irq     = spapr_irq_set_irq_xics,
> -    .get_nodename = spapr_irq_get_nodename_xics,
>      .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
> @@ -362,11 +356,6 @@ static void spapr_irq_set_irq_xive(void *opaque, int 
> irq, int val)
>      }
>  }
>  
> -static const char *spapr_irq_get_nodename_xive(SpaprMachineState *spapr)
> -{
> -    return spapr->xive->nodename;
> -}
> -
>  static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> @@ -393,7 +382,6 @@ SpaprIrq spapr_irq_xive = {
>      .post_load   = spapr_irq_post_load_xive,
>      .reset       = spapr_irq_reset_xive,
>      .set_irq     = spapr_irq_set_irq_xive,
> -    .get_nodename = spapr_irq_get_nodename_xive,
>      .init_kvm    = spapr_irq_init_kvm_xive,
>  };
>  
> @@ -538,11 +526,6 @@ static void spapr_irq_set_irq_dual(void *opaque, int 
> irq, int val)
>      spapr_irq_current(spapr)->set_irq(spapr, irq, val);
>  }
>  
> -static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
> -{
> -    return spapr_irq_current(spapr)->get_nodename(spapr);
> -}
> -
>  /*
>   * Define values in sync with the XIVE and XICS backend
>   */
> @@ -560,7 +543,6 @@ SpaprIrq spapr_irq_dual = {
>      .post_load   = spapr_irq_post_load_dual,
>      .reset       = spapr_irq_reset_dual,
>      .set_irq     = spapr_irq_set_irq_dual,
> -    .get_nodename = spapr_irq_get_nodename_dual,
>      .init_kvm    = NULL, /* should not be used */
>  };
>  
> @@ -697,13 +679,13 @@ void spapr_irq_reset(SpaprMachineState *spapr, Error 
> **errp)
>  
>  int spapr_irq_get_phandle(SpaprMachineState *spapr, void *fdt, Error **errp)
>  {
> -    const char *nodename = spapr->irq->get_nodename(spapr);
> +    const char *nodename = "interrupt-controller";
>      int offset, phandle;
>  
>      offset = fdt_subnode_offset(fdt, 0, nodename);
>      if (offset < 0) {
> -        error_setg(errp, "Can't find node \"%s\": %s", nodename,
> -                   fdt_strerror(offset));
> +        error_setg(errp, "Can't find node \"%s\": %s",
> +                   nodename, fdt_strerror(offset));
>          return -1;
>      }
>  
> @@ -787,6 +769,5 @@ SpaprIrq spapr_irq_xics_legacy = {
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
>      .set_irq     = spapr_irq_set_irq_xics,
> -    .get_nodename = spapr_irq_get_nodename_xics,
>      .init_kvm    = spapr_irq_init_kvm_xics,
>  };
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index a4e790ef60..9b60378e28 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,7 +52,6 @@ typedef struct SpaprIrq {
>      int (*post_load)(SpaprMachineState *spapr, int version_id);
>      void (*reset)(SpaprMachineState *spapr, Error **errp);
>      void (*set_irq)(void *opaque, int srcno, int val);
> -    const char *(*get_nodename)(SpaprMachineState *spapr);
>      void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>  } SpaprIrq;
>