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

[David Gibson]

On Tue, Dec 11, 2018 at 11:42:03AM +0100, 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. 
> 
> I don't think we can move 'smc->irq' to sPAPRMachineState.

I think you could..

> So we will
> need an helper returning the appropriate backend depending on the machine 
> option and 'smc->irq' should disappear.

..but this approach might be easier.

> 
> > We can make POWER9 the default cpu for 4.0 onwards as well, if you want.
> 
> OK.
> 
> C.
> 

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