Re: [PATCH for-6.0 6/8] spapr/xics: Add "nr-servers" property

[Cédric Le Goater]

On 11/20/20 6:46 PM, Greg Kurz wrote:
> The sPAPR ICS device exposes the range of vCPU ids it can handle in
> the "ibm,interrupt-server-ranges" FDT property. The highest vCPU
> id, ie. spapr_max_server_number(), is obtained from the machine
> through the "nr_servers" argument of the generic spapr_irq_dt() call.
> 
> We want to drop the "nr_servers" argument from the API because it
> doesn't make sense for the sPAPR XIVE device actually.
> 
> On POWER9, we also pass the highest vCPU id to the KVM XICS-on-XIVE
> device, in order to optimize resource allocation in the HW.
> 
> This is enough motivation to introduce an "nr-servers" property
> and to use it for both purposes.

I don't see a strong justification for storing this information at
the interrupt controller level. If it can be kept at the machine 
level, like it is today, I think it's better.

C. 

 
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/spapr.h      |  4 ++--
>  include/hw/ppc/xics_spapr.h | 21 +++++++++++++++++---
>  hw/intc/xics_kvm.c          |  2 +-
>  hw/intc/xics_spapr.c        | 38 ++++++++++++++++++++++++-------------
>  hw/ppc/spapr.c              |  5 +++--
>  hw/ppc/spapr_irq.c          |  7 +++++--
>  6 files changed, 54 insertions(+), 23 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfbdc..b76c84a2f884 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -10,7 +10,7 @@
>  #include "hw/ppc/spapr_irq.h"
>  #include "qom/object.h"
>  #include "hw/ppc/spapr_xive.h"  /* For SpaprXive */
> -#include "hw/ppc/xics.h"        /* For ICSState */
> +#include "hw/ppc/xics_spapr.h"  /* For IcsSpaprState */
>  #include "hw/ppc/spapr_tpm_proxy.h"
>  
>  struct SpaprVioBus;
> @@ -230,7 +230,7 @@ struct SpaprMachineState {
>      SpaprIrq *irq;
>      qemu_irq *qirqs;
>      SpaprInterruptController *active_intc;
> -    ICSState *ics;
> +    IcsSpaprState *ics;
>      SpaprXive *xive;
>  
>      bool cmd_line_caps[SPAPR_CAP_NUM];
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> index de752c0d2c7e..1a483a873b62 100644
> --- a/include/hw/ppc/xics_spapr.h
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -28,12 +28,27 @@
>  #define XICS_SPAPR_H
>  
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
>  #include "qom/object.h"
>  
> +typedef struct IcsSpaprState {
> +    /*< private >*/
> +    ICPState parent_obj;
> +
> +    /*
> +     * The ICS needs to know the upper limit to vCPU ids it
> +     * might be exposed to in order to size the vCPU id range
> +     * in "ibm,interrupt-server-ranges" and to optimize HW
> +     * resource allocation when using the XICS-on-XIVE KVM
> +     * device. It is the purpose of the "nr-servers" property
> +     * which *must* be set to a non-null value before realizing
> +     * the ICS.
> +     */
> +    uint32_t nr_servers;
> +} IcsSpaprState;
> +
>  #define TYPE_ICS_SPAPR "ics-spapr"
> -/* This is reusing the ICSState typedef from TYPE_ICS */
> -DECLARE_INSTANCE_CHECKER(ICSState, ICS_SPAPR,
> -                         TYPE_ICS_SPAPR)
> +DECLARE_INSTANCE_CHECKER(IcsSpaprState, ICS_SPAPR, TYPE_ICS_SPAPR)
>  
>  int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>                       Error **errp);
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 570d635bcc08..ecbbda0e249b 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -350,7 +350,7 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>  int xics_kvm_connect(SpaprInterruptController *intc, uint32_t nr_servers,
>                       Error **errp)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>      int rc;
>      CPUState *cs;
>      Error *local_err = NULL;
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 8ae4f41459c3..ce147e8980ed 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -34,6 +34,7 @@
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/xics_spapr.h"
>  #include "hw/ppc/fdt.h"
> +#include "hw/qdev-properties.h"
>  #include "qapi/visitor.h"
>  
>  /*
> @@ -154,7 +155,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>                            uint32_t nargs, target_ulong args,
>                            uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->ics;
> +    ICSState *ics = ICS(spapr->ics);
>      uint32_t nr, srcno, server, priority;
>  
>      CHECK_EMULATED_XICS_RTAS(spapr, rets);
> @@ -189,7 +190,7 @@ static void rtas_get_xive(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>                            uint32_t nargs, target_ulong args,
>                            uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->ics;
> +    ICSState *ics = ICS(spapr->ics);
>      uint32_t nr, srcno;
>  
>      CHECK_EMULATED_XICS_RTAS(spapr, rets);
> @@ -221,7 +222,7 @@ static void rtas_int_off(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>                           uint32_t nargs, target_ulong args,
>                           uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->ics;
> +    ICSState *ics = ICS(spapr->ics);
>      uint32_t nr, srcno;
>  
>      CHECK_EMULATED_XICS_RTAS(spapr, rets);
> @@ -254,7 +255,7 @@ static void rtas_int_on(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>                          uint32_t nargs, target_ulong args,
>                          uint32_t nret, target_ulong rets)
>  {
> -    ICSState *ics = spapr->ics;
> +    ICSState *ics = ICS(spapr->ics);
>      uint32_t nr, srcno;
>  
>      CHECK_EMULATED_XICS_RTAS(spapr, rets);
> @@ -285,10 +286,13 @@ static void rtas_int_on(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>  
>  static void ics_spapr_realize(DeviceState *dev, Error **errp)
>  {
> -    ICSState *ics = ICS_SPAPR(dev);
> -    ICSStateClass *icsc = ICS_GET_CLASS(ics);
> +    IcsSpaprState *sics = ICS_SPAPR(dev);
> +    ICSStateClass *icsc = ICS_GET_CLASS(dev);
>      Error *local_err = NULL;
>  
> +    /* Set by spapr_irq_init() */
> +    g_assert(sics->nr_servers);
> +
>      icsc->parent_realize(dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -312,7 +316,7 @@ static void xics_spapr_dt(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>                            void *fdt, uint32_t phandle)
>  {
>      uint32_t interrupt_server_ranges_prop[] = {
> -        0, cpu_to_be32(nr_servers),
> +        0, cpu_to_be32(ICS_SPAPR(intc)->nr_servers),
>      };
>      int node;
>  
> @@ -333,7 +337,7 @@ static void xics_spapr_dt(SpaprInterruptController *intc, 
> uint32_t nr_servers,
>  static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>                                         PowerPCCPU *cpu, Error **errp)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>      Object *obj;
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>  
> @@ -364,7 +368,7 @@ static void 
> xics_spapr_cpu_intc_destroy(SpaprInterruptController *intc,
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>  
>      assert(ics);
>      assert(ics_valid_irq(ics, irq));
> @@ -380,7 +384,7 @@ static int xics_spapr_claim_irq(SpaprInterruptController 
> *intc, int irq,
>  
>  static void xics_spapr_free_irq(SpaprInterruptController *intc, int irq)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>      uint32_t srcno = irq - ics->offset;
>  
>      assert(ics_valid_irq(ics, irq));
> @@ -390,7 +394,7 @@ static void xics_spapr_free_irq(SpaprInterruptController 
> *intc, int irq)
>  
>  static void xics_spapr_set_irq(SpaprInterruptController *intc, int irq, int 
> val)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>      uint32_t srcno = irq - ics->offset;
>  
>      ics_set_irq(ics, srcno, val);
> @@ -398,7 +402,7 @@ static void xics_spapr_set_irq(SpaprInterruptController 
> *intc, int irq, int val)
>  
>  static void xics_spapr_print_info(SpaprInterruptController *intc, Monitor 
> *mon)
>  {
> -    ICSState *ics = ICS_SPAPR(intc);
> +    ICSState *ics = ICS(intc);
>      CPUState *cs;
>  
>      CPU_FOREACH(cs) {
> @@ -426,7 +430,8 @@ static int xics_spapr_activate(SpaprInterruptController 
> *intc,
>                                 uint32_t nr_servers, Error **errp)
>  {
>      if (kvm_enabled()) {
> -        return spapr_irq_init_kvm(xics_kvm_connect, intc, nr_servers, errp);
> +        return spapr_irq_init_kvm(xics_kvm_connect, intc,
> +                                  ICS_SPAPR(intc)->nr_servers, errp);
>      }
>      return 0;
>  }
> @@ -438,6 +443,11 @@ static void 
> xics_spapr_deactivate(SpaprInterruptController *intc)
>      }
>  }
>  
> +static Property ics_spapr_properties[] = {
> +    DEFINE_PROP_UINT32("nr-servers", IcsSpaprState, nr_servers, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ics_spapr_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -446,6 +456,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void 
> *data)
>  
>      device_class_set_parent_realize(dc, ics_spapr_realize,
>                                      &isc->parent_realize);
> +    device_class_set_props(dc, ics_spapr_properties);
>      sicc->activate = xics_spapr_activate;
>      sicc->deactivate = xics_spapr_deactivate;
>      sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> @@ -462,6 +473,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void 
> *data)
>  static const TypeInfo ics_spapr_info = {
>      .name = TYPE_ICS_SPAPR,
>      .parent = TYPE_ICS,
> +    .instance_size = sizeof(IcsSpaprState),
>      .class_init = ics_spapr_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_SPAPR_INTC },
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12a012d9dd09..21de0456446b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4218,15 +4218,16 @@ static void spapr_phb_placement(SpaprMachineState 
> *spapr, uint32_t index,
>  static ICSState *spapr_ics_get(XICSFabric *dev, int irq)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(dev);
> +    ICSState *ics = ICS(spapr->ics);
>  
> -    return ics_valid_irq(spapr->ics, irq) ? spapr->ics : NULL;
> +    return ics_valid_irq(ics, irq) ? ics : NULL;
>  }
>  
>  static void spapr_ics_resend(XICSFabric *dev)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(dev);
>  
> -    ics_resend(spapr->ics);
> +    ics_resend(ICS(spapr->ics));
>  }
>  
>  static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 2dacbecfd5fd..be6f4041e433 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -316,6 +316,9 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
> **errp)
>          object_property_set_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                   &error_abort);
>          object_property_set_int(obj, "nr-irqs", smc->nr_xirqs, &error_abort);
> +        object_property_set_uint(obj, "nr-servers",
> +                                 spapr_max_server_number(spapr),
> +                                 &error_abort);
>          if (!qdev_realize(DEVICE(obj), NULL, errp)) {
>              return;
>          }
> @@ -426,7 +429,7 @@ qemu_irq spapr_qirq(SpaprMachineState *spapr, int irq)
>      assert(irq < (smc->nr_xirqs + SPAPR_XIRQ_BASE));
>  
>      if (spapr->ics) {
> -        assert(ics_valid_irq(spapr->ics, irq));
> +        assert(ics_valid_irq(ICS(spapr->ics), irq));
>      }
>      if (spapr->xive) {
>          assert(irq < spapr->xive->nr_irqs);
> @@ -556,7 +559,7 @@ static int ics_find_free_block(ICSState *ics, int num, 
> int alignnum)
>  
>  int spapr_irq_find(SpaprMachineState *spapr, int num, bool align, Error 
> **errp)
>  {
> -    ICSState *ics = spapr->ics;
> +    ICSState *ics = ICS(spapr->ics);
>      int first = -1;
>  
>      assert(ics);
>