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

[Cédric Le Goater]

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.
 
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);
     }
 }