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 <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.
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
