Re: [Qemu-devel] [PATCH v7 18/19] spapr: add a 'pseries-4.0-xive' machine type

[David Gibson]

On Tue, Dec 11, 2018 at 05:44:26PM +0100, Cédric Le Goater wrote:
> On 12/11/18 11:42 AM, Cédric Le Goater wrote:
> > On 12/11/18 3:06 AM, David Gibson wrote:
> >> On Mon, Dec 10, 2018 at 11:17:33PM +0100, Cédric Le Goater wrote:
> >>> On 12/9/18 8:46 PM, Cédric Le Goater wrote:
> >>>> This pseries machine makes use of a new sPAPR IRQ backend supporting
> >>>> the XIVE interrupt mode.
> >>>>
> >>>> The guest OS is required to have support for the XIVE exploitation
> >>>> mode of the POWER9 interrupt controller.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <address@hidden>
> >>>> ---
> >>>>  hw/ppc/spapr.c | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index 4012ebd794a4..3cc134a0b673 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -3985,6 +3985,21 @@ static void 
> >>>> spapr_machine_4_0_class_options(MachineClass *mc)
> >>>>  
> >>>>  DEFINE_SPAPR_MACHINE(4_0, "4.0", true);
> >>>>  
> >>>> +static void spapr_machine_4_0_xive_instance_options(MachineState 
> >>>> *machine)
> >>>> +{
> >>>> +    spapr_machine_4_0_instance_options(machine);
> >>>> +}
> >>>> +
> >>>> +static void spapr_machine_4_0_xive_class_options(MachineClass *mc)
> >>>> +{
> >>>> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> >>>> +
> >>>> +    spapr_machine_4_0_class_options(mc);> +    smc->irq = 
> >>>> &spapr_irq_xive;
> >>>
> >>> I have been adding checks on the CPU model to export the XIVE capability 
> >>> only on POWER9 processors but it breaks some of the tests.
> >>>
> >>> I was wondering if we could add a default POWER9 CPU to the -xive machine 
> >>> : 
> >>>
> >>>   + mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> >>>
> >>> and if we could change tests/cpu-plug-test.c with :
> >>>
> >>>   @@ -198,8 +198,13 @@ static void add_pseries_test_case(const
> >>>        }
> >>>        data = g_new(PlugTestData, 1);
> >>>        data->machine = g_strdup(mname);
> >>>   -    data->cpu_model = "power8_v2.0";
> >>>   -    data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> >>>   +    if (g_str_has_suffix(mname, "xive")) {
> >>>   +        data->cpu_model = "power9_v2.0";
> >>>   +        data->device_model = g_strdup("power9_v2.0-spapr-cpu-core");
> >>>   +    } else {
> >>>   +        data->cpu_model = "power8_v2.0";
> >>>   +        data->device_model = g_strdup("power8_v2.0-spapr-cpu-core");
> >>>   +    }
> >>>        data->sockets = 2;
> >>>        data->cores = 3;
> >>>        data->threads = 1;
> >>>
> >>> or if there is a better way ?
> >>
> >> So, I'd actually prefer a machine option, rather than wholly separate
> >> machine types to select xics/xive/dual.  Machine types was fine while
> >> prototyping this, but I don't think we want to actually merge new
> >> machine types for it.
> > 
> > I agree. 
> > 
> >> So, instead I think we want a machine option which can be set to
> >> xics/xive/dual, with xics being the default for earlier machine types
> >> and dual the default for 4.0 onwards.
> > 
> > I will revive an old patch doing just that. 
> > 
> > The question now is how to link the sPAPRMachineState instance to 
> > the selected sPAPR IRQ backend. 
> 
> Would something like below be acceptable ? If so I will change the 
> remaining patches to use 'spapr->irq' and not 'smc->irq' anymore.

Yeah, I think that looks ok.

>  
> Thanks,
> 
> C.
> 
> Index: qemu-xive.git/include/hw/ppc/spapr.h
> ===================================================================
> --- qemu-xive.git.orig/include/hw/ppc/spapr.h
> +++ qemu-xive.git/include/hw/ppc/spapr.h
> @@ -177,6 +177,7 @@ struct sPAPRMachineState {
>      int32_t irq_map_nr;
>      unsigned long *irq_map;
>      sPAPRXive  *xive;
> +    sPAPRIrq *irq;
>  
>      bool cmd_line_caps[SPAPR_CAP_NUM];
>      sPAPRCapabilities def, eff, mig;
> Index: qemu-xive.git/hw/ppc/spapr.c
> ===================================================================
> --- qemu-xive.git.orig/hw/ppc/spapr.c
> +++ qemu-xive.git/hw/ppc/spapr.c
> @@ -1302,7 +1301,7 @@ static void *spapr_build_fdt(sPAPRMachin
>      }
>  
>      QLIST_FOREACH(phb, &spapr->phbs, list) {
> -        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt, 
> smc->irq->nr_msis);
> +        ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt, 
> spapr->irq->nr_msis);
>          if (ret < 0) {
>              error_report("couldn't setup PCI devices in fdt");
>              exit(1);
> @@ -3056,9 +3055,38 @@ static void spapr_set_vsmt(Object *obj,
>      visit_type_uint32(v, name, (uint32_t *)opaque, errp);
>  }
>  
> +static char *spapr_get_irq(Object *obj, Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +    if (spapr->irq == &spapr_irq_xics_legacy) {
> +        return g_strdup("legacy");
> +    } else if (spapr->irq == &spapr_irq_xics) {
> +        return g_strdup("xics");
> +    } else if (spapr->irq == &spapr_irq_xive) {
> +        return g_strdup("xive");
> +    }
> +    g_assert_not_reached();
> +}
> +
> +static void spapr_set_irq(Object *obj, const char *value, Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> +    /* We don't want to set the legacy IRQ backend */
> +    if (strcmp(value, "xics") == 0) {
> +        spapr->irq = &spapr_irq_xics;
> +    } else if (strcmp(value, "xive") == 0) {
> +        spapr->irq = &spapr_irq_xive;
> +    } else {
> +        error_setg(errp, "Bad value for \"irq\" property");
> +    }
> +}
> +
>  static void spapr_instance_init(Object *obj)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>  
>      spapr->htab_fd = -1;
>      spapr->use_hotplug_event_source = true;
> @@ -3092,6 +3120,13 @@ static void spapr_instance_init(Object *
>                                      " the host's SMT mode", &error_abort);
>      object_property_add_bool(obj, "vfio-no-msix-emulation",
>                               spapr_get_msix_emulation, NULL, NULL);
> +
> +    /* Get the default from the machine class */
> +    spapr->irq = smc->irq;
> +    object_property_add_str(obj, "irq", spapr_get_irq, spapr_set_irq, NULL);
> +    object_property_set_description(obj, "irq",
> +                 "Specifies the interrupt controller mode (xics, xive)",
> +                 NULL);
>  }
>  
>  static void spapr_machine_finalizefn(Object *obj)
> @@ -3814,9 +3849,8 @@ static void spapr_pic_print_info(Interru
>                                   Monitor *mon)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>  
> -    smc->irq->print_info(spapr, mon);
> +    spapr->irq->print_info(spapr, mon);
>  }
>  
>  int spapr_get_vcpu_id(PowerPCCPU *cpu)
> Index: qemu-xive.git/hw/ppc/spapr_irq.c
> ===================================================================
> --- qemu-xive.git.orig/hw/ppc/spapr_irq.c
> +++ qemu-xive.git/hw/ppc/spapr_irq.c
> @@ -94,8 +94,7 @@ error:
>  static void spapr_irq_init_xics(sPAPRMachineState *spapr, Error **errp)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -    int nr_irqs = smc->irq->nr_irqs;
> +    int nr_irqs = spapr->irq->nr_irqs;
>      Error *local_err = NULL;
>  
>      if (kvm_enabled()) {
> @@ -234,7 +233,6 @@ sPAPRIrq spapr_irq_xics = {
>  static void spapr_irq_init_xive(sPAPRMachineState *spapr, Error **errp)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      uint32_t nr_servers = spapr_max_server_number(spapr);
>      DeviceState *dev;
>      int i;
> @@ -248,7 +246,7 @@ static void spapr_irq_init_xive(sPAPRMac
>      }
>  
>      dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
> -    qdev_prop_set_uint32(dev, "nr-irqs", smc->irq->nr_irqs);
> +    qdev_prop_set_uint32(dev, "nr-irqs", spapr->irq->nr_irqs);
>      /*
>       * 8 XIVE END structures per CPU. One for each available priority
>       */
> @@ -353,50 +351,38 @@ sPAPRIrq spapr_irq_xive = {
>   */
>  void spapr_irq_init(sPAPRMachineState *spapr, Error **errp)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
>      /* Initialize the MSI IRQ allocator. */
>      if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> -        spapr_irq_msi_init(spapr, smc->irq->nr_msis);
> +        spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>      }
>  
> -    smc->irq->init(spapr, errp);
> +    spapr->irq->init(spapr, errp);
>  }
>  
>  int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error 
> **errp)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
> -    return smc->irq->claim(spapr, irq, lsi, errp);
> +    return spapr->irq->claim(spapr, irq, lsi, errp);
>  }
>  
>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
> -    smc->irq->free(spapr, irq, num);
> +    spapr->irq->free(spapr, irq, num);
>  }
>  
>  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
> -    return smc->irq->qirq(spapr, irq);
> +    return spapr->irq->qirq(spapr, irq);
>  }
>  
>  int spapr_irq_post_load(sPAPRMachineState *spapr, int version_id)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
> -    return smc->irq->post_load(spapr, version_id);
> +    return spapr->irq->post_load(spapr, version_id);
>  }
>  
>  void spapr_irq_reset(sPAPRMachineState *spapr, Error **errp)
>  {
> -    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> -
> -    if (smc->irq->reset) {
> -        smc->irq->reset(spapr, errp);
> +    if (spapr->irq->reset) {
> +        spapr->irq->reset(spapr, errp);
>      }
>  }
>  
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature