| Message ID | 1528295806-90593-4-git-send-email-babu.moger@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v12,1/4] i386: Add support for CPUID_8000_001E for AMD | expand |
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 <babu.moger@amd.com> 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. On the other hand, I believe you expect: * "-machine pc -cpu EPYC -smp 8" to automatically enable topoext. * "-machine pc -cpu Opteron_G1 -smp 8" to not enable topoext. * What about "-machine -cpu Opteron_G1 -smp 8,threads=2"? 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. 1) "+topoext" explicitly configured (any machine-type): * Must validate topology and refuse to start if unsupported. 2) Older machine-types: * Must never enable topoext automatically, even if using "EPYC" or "threads=2" 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. * Should this enable topoext automatically even if 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? Is the above description accurate? Do you agree with these requirements? 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.
Hi Eduardo, Sorry for the late response. Got pulled into something else. > -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Wednesday, June 6, 2018 5:40 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > 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 <babu.moger@amd.com> > > 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
On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote: > Hi Eduardo, > Sorry for the late response. Got pulled into something else. > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Wednesday, June 6, 2018 5:40 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > 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 <babu.moger@amd.com> > > > > 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. This doesn't sound right. The code in this series will error out of TOPOEXT is enabled and you have more than 64 VCPUs. But I just noticed we have a bug introduced by: commit f548222c24342ca74689de7794f9006b43f86a54 Author: Xiao Guangrong <xiaoguangrong@tencent.com> Date: Thu May 3 16:06:11 2018 +0800 migration: introduce decompress-error-check QEMU 3.0 enables strict check for compression & decompression to make the migration more robust, that depends on the source to fix the internal design which triggers the unexpected error conditions To make it work for migrating old version QEMU to 2.13 QEMU, we introduce this parameter to disable the error check on the destination which is the default behavior of the machine type which is older than 2.13, alternately, the strict check can be enabled explicitly as followings: -M pc-q35-2.11 -global migration.decompress-error-check=true Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> Reviewed-by: Juan Quintela <quintela@redhat.com> Signed-off-by: Juan Quintela <quintela@redhat.com> This commits added PC_COMPAT_2_12 to the 3.0 machine-types. Because of this bug, TOPOEXT is being unconditionally disabled on all machine-types, unless I apply the fix below: diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 3d81136065..b4c5b03274 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -430,7 +430,6 @@ static void pc_i440fx_3_0_machine_options(MachineClass *m) pc_i440fx_machine_options(m); m->alias = "pc"; m->is_default = 1; - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); } DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL, diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index b60cbb9266..83d6d75efa 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -312,7 +312,6 @@ static void pc_q35_3_0_machine_options(MachineClass *m) { pc_q35_machine_options(m); m->alias = "q35"; - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); } DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL, > > > > > 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. OK, makes sense to me. > > > > > > 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) Right, so you agree that in this case we must _not_ error out if topology is unsupported, correct? Otherwise we will break this existing use case: "-machine pc -cpu EPYC -smp 64". > > 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. Thanks for confirming the requirements above. But it doesn't seem to be possible represent these requirements with just one bit. Otherwise you can't differentiate explicit "+topoext" (1 above) from topoext being implicitly enabled by "-cpu EPYC" (3 above). Another problem is query-cpu-model-expansion QMP command: this patch makes "topoext" appear on the output of "query-cpu-model-expansion model=EPYC", meaning that management software will assume everybody using the "EPYC" CPU model will require +topoext. A separate "auto-topoext" property would avoid this issue. (Yeah, this is tricky. I want to eventually encode these subtle rules in automated test cases, so these issues could be detected by software instead of code inspection.)
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, June 8, 2018 2:23 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Juan > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote: > > Hi Eduardo, > > Sorry for the late response. Got pulled into something else. > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Wednesday, June 6, 2018 5:40 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > > 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 <babu.moger@amd.com> > > > > > > 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. > > This doesn't sound right. The code in this series will error out > of TOPOEXT is enabled and you have more than 64 VCPUs. > > But I just noticed we have a bug introduced by: Oh.. Ok.. Let me retry again with the new patch. > > commit f548222c24342ca74689de7794f9006b43f86a54 > Author: Xiao Guangrong <xiaoguangrong@tencent.com> > Date: Thu May 3 16:06:11 2018 +0800 > > migration: introduce decompress-error-check > > QEMU 3.0 enables strict check for compression & decompression to > make the migration more robust, that depends on the source to fix > the internal design which triggers the unexpected error conditions > > To make it work for migrating old version QEMU to 2.13 QEMU, we > introduce this parameter to disable the error check on the > destination which is the default behavior of the machine type > which is older than 2.13, alternately, the strict check can be > enabled explicitly as followings: > -M pc-q35-2.11 -global migration.decompress-error-check=true > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types. > Because of this bug, TOPOEXT is being unconditionally disabled on > all machine-types, unless I apply the fix below: > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 3d81136065..b4c5b03274 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -430,7 +430,6 @@ static void > pc_i440fx_3_0_machine_options(MachineClass *m) > pc_i440fx_machine_options(m); > m->alias = "pc"; > m->is_default = 1; > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > } > > DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL, > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index b60cbb9266..83d6d75efa 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -312,7 +312,6 @@ static void > pc_q35_3_0_machine_options(MachineClass *m) > { > pc_q35_machine_options(m); > m->alias = "q35"; > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > } > > DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL, > > > > > > > > > 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. > > OK, makes sense to me. > > > > > > > > > > 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) > > Right, so you agree that in this case we must _not_ error out if > topology is unsupported, correct? Otherwise we will break this > existing use case: > "-machine pc -cpu EPYC -smp 64". Ok. I will test this with new fix patch. > > > > > 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. > > Thanks for confirming the requirements above. > > But it doesn't seem to be possible represent these requirements > with just one bit. Otherwise you can't differentiate explicit > "+topoext" (1 above) from topoext being implicitly enabled by > "-cpu EPYC" (3 above). > > Another problem is query-cpu-model-expansion QMP command: this > patch makes "topoext" appear on the output of > "query-cpu-model-expansion model=EPYC", meaning that management > software will assume everybody using the "EPYC" CPU model will > require +topoext. A separate "auto-topoext" property would avoid > this issue. Sure. Will work on it. Yes, I know all these combinations make it very tricky. > > (Yeah, this is tricky. I want to eventually encode these subtle > rules in automated test cases, so these issues could be detected > by software instead of code inspection.) > > -- > Eduardo
On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote: > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Friday, June 8, 2018 2:23 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; Juan > > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC > > CPU > > > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote: > > > Hi Eduardo, > > > Sorry for the late response. Got pulled into something else. > > > > > > > -----Original Message----- > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Sent: Wednesday, June 6, 2018 5:40 PM > > > > To: Moger, Babu <Babu.Moger@amd.com> > > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > > pbonzini@redhat.com; > > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > > > 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 <babu.moger@amd.com> > > > > > > > > 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. > > > > This doesn't sound right. The code in this series will error out > > of TOPOEXT is enabled and you have more than 64 VCPUs. > > > > But I just noticed we have a bug introduced by: > > Oh.. Ok.. Let me retry again with the new patch. > > > > > commit f548222c24342ca74689de7794f9006b43f86a54 > > Author: Xiao Guangrong <xiaoguangrong@tencent.com> > > Date: Thu May 3 16:06:11 2018 +0800 > > > > migration: introduce decompress-error-check > > > > QEMU 3.0 enables strict check for compression & decompression to > > make the migration more robust, that depends on the source to fix > > the internal design which triggers the unexpected error conditions > > > > To make it work for migrating old version QEMU to 2.13 QEMU, we > > introduce this parameter to disable the error check on the > > destination which is the default behavior of the machine type > > which is older than 2.13, alternately, the strict check can be > > enabled explicitly as followings: > > -M pc-q35-2.11 -global migration.decompress-error-check=true > > > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types. > > Because of this bug, TOPOEXT is being unconditionally disabled on > > all machine-types, unless I apply the fix below: > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > index 3d81136065..b4c5b03274 100644 > > --- a/hw/i386/pc_piix.c > > +++ b/hw/i386/pc_piix.c > > @@ -430,7 +430,6 @@ static void > > pc_i440fx_3_0_machine_options(MachineClass *m) > > pc_i440fx_machine_options(m); > > m->alias = "pc"; > > m->is_default = 1; > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > } > > > > DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL, > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > index b60cbb9266..83d6d75efa 100644 > > --- a/hw/i386/pc_q35.c > > +++ b/hw/i386/pc_q35.c > > @@ -312,7 +312,6 @@ static void > > pc_q35_3_0_machine_options(MachineClass *m) > > { > > pc_q35_machine_options(m); > > m->alias = "q35"; > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > } > > > > DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL, > > > > > > > > > > > > > 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. > > > > OK, makes sense to me. > > > > > > > > > > > > > > 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) > > > > Right, so you agree that in this case we must _not_ error out if > > topology is unsupported, correct? Otherwise we will break this > > existing use case: > > "-machine pc -cpu EPYC -smp 64". > > Ok. I will test this with new fix patch. > > > > > > > > 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. > > > > Thanks for confirming the requirements above. > > > > But it doesn't seem to be possible represent these requirements > > with just one bit. Otherwise you can't differentiate explicit > > "+topoext" (1 above) from topoext being implicitly enabled by > > "-cpu EPYC" (3 above). > > > > Another problem is query-cpu-model-expansion QMP command: this > > patch makes "topoext" appear on the output of > > "query-cpu-model-expansion model=EPYC", meaning that management > > software will assume everybody using the "EPYC" CPU model will > > require +topoext. A separate "auto-topoext" property would avoid > > this issue. > > Sure. Will work on it. Yes, I know all these combinations make it very tricky. I will rewrite my proposal, to make sure we are on the same page: 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: [CHANGED in v2] * Will set auto-topoext=off (can be done on compat_props) * No need to set topoext=off on compat_props (because no CPU model will have topoext=on, see #3 below) 3) "EPYC" CPU model (on new machine-types): [CHANGED IN v2] * Will set auto-topoext=on (using a new field on X86CPUDefinition) * Will NOT set TOPOEXT on CPU model table (or it would break query-cpu-model-expansion) 4) Other AMD CPU models (on all machine-types): [CHANGED IN v2] * auto-topoext=off (the default) * Will keep TOPOEXT disabled on env->features (the default) x86_cpu_realizefn: if {cpu->cpudef && cpu->cpudef->auto_topoext && TOPOEXT not in env->user_features && supported_topology) { set TOPOEXT in env->features } if (TOPOEXT in env->features && !supported_topology) error; } I think this logic will fulfill all the requirements above.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, June 8, 2018 2:50 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > kash@tripleback.net; mtosatti@redhat.com; xiaoguangrong@tencent.com; > qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>; > pbonzini@redhat.com; rth@twiddle.net > Subject: Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on > AMD EPYC CPU > > On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote: > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Friday, June 8, 2018 2:23 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; > Juan > > > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com > > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC > > > CPU > > > > > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote: > > > > Hi Eduardo, > > > > Sorry for the late response. Got pulled into something else. > > > > > > > > > -----Original Message----- > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > > Sent: Wednesday, June 6, 2018 5:40 PM > > > > > To: Moger, Babu <Babu.Moger@amd.com> > > > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > > > pbonzini@redhat.com; > > > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > > > > 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 <babu.moger@amd.com> > > > > > > > > > > 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. > > > > > > This doesn't sound right. The code in this series will error out > > > of TOPOEXT is enabled and you have more than 64 VCPUs. > > > > > > But I just noticed we have a bug introduced by: > > > > Oh.. Ok.. Let me retry again with the new patch. > > > > > > > > commit f548222c24342ca74689de7794f9006b43f86a54 > > > Author: Xiao Guangrong <xiaoguangrong@tencent.com> > > > Date: Thu May 3 16:06:11 2018 +0800 > > > > > > migration: introduce decompress-error-check > > > > > > QEMU 3.0 enables strict check for compression & decompression to > > > make the migration more robust, that depends on the source to fix > > > the internal design which triggers the unexpected error conditions > > > > > > To make it work for migrating old version QEMU to 2.13 QEMU, we > > > introduce this parameter to disable the error check on the > > > destination which is the default behavior of the machine type > > > which is older than 2.13, alternately, the strict check can be > > > enabled explicitly as followings: > > > -M pc-q35-2.11 -global migration.decompress-error-check=true > > > > > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types. > > > Because of this bug, TOPOEXT is being unconditionally disabled on > > > all machine-types, unless I apply the fix below: > > > > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > index 3d81136065..b4c5b03274 100644 > > > --- a/hw/i386/pc_piix.c > > > +++ b/hw/i386/pc_piix.c > > > @@ -430,7 +430,6 @@ static void > > > pc_i440fx_3_0_machine_options(MachineClass *m) > > > pc_i440fx_machine_options(m); > > > m->alias = "pc"; > > > m->is_default = 1; > > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > > > > DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL, > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > index b60cbb9266..83d6d75efa 100644 > > > --- a/hw/i386/pc_q35.c > > > +++ b/hw/i386/pc_q35.c > > > @@ -312,7 +312,6 @@ static void > > > pc_q35_3_0_machine_options(MachineClass *m) > > > { > > > pc_q35_machine_options(m); > > > m->alias = "q35"; > > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > > > > DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL, > > > > > > > > > > > > > > > > > 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. > > > > > > OK, makes sense to me. > > > > > > > > > > > > > > > > > > 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) > > > > > > Right, so you agree that in this case we must _not_ error out if > > > topology is unsupported, correct? Otherwise we will break this > > > existing use case: > > > "-machine pc -cpu EPYC -smp 64". > > > > Ok. I will test this with new fix patch. > > > > > > > > > > > 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. > > > > > > Thanks for confirming the requirements above. > > > > > > But it doesn't seem to be possible represent these requirements > > > with just one bit. Otherwise you can't differentiate explicit > > > "+topoext" (1 above) from topoext being implicitly enabled by > > > "-cpu EPYC" (3 above). > > > > > > Another problem is query-cpu-model-expansion QMP command: this > > > patch makes "topoext" appear on the output of > > > "query-cpu-model-expansion model=EPYC", meaning that management > > > software will assume everybody using the "EPYC" CPU model will > > > require +topoext. A separate "auto-topoext" property would avoid > > > this issue. > > > > Sure. Will work on it. Yes, I know all these combinations make it very > tricky. > > I will rewrite my proposal, to make sure we are on the same page: Looks good. Thanks > > 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: > [CHANGED in v2] > * Will set auto-topoext=off (can be done on compat_props) > * No need to set topoext=off on compat_props (because no CPU model will > have > topoext=on, see #3 below) > > 3) "EPYC" CPU model (on new machine-types): > [CHANGED IN v2] > * Will set auto-topoext=on (using a new field on X86CPUDefinition) > * Will NOT set TOPOEXT on CPU model table (or it would break > query-cpu-model-expansion) > > 4) Other AMD CPU models (on all machine-types): > [CHANGED IN v2] > * auto-topoext=off (the default) > * Will keep TOPOEXT disabled on env->features (the default) > > > x86_cpu_realizefn: > > if {cpu->cpudef && cpu->cpudef->auto_topoext && > TOPOEXT not in env->user_features && > supported_topology) { > set TOPOEXT in env->features > } > > if (TOPOEXT in env->features && !supported_topology) > error; > } > > I think this logic will fulfill all the requirements above. Yes. I think so. Will test it and confirm. Thanks > > -- > Eduardo
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, June 8, 2018 2:50 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > kash@tripleback.net; mtosatti@redhat.com; xiaoguangrong@tencent.com; > qemu-devel@nongnu.org; Juan Quintela <quintela@redhat.com>; > pbonzini@redhat.com; rth@twiddle.net > Subject: Re: [Qemu-devel] [PATCH v12 3/4] i386: Enable TOPOEXT feature on > AMD EPYC CPU > > On Fri, Jun 08, 2018 at 07:36:05PM +0000, Moger, Babu wrote: > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Friday, June 8, 2018 2:23 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com; > Juan > > > Quintela <quintela@redhat.com>; xiaoguangrong@tencent.com > > > Subject: Re: [PATCH v12 3/4] i386: Enable TOPOEXT feature on AMD EPYC > > > CPU > > > > > > On Fri, Jun 08, 2018 at 06:40:16PM +0000, Moger, Babu wrote: > > > > Hi Eduardo, > > > > Sorry for the late response. Got pulled into something else. > > > > > > > > > -----Original Message----- > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > > Sent: Wednesday, June 6, 2018 5:40 PM > > > > > To: Moger, Babu <Babu.Moger@amd.com> > > > > > Cc: mst@redhat.com; marcel.apfelbaum@gmail.com; > > > pbonzini@redhat.com; > > > > > rth@twiddle.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > > > kvm@vger.kernel.org; kash@tripleback.net; geoff@hostfission.com > > > > > 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 <babu.moger@amd.com> > > > > > > > > > > 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. > > > > > > This doesn't sound right. The code in this series will error out > > > of TOPOEXT is enabled and you have more than 64 VCPUs. > > > > > > But I just noticed we have a bug introduced by: > > > > Oh.. Ok.. Let me retry again with the new patch. > > > > > > > > commit f548222c24342ca74689de7794f9006b43f86a54 > > > Author: Xiao Guangrong <xiaoguangrong@tencent.com> > > > Date: Thu May 3 16:06:11 2018 +0800 > > > > > > migration: introduce decompress-error-check > > > > > > QEMU 3.0 enables strict check for compression & decompression to > > > make the migration more robust, that depends on the source to fix > > > the internal design which triggers the unexpected error conditions > > > > > > To make it work for migrating old version QEMU to 2.13 QEMU, we > > > introduce this parameter to disable the error check on the > > > destination which is the default behavior of the machine type > > > which is older than 2.13, alternately, the strict check can be > > > enabled explicitly as followings: > > > -M pc-q35-2.11 -global migration.decompress-error-check=true > > > > > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com> > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > > > This commits added PC_COMPAT_2_12 to the 3.0 machine-types. > > > Because of this bug, TOPOEXT is being unconditionally disabled on > > > all machine-types, unless I apply the fix below: > > > > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > index 3d81136065..b4c5b03274 100644 > > > --- a/hw/i386/pc_piix.c > > > +++ b/hw/i386/pc_piix.c > > > @@ -430,7 +430,6 @@ static void > > > pc_i440fx_3_0_machine_options(MachineClass *m) > > > pc_i440fx_machine_options(m); > > > m->alias = "pc"; > > > m->is_default = 1; > > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > > > > DEFINE_I440FX_MACHINE(v3_0, "pc-i440fx-3.0", NULL, > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > > > index b60cbb9266..83d6d75efa 100644 > > > --- a/hw/i386/pc_q35.c > > > +++ b/hw/i386/pc_q35.c > > > @@ -312,7 +312,6 @@ static void > > > pc_q35_3_0_machine_options(MachineClass *m) > > > { > > > pc_q35_machine_options(m); > > > m->alias = "q35"; > > > - SET_MACHINE_COMPAT(m, PC_COMPAT_2_12); > > > } > > > > > > DEFINE_Q35_MACHINE(v3_0, "pc-q35-3.0", NULL, > > > > > > > > > > > > > > > > > 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. > > > > > > OK, makes sense to me. > > > > > > > > > > > > > > > > > > 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) > > > > > > Right, so you agree that in this case we must _not_ error out if > > > topology is unsupported, correct? Otherwise we will break this > > > existing use case: > > > "-machine pc -cpu EPYC -smp 64". > > > > Ok. I will test this with new fix patch. > > > > > > > > > > > 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. > > > > > > Thanks for confirming the requirements above. > > > > > > But it doesn't seem to be possible represent these requirements > > > with just one bit. Otherwise you can't differentiate explicit > > > "+topoext" (1 above) from topoext being implicitly enabled by > > > "-cpu EPYC" (3 above). > > > > > > Another problem is query-cpu-model-expansion QMP command: this > > > patch makes "topoext" appear on the output of > > > "query-cpu-model-expansion model=EPYC", meaning that management > > > software will assume everybody using the "EPYC" CPU model will > > > require +topoext. A separate "auto-topoext" property would avoid > > > this issue. > > > > Sure. Will work on it. Yes, I know all these combinations make it very > tricky. > > I will rewrite my proposal, to make sure we are on the same page: > > 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: > [CHANGED in v2] > * Will set auto-topoext=off (can be done on compat_props) > * No need to set topoext=off on compat_props (because no CPU model will > have > topoext=on, see #3 below) > > 3) "EPYC" CPU model (on new machine-types): > [CHANGED IN v2] > * Will set auto-topoext=on (using a new field on X86CPUDefinition) > * Will NOT set TOPOEXT on CPU model table (or it would break > query-cpu-model-expansion) > > 4) Other AMD CPU models (on all machine-types): > [CHANGED IN v2] > * auto-topoext=off (the default) > * Will keep TOPOEXT disabled on env->features (the default) > > > x86_cpu_realizefn: > > if {cpu->cpudef && cpu->cpudef->auto_topoext && > TOPOEXT not in env->user_features && > supported_topology) { > set TOPOEXT in env->features > } > > if (TOPOEXT in env->features && !supported_topology) > error; > } > > I think this logic will fulfill all the requirements above. Hi Eduardo , I have sent the v13 version. I am still testing it. Wanted to get feedback if I missed anything. Thanks > > -- > Eduardo
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 04d1f8c..2630a91 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -303,6 +303,10 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); .driver = TYPE_X86_CPU,\ .property = "legacy-cache",\ .value = "on",\ + },{\ + .driver = "EPYC-" TYPE_X86_CPU,\ + .property = "topoext",\ + .value = "off",\ }, #define PC_COMPAT_2_11 \ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index fc5c66d..40bf5cb 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -2568,7 +2568,8 @@ static X86CPUDefinition builtin_x86_defs[] = { .features[FEAT_8000_0001_ECX] = CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | - CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM, + CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM | + CPUID_EXT3_TOPOEXT, .features[FEAT_7_0_EBX] = CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED | @@ -2613,7 +2614,8 @@ static X86CPUDefinition builtin_x86_defs[] = { .features[FEAT_8000_0001_ECX] = CPUID_EXT3_OSVW | CPUID_EXT3_3DNOWPREFETCH | CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM | - CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM, + CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM | + CPUID_EXT3_TOPOEXT, .features[FEAT_8000_0008_EBX] = CPUID_8000_0008_EBX_IBPB, .features[FEAT_7_0_EBX] = @@ -4681,6 +4683,11 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error **errp) x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A); } + /* TOPOEXT feature requires 0x8000001E */ + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) { + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); + } + /* SEV requires CPUID[0x8000001F] */ if (sev_enabled()) { x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001F);
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 <babu.moger@amd.com> --- include/hw/i386/pc.h | 4 ++++ target/i386/cpu.c | 11 +++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-)