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

[Cédric Le Goater]

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. So we will
need an helper returning the appropriate backend depending on the machine 
option and 'smc->irq' should disappear.

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

OK.

C.