| Message ID | 1528498581-131037-3-git-send-email-babu.moger@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Enable TOPOEXT to support hyperthreading on AMD CPU | expand |
Hi Eduardo, Planning to make couple of changes after testing and review. Let me know if I missed something. Will wait for your feedback before the next revision. > -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Babu Moger > Sent: Friday, June 8, 2018 5:56 PM > To: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage > topoext > > Introduce the auto_topoext bit to to control topoext feature. > > Also add new field auto_topoext(in X86CPUDefinition). This will > be used to enable topoext on newer CPU models where topoext can > be supported. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > include/hw/i386/pc.h | 4 ++++ > target/i386/cpu.c | 12 ++++++++++++ > target/i386/cpu.h | 5 +++++ > 3 files changed, 21 insertions(+) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 04d1f8c..cc30ec3 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 = TYPE_X86_CPU,\ > + .property = "auto-topoext",\ > + .value = "off",\ > }, We don't need this. We are already setting this false in x86_cpu_properties. But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. That will be a separate patch. Correct? > > #define PC_COMPAT_2_11 \ > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 86fb1a4..d3411ed 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition { > FeatureWordArray features; > const char *model_id; > CPUCaches *cache_info; > + /* Set it if topoext can be enabled in CPU models */ > + int auto_topoext; > }; > > static CPUCaches epyc_cache_info = { > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, > X86CPUDefinition *def, Error **errp) > /* legacy-cache defaults to 'off' if CPU model provides cache info */ > cpu->legacy_cache = !def->cache_info; > > + /* Set auto_topoext if both machine property and CPU model supports it > */ > + cpu->auto_topoext = cpu->auto_topoext & def->auto_topoext; This could be more appropriate like this. cpu->auto_topoext = cpu->auto_topoext && def->auto_topoext; > + > /* Special cases not set in the X86CPUDefinition structs: */ > /* TODO: in-kernel irqchip for hvf */ > if (kvm_enabled()) { > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = { > DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true), > > /* > + * auto-topoext property will be used to enable topoext feature. > + * This will be disabled on all the older CPU models. Will be > + * enabled on newer CPU modeles which can support topology extention. > + */ > + DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false), > + > + /* > * From "Requirements for Implementing the Microsoft > * Hypervisor Interface": > * https://docs.microsoft.com/en-us/virtualization/hyper-v-on- > windows/reference/tlfs > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 89c82be..8783d36 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1409,6 +1409,11 @@ struct X86CPU { > */ > bool legacy_cache; > > + /* Compatibility bits to enable topoext feature on all newer machines > + * Disabled on older machines. Enabled on newer CPU models > + */ > + bool auto_topoext; > + > /* Compatibility bits for old machine types: */ > bool enable_cpuid_0xb; > > -- > 1.8.3.1
On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote: > Hi Eduardo, > Planning to make couple of changes after testing and review. Let me know if I missed something. > Will wait for your feedback before the next revision. > > > -----Original Message----- > > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > > On Behalf Of Babu Moger > > Sent: Friday, June 8, 2018 5:56 PM > > To: mst@redhat.com; marcel.apfelbaum@gmail.com; pbonzini@redhat.com; > > rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com > > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu > > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com > > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage > > topoext > > > > Introduce the auto_topoext bit to to control topoext feature. > > > > Also add new field auto_topoext(in X86CPUDefinition). This will > > be used to enable topoext on newer CPU models where topoext can > > be supported. > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > --- > > include/hw/i386/pc.h | 4 ++++ > > target/i386/cpu.c | 12 ++++++++++++ > > target/i386/cpu.h | 5 +++++ > > 3 files changed, 21 insertions(+) > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > index 04d1f8c..cc30ec3 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 = TYPE_X86_CPU,\ > > + .property = "auto-topoext",\ > > + .value = "off",\ > > }, > > We don't need this. We are already setting this false in x86_cpu_properties. > But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we define it. > That will be a separate patch. Correct? PC_COMPAT_3_0 will exist only on QEMU 3.1. If you want to define something as the default behavior in pc-3.0, just define it as the default in the device code, which is exactly what you are already doing in x86_cpu_load_def() below. In other words, just set auto_topoext=true on the CPU model table for EPYC, and it should be enough. On PC_COMPAT_2_12, both would work: { TYPE_X86_CPU, "auto-topoext", "off" } or { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }. I prefer the latter, but both would work. > > > > > #define PC_COMPAT_2_11 \ > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 86fb1a4..d3411ed 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition { > > FeatureWordArray features; > > const char *model_id; > > CPUCaches *cache_info; > > + /* Set it if topoext can be enabled in CPU models */ > > + int auto_topoext; > > }; > > > > static CPUCaches epyc_cache_info = { > > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, > > X86CPUDefinition *def, Error **errp) > > /* legacy-cache defaults to 'off' if CPU model provides cache info */ > > cpu->legacy_cache = !def->cache_info; > > > > + /* Set auto_topoext if both machine property and CPU model supports it > > */ > > + cpu->auto_topoext = cpu->auto_topoext & def->auto_topoext; > > This could be more appropriate like this. > > cpu->auto_topoext = cpu->auto_topoext && def->auto_topoext; cpu->auto_topoext is supposed to be already false, here. Why not just: cpu->auto_topoext = def->auto_topoext; ? > > > + > > /* Special cases not set in the X86CPUDefinition structs: */ > > /* TODO: in-kernel irqchip for hvf */ > > if (kvm_enabled()) { > > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = { > > DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true), > > > > /* > > + * auto-topoext property will be used to enable topoext feature. > > + * This will be disabled on all the older CPU models. Will be > > + * enabled on newer CPU modeles which can support topology extention. > > + */ > > + DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false), > > + > > + /* > > * From "Requirements for Implementing the Microsoft > > * Hypervisor Interface": > > * https://docs.microsoft.com/en-us/virtualization/hyper-v-on- > > windows/reference/tlfs > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index 89c82be..8783d36 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1409,6 +1409,11 @@ struct X86CPU { > > */ > > bool legacy_cache; > > > > + /* Compatibility bits to enable topoext feature on all newer machines > > + * Disabled on older machines. Enabled on newer CPU models > > + */ > > + bool auto_topoext; > > + > > /* Compatibility bits for old machine types: */ > > bool enable_cpuid_0xb; > > > > -- > > 1.8.3.1 >
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, June 11, 2018 3:46 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 v13 2/5] i386: Introduce auto_topoext bit to manage > topoext > > On Mon, Jun 11, 2018 at 08:25:32PM +0000, Moger, Babu wrote: > > Hi Eduardo, > > Planning to make couple of changes after testing and review. Let me know > if I missed something. > > Will wait for your feedback before the next revision. > > > > > -----Original Message----- > > > From: kvm-owner@vger.kernel.org [mailto:kvm- > owner@vger.kernel.org] > > > On Behalf Of Babu Moger > > > Sent: Friday, June 8, 2018 5:56 PM > > > To: mst@redhat.com; marcel.apfelbaum@gmail.com; > pbonzini@redhat.com; > > > rth@twiddle.net; ehabkost@redhat.com; mtosatti@redhat.com > > > Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; Moger, Babu > > > <Babu.Moger@amd.com>; kash@tripleback.net; geoff@hostfission.com > > > Subject: [PATCH v13 2/5] i386: Introduce auto_topoext bit to manage > > > topoext > > > > > > Introduce the auto_topoext bit to to control topoext feature. > > > > > > Also add new field auto_topoext(in X86CPUDefinition). This will > > > be used to enable topoext on newer CPU models where topoext can > > > be supported. > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > --- > > > include/hw/i386/pc.h | 4 ++++ > > > target/i386/cpu.c | 12 ++++++++++++ > > > target/i386/cpu.h | 5 +++++ > > > 3 files changed, 21 insertions(+) > > > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > > > index 04d1f8c..cc30ec3 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 = TYPE_X86_CPU,\ > > > + .property = "auto-topoext",\ > > > + .value = "off",\ > > > }, > > > > We don't need this. We are already setting this false in > x86_cpu_properties. > > But we need to set it "on" for EPYC on PC_COMPAT_3_0 whenever we > define it. > > That will be a separate patch. Correct? > > PC_COMPAT_3_0 will exist only on QEMU 3.1. If you want to define > something as the default behavior in pc-3.0, just define it as > the default in the device code, which is exactly what you are > already doing in x86_cpu_load_def() below. > > In other words, just set auto_topoext=true on the CPU model table > for EPYC, and it should be enough. > > On PC_COMPAT_2_12, both would work: > { TYPE_X86_CPU, "auto-topoext", "off" } > or > { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }. > > I prefer the latter, but both would work. Ok. Sure > > > > > > > > > > #define PC_COMPAT_2_11 \ > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index 86fb1a4..d3411ed 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -1283,6 +1283,8 @@ struct X86CPUDefinition { > > > FeatureWordArray features; > > > const char *model_id; > > > CPUCaches *cache_info; > > > + /* Set it if topoext can be enabled in CPU models */ > > > + int auto_topoext; > > > }; > > > > > > static CPUCaches epyc_cache_info = { > > > @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, > > > X86CPUDefinition *def, Error **errp) > > > /* legacy-cache defaults to 'off' if CPU model provides cache info */ > > > cpu->legacy_cache = !def->cache_info; > > > > > > + /* Set auto_topoext if both machine property and CPU model > supports it > > > */ > > > + cpu->auto_topoext = cpu->auto_topoext & def->auto_topoext; > > > > This could be more appropriate like this. > > > > cpu->auto_topoext = cpu->auto_topoext && def->auto_topoext; > > cpu->auto_topoext is supposed to be already false, here. Why not > just: > cpu->auto_topoext = def->auto_topoext; > ? Ok. Will do it. > > > > > > + > > > /* Special cases not set in the X86CPUDefinition structs: */ > > > /* TODO: in-kernel irqchip for hvf */ > > > if (kvm_enabled()) { > > > @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = { > > > DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true), > > > > > > /* > > > + * auto-topoext property will be used to enable topoext feature. > > > + * This will be disabled on all the older CPU models. Will be > > > + * enabled on newer CPU modeles which can support topology > extention. > > > + */ > > > + DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false), > > > + > > > + /* > > > * From "Requirements for Implementing the Microsoft > > > * Hypervisor Interface": > > > * https://docs.microsoft.com/en-us/virtualization/hyper-v-on- > > > windows/reference/tlfs > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > > index 89c82be..8783d36 100644 > > > --- a/target/i386/cpu.h > > > +++ b/target/i386/cpu.h > > > @@ -1409,6 +1409,11 @@ struct X86CPU { > > > */ > > > bool legacy_cache; > > > > > > + /* Compatibility bits to enable topoext feature on all newer machines > > > + * Disabled on older machines. Enabled on newer CPU models > > > + */ > > > + bool auto_topoext; > > > + > > > /* Compatibility bits for old machine types: */ > > > bool enable_cpuid_0xb; > > > > > > -- > > > 1.8.3.1 > > > > -- > Eduardo
On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote: [...] > On PC_COMPAT_2_12, both would work: > { TYPE_X86_CPU, "auto-topoext", "off" } > or > { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }. > > I prefer the latter, but both would work. Oh, while we're at it: please name the property "x-auto-topoext", to indicate it's only for QEMU internal use or debugging, and not a supported command-line option.
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Eduardo Habkost > Sent: Monday, June 11, 2018 4:05 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 v13 2/5] i386: Introduce auto_topoext bit to manage > topoext > > On Mon, Jun 11, 2018 at 05:46:23PM -0300, Eduardo Habkost wrote: > [...] > > On PC_COMPAT_2_12, both would work: > > { TYPE_X86_CPU, "auto-topoext", "off" } > > or > > { "EPYC" "-" TYPE_X86_CPU, "auto-topoext", "off" }. > > > > I prefer the latter, but both would work. > > Oh, while we're at it: please name the property "x-auto-topoext", > to indicate it's only for QEMU internal use or debugging, and not > a supported command-line option. Ok. Sure. > > -- > Eduardo
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 04d1f8c..cc30ec3 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 = TYPE_X86_CPU,\ + .property = "auto-topoext",\ + .value = "off",\ }, #define PC_COMPAT_2_11 \ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 86fb1a4..d3411ed 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1283,6 +1283,8 @@ struct X86CPUDefinition { FeatureWordArray features; const char *model_id; CPUCaches *cache_info; + /* Set it if topoext can be enabled in CPU models */ + int auto_topoext; }; static CPUCaches epyc_cache_info = { @@ -3517,6 +3519,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) /* legacy-cache defaults to 'off' if CPU model provides cache info */ cpu->legacy_cache = !def->cache_info; + /* Set auto_topoext if both machine property and CPU model supports it */ + cpu->auto_topoext = cpu->auto_topoext & def->auto_topoext; + /* Special cases not set in the X86CPUDefinition structs: */ /* TODO: in-kernel irqchip for hvf */ if (kvm_enabled()) { @@ -5382,6 +5387,13 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("legacy-cache", X86CPU, legacy_cache, true), /* + * auto-topoext property will be used to enable topoext feature. + * This will be disabled on all the older CPU models. Will be + * enabled on newer CPU modeles which can support topology extention. + */ + DEFINE_PROP_BOOL("auto-topoext", X86CPU, auto_topoext, false), + + /* * From "Requirements for Implementing the Microsoft * Hypervisor Interface": * https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/tlfs diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 89c82be..8783d36 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1409,6 +1409,11 @@ struct X86CPU { */ bool legacy_cache; + /* Compatibility bits to enable topoext feature on all newer machines + * Disabled on older machines. Enabled on newer CPU models + */ + bool auto_topoext; + /* Compatibility bits for old machine types: */ bool enable_cpuid_0xb;
Introduce the auto_topoext bit to to control topoext feature. Also add new field auto_topoext(in X86CPUDefinition). This will be used to enable topoext on newer CPU models where topoext can be supported. Signed-off-by: Babu Moger <babu.moger@amd.com> --- include/hw/i386/pc.h | 4 ++++ target/i386/cpu.c | 12 ++++++++++++ target/i386/cpu.h | 5 +++++ 3 files changed, 21 insertions(+)