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 <c...@kaod.org> >>>> --- >>>> 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); } }