Re: [PATCH v3 2/3] hw/i386: Add a new check to configure smp dies for EPYC

[Babu Moger]

On 8/7/20 11:52 AM, Daniel P. Berrangé wrote:
> On Fri, Aug 07, 2020 at 11:32:51AM -0500, Babu Moger wrote:
>> Adding a new check to warn the users to configure 'dies' when
>> topology is numa configured. It makes it easy to build the
>> topology for EPYC models.
> 
> This says you're adding a warning....
> 
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  hw/i386/x86.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index 67bee1bcb8..2a6ce56ef1 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -138,6 +138,13 @@ void x86_cpus_init(X86MachineState *x86ms, int 
>> default_cpu_version)
>>  
>>      /* Check for apicid encoding */
>>      if (cpu_x86_use_epyc_apic_id_encoding(ms->cpu_type)) {
>> +        if ((ms->numa_state->num_nodes > 0) &&
>> +            ms->numa_state->num_nodes != (ms->smp.sockets * 
>> x86ms->smp_dies)) {
>> +            error_setg(&error_fatal, "Numa configuration requires smp 
>> 'dies' "
>> +                       "parameter. Configure the cpu topology properly with 
>> "
>> +                       "max_cpus = sockets * dies * cores * threads");
> 
> ...but you're actually making this a fatal error, not a warning.
> 
> I'm not sure this is really OK. Wouldn't this mean that existing VMs
> deployed today, risk triggering this fatal error next time they
> are booted, or live migrated.  If it is possible someone is using
> such a config today, I don't think we can break it.

Yes. that is correct. May be we can display warning(not fatal) and assign
the smp_dies = numa_nodes / sockets. But that requires re-arrange of smp
parameters. Not sure if that is ok.

> 
> Regards,
> Daniel
>