Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC CPU

[Moger, Babu]

Hi Eduardo, 
Sorry for the late response. Got pulled into something else.

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Wednesday, June 6, 2018 5:40 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC
> CPU
> 
> On Wed, Jun 06, 2018 at 10:36:45AM -0400, Babu Moger wrote:
> > Enable TOPOEXT feature on EPYC CPU. This is required to support
> > hyperthreading on VM guests. Also extend xlevel to 0x8000001E.
> >
> > Disable TOPOEXT feature for legacy machines.
> >
> > Signed-off-by: Babu Moger <address@hidden>
> 
> Now, I just noticed we have a problem here:
> 
> "-machine pc -cpu EPYC -smp 64" works today
> 
> This patch makes it stop working, but it shouldn't.

No. It works fine. I have tested it.

> 
> On the other hand, I believe you expect:
> * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext.
Yes. Only on new machines-types
> * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext.
Yes.
> * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"?
No. This should not enable topoext.  Topoext is not supported by Opteron_G1.
This should warn about hyperthreading and continue.
> 
> 
> We also have other requirements, I will try to enumerate all of
> them below:
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Must never enable topoext.
Yes.
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Must validate topology and refuse to start if unsupported.

Yes.

> 
> 2) Older machine-types:
> * Must never enable topoext automatically, even if using "EPYC"
>   or "threads=2"
> 
Yes.

> 3) "EPYC" CPU model (on new machine-types):
> * Should enable topoext automatically, but only if topology is
>   supported.
> * Must not error out if topology is not supported.
In new machine types we will enable topoext for "EPYC" CPU model.
Right now(old machine type) we can disable for all the CPU models. 
So, we don't need two bits(topoext and auto-topoext)

I thought we should error out if topology cannot be supported. But we can 
warn(disable topoext) and continue that is another option.

> * Should this enable topoext automatically even if threads=1?

Yes. We should enable even with threads=1.

> 
> 4) Other AMD CPU models with "threads=2" (on new machine-types):
> * We might want to make this enable topoext automatically, too.
>   What do you think?

  No. We should not enable topoext here. We should depend on CPU model table 
here.

> 
> Is the above description accurate?  Do you agree with these
> requirements?

With these requirements in mind, I will send that patches. We can start our 
discussion.
We don't need one more bits. That is my opinion. 


> 
> We're trying to use the "topoext" property to cover all cases
> above, but it looks like we need at least 2 bits to represent all
> possible cases.
> 
> 
> Maybe we can represent the cases above with two properties:
> "topoext" and "auto-topoext".  Then each case would be
> represented by:
> 
> 0) "-topoext" explicitly configured (any machine-type):
> * Will clear TOPOEXT on env->features and set TOPOEXT on
>   env->user_features
>   (already done today)
> 
> 1) "+topoext" explicitly configured (any machine-type):
> * Will set TOPOEXT on both env->user_features and env->features
>   (already done today)
> 
> 2) Older machine-types:
> * Will set auto-topoext=off (can be done on compat_props)
> * Will set topoext=off on EPYC CPU model (so TOPOEXT won't be set
>   by default on env->features) (can be done on compat_props)
> 
> 3) "EPYC" CPU model (on new machine-types):
> * Will set auto-topoext=on (can be the default for all CPU
>   models)
> * Will set TOPOEXT on env->features) (can be done on CPU model table)
> 
> 4) Other AMD CPU models with "threads=2" (on new machine-types):
> * Will set auto-topoext=on (can be the default on all CPU models)
> * Will keep TOPOEXT disabled on env->features (done on the CPU
>   model table)
> 
> 
> Then the rules would be:
> 
>   if {auto_topoext && TOPOEXT not in env->user_features) {
>       if (supported_topology) {
>           if (threads > 1)
>               set TOPOEXT in env->features
>       } else
>           unset TOPOEXT in env->features
>   }
> 
>   if (TOPOEXT in env->features && !supported_topology)
>       error;
>   }
> 
> I think this would fulfill all the requirements above.  Please
> help me confirm that.
> 
> --
> Eduardo