| Message ID | 1528498581-131037-4-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote: > Enable TOPOEXT feature on EPYC CPU. This is required to support > hyperthreading on VM guests. Also extend xlevel to 0x8000001E. > > TOPOEXT feature is disabled for legacy machines. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > target/i386/cpu.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index d3411ed..4dd9a82 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > .xlevel = 0x8000000A, > .model_id = "AMD EPYC Processor", > .cache_info = &epyc_cache_info, > + .auto_topoext = 1, > }, > { > .name = "EPYC-IBPB", > @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > .xlevel = 0x8000000A, > .model_id = "AMD EPYC Processor (with IBPB)", > .cache_info = &epyc_cache_info, > + .auto_topoext = 1, > }, > }; > > @@ -4672,6 +4674,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); > + } I suggest moving this hunk to a separate patch. I'm not 100% sure yet if this will require compat_props code to disable auto-xlevel-increase on older machine-types.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, June 11, 2018 3:51 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 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Fri, Jun 08, 2018 at 06:56:19PM -0400, Babu Moger wrote: > > Enable TOPOEXT feature on EPYC CPU. This is required to support > > hyperthreading on VM guests. Also extend xlevel to 0x8000001E. > > > > TOPOEXT feature is disabled for legacy machines. > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > --- > > target/i386/cpu.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index d3411ed..4dd9a82 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > > .xlevel = 0x8000000A, > > .model_id = "AMD EPYC Processor", > > .cache_info = &epyc_cache_info, > > + .auto_topoext = 1, > > }, > > { > > .name = "EPYC-IBPB", > > @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = { > > .xlevel = 0x8000000A, > > .model_id = "AMD EPYC Processor (with IBPB)", > > .cache_info = &epyc_cache_info, > > + .auto_topoext = 1, > > }, > > }; > > > > @@ -4672,6 +4674,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); > > + } > > I suggest moving this hunk to a separate patch. I'm not 100% Sure. Will move it to separate patch. > sure yet if this will require compat_props code to disable > auto-xlevel-increase on older machine-types. > > -- > Eduardo
On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote: [...] > > + /* TOPOEXT feature requires 0x8000001E */ > > + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) { > > + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > + } > > I suggest moving this hunk to a separate patch. I'm not 100% > sure yet if this will require compat_props code to disable > auto-xlevel-increase on older machine-types. The problem here is that: $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext currently results in xlevel=0x8000001A, since QEMU 1.3. (The same applies to all machine-types between 1.3 and 2.12) I was hoping that we could declare topoext as non-migration-safe, but I believe libvirt will already include "topoext" when using "host-model" if the host CPU supports TOPOEXT. Jiri, can you confirm that? We can address that with a "x-topoext-auto-xlevel" property, set to true on all CPU models by default, and disabled by PC_COMPAT_2_12. The code would become: if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) { x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); } Or, we could simply declare that "-cpu Opteron_G4,+topoext" will never increase xlevel automatically (on any machine-type), and change the code above to: if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) { x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); }
> -----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:10 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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Mon, Jun 11, 2018 at 05:50:30PM -0300, Eduardo Habkost wrote: > [...] > > > + /* TOPOEXT feature requires 0x8000001E */ > > > + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) > { > > > + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, > 0x8000001E); > > > + } > > > > I suggest moving this hunk to a separate patch. I'm not 100% > > sure yet if this will require compat_props code to disable > > auto-xlevel-increase on older machine-types. > > The problem here is that: > $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext > currently results in xlevel=0x8000001A, since QEMU 1.3. > > (The same applies to all machine-types between 1.3 and 2.12) > > I was hoping that we could declare topoext as non-migration-safe, > but I believe libvirt will already include "topoext" when using > "host-model" if the host CPU supports TOPOEXT. Jiri, can you > confirm that? > > We can address that with a "x-topoext-auto-xlevel" property, set > to true on all CPU models by default, and disabled by > PC_COMPAT_2_12. > > The code would become: > > if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] & > CPUID_EXT3_TOPOEXT) { > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > } > > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will > never increase xlevel automatically (on any machine-type), and > change the code above to: > > if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & > CPUID_EXT3_TOPOEXT) { > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > } I was going to do this. But there is one problem. We don't set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't work. One more thing I noticed that feature setting should happen much before x86_cpu_realizefn. Couple of options. First option. 1. Set both feature and xlevel here(in x86_cpu_expand_features). if (cpu->x_auto_topoext { env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT; x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); } 2. And remove feature setting in x86_cpu_realizefn. Or Second option 1.Set the feature bit in CPU model table. 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext 3. And remove feature setting in x86_cpu_realizefn. I prefer the second option. > > -- > Eduardo
On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote: > > [...] > > > > + /* TOPOEXT feature requires 0x8000001E */ > > > > + if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_TOPOEXT) > > { > > > > + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, > > 0x8000001E); > > > > + } > > > > > > I suggest moving this hunk to a separate patch. I'm not 100% > > > sure yet if this will require compat_props code to disable > > > auto-xlevel-increase on older machine-types. > > > > The problem here is that: > > $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext > > currently results in xlevel=0x8000001A, since QEMU 1.3. > > > > (The same applies to all machine-types between 1.3 and 2.12) > > > > I was hoping that we could declare topoext as non-migration-safe, > > but I believe libvirt will already include "topoext" when using > > "host-model" if the host CPU supports TOPOEXT. Jiri, can you > > confirm that? > > > > We can address that with a "x-topoext-auto-xlevel" property, set > > to true on all CPU models by default, and disabled by > > PC_COMPAT_2_12. > > > > The code would become: > > > > if (cpu->topoext_auto_xlevel && env->features[FEAT_8000_0001_ECX] & > > CPUID_EXT3_TOPOEXT) { > > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > } > > > > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will > > never increase xlevel automatically (on any machine-type), and > > change the code above to: > > > > if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & > > CPUID_EXT3_TOPOEXT) { > > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > } > > I was going to do this. But there is one problem. We don't > set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't > work. Won't this work if the auto_topoext handling is done before x86_cpu_expand_features() is called? > One more thing I noticed that feature setting should happen > much before x86_cpu_realizefn. Why? > > Couple of options. > First option. > 1. Set both feature and xlevel here(in x86_cpu_expand_features). > if (cpu->x_auto_topoext { > env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT; > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > } > 2. And remove feature setting in x86_cpu_realizefn. This would make TOPOEXT be included in 'query-cpu-model-expansion model=EPYC', which would be incorrect because TOPOEXT won't always be enabled when using EPYC. > > Or > > Second option > 1.Set the feature bit in CPU model table. > 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext > 3. And remove feature setting in x86_cpu_realizefn. > > I prefer the second option. Same here: TOPOEXT would be included in 'query-cpu-model-expansion model=EPYC', and this would be incorrect. I'm starting to think that enabling TOPOEXT automatically is adding too much complexity and compatibility problems, and it's better to leave this task to management software. The main problem here is: This works today with QEMU 2.12 + Linux <= 4.15: $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2" and must keep working with QEMU 3.0 and Linux <= 4.15. In addition to that, the results for: $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] must be deterministic and expose exactly the same CPUID data even if host hardware or software changes, as long as the QEMU command-line is the same. Do you see a way to fulfill those two constraints while making "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically?
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Tuesday, June 12, 2018 12: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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Tue, Jun 12, 2018 at 04:29:25PM +0000, Moger, Babu wrote: > > > [...] > > > > > + /* TOPOEXT feature requires 0x8000001E */ > > > > > + if (env->features[FEAT_8000_0001_ECX] & > CPUID_EXT3_TOPOEXT) > > > { > > > > > + x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, > > > 0x8000001E); > > > > > + } > > > > > > > > I suggest moving this hunk to a separate patch. I'm not 100% > > > > sure yet if this will require compat_props code to disable > > > > auto-xlevel-increase on older machine-types. > > > > > > The problem here is that: > > > $QEMU -machine pc-i440fx-1.3 -cpu Opteron_G4,+topoext > > > currently results in xlevel=0x8000001A, since QEMU 1.3. > > > > > > (The same applies to all machine-types between 1.3 and 2.12) > > > > > > I was hoping that we could declare topoext as non-migration-safe, > > > but I believe libvirt will already include "topoext" when using > > > "host-model" if the host CPU supports TOPOEXT. Jiri, can you > > > confirm that? > > > > > > We can address that with a "x-topoext-auto-xlevel" property, set > > > to true on all CPU models by default, and disabled by > > > PC_COMPAT_2_12. > > > > > > The code would become: > > > > > > if (cpu->topoext_auto_xlevel && env- > >features[FEAT_8000_0001_ECX] & > > > CPUID_EXT3_TOPOEXT) { > > > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > > } > > > > > > Or, we could simply declare that "-cpu Opteron_G4,+topoext" will > > > never increase xlevel automatically (on any machine-type), and > > > change the code above to: > > > > > > if (cpu->auto_topoext && env->features[FEAT_8000_0001_ECX] & > > > CPUID_EXT3_TOPOEXT) { > > > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > > } > > > > I was going to do this. But there is one problem. We don't > > set the CPUID_EXT3_TOPOEXT in CPU model table. So this won't > > work. > > Won't this work if the auto_topoext handling is done before > x86_cpu_expand_features() is called? Yes. That works. We can go with this approach. > > > > One more thing I noticed that feature setting should happen > > much before x86_cpu_realizefn. > > Why? I was wrong here. It should be done before x86_cpu_expand_features. > > > > > Couple of options. > > First option. > > 1. Set both feature and xlevel here(in x86_cpu_expand_features). > > if (cpu->x_auto_topoext { > > env->features[FEAT_8000_0001_ECX] |= CPUID_EXT3_TOPOEXT; > > x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000001E); > > } > > 2. And remove feature setting in x86_cpu_realizefn. > > This would make TOPOEXT be included in 'query-cpu-model-expansion > model=EPYC', which would be incorrect because TOPOEXT won't > always be enabled when using EPYC. > > > > > > Or > > > > Second option > > 1.Set the feature bit in CPU model table. > > 2. Set xlevel in x86_cpu_expand_features using cpu->x_auto_topoext > > 3. And remove feature setting in x86_cpu_realizefn. > > > > I prefer the second option. > > Same here: TOPOEXT would be included in > 'query-cpu-model-expansion model=EPYC', and this would be > incorrect. > > > I'm starting to think that enabling TOPOEXT automatically is > adding too much complexity and compatibility problems, and it's > better to leave this task to management software. > > The main problem here is: > > This works today with QEMU 2.12 + Linux <= 4.15: > $ $QEMU -machine pc -cpu EPYC,enforce -smp > 8,sockets=2,cores=2,threads=2" > and must keep working with QEMU 3.0 and Linux <= 4.15. > > In addition to that, the results for: > $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] > must be deterministic and expose exactly the same CPUID data even > if host hardware or software changes, as long as the QEMU > command-line is the same. > > Do you see a way to fulfill those two constraints while making > "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically? > Now(setting feature before x86_cpu_expand_features), enabling TOPOEXT appears to work fine. > -- > Eduardo
On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote: [...] > > I'm starting to think that enabling TOPOEXT automatically is > > adding too much complexity and compatibility problems, and it's > > better to leave this task to management software. > > > > The main problem here is: > > > > This works today with QEMU 2.12 + Linux <= 4.15: > > $ $QEMU -machine pc -cpu EPYC,enforce -smp > > 8,sockets=2,cores=2,threads=2" > > and must keep working with QEMU 3.0 and Linux <= 4.15. > > > > In addition to that, the results for: > > $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] > > must be deterministic and expose exactly the same CPUID data even > > if host hardware or software changes, as long as the QEMU > > command-line is the same. > > > > Do you see a way to fulfill those two constraints while making > > "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically? > > > > Now(setting feature before x86_cpu_expand_features), enabling > TOPOEXT appears to work fine. What about the above constraints? Are you really fulfilling them? This one is tricky: ] This works today with QEMU 2.12 + Linux <= 4.15: ] $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2" ] and must keep working with QEMU 3.0 and Linux <= 4.15. If we enable TOPOEXT unconditionally, the command-line won't work with Linux <= 4.15. If we enable TOPOEXT only if the kernel returns TOPOEXT on GET_SUPPORTED_CPUID, we break the second constraint: ] The results for: ] $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] ] must be deterministic and expose exactly the same CPUID data even ] if host hardware or software changes, as long as the QEMU ] command-line is the same.
On 06/12/2018 02:05 PM, Eduardo Habkost wrote: > On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote: > [...] >>> I'm starting to think that enabling TOPOEXT automatically is >>> adding too much complexity and compatibility problems, and it's >>> better to leave this task to management software. >>> >>> The main problem here is: >>> >>> This works today with QEMU 2.12 + Linux <= 4.15: >>> $ $QEMU -machine pc -cpu EPYC,enforce -smp >>> 8,sockets=2,cores=2,threads=2" >>> and must keep working with QEMU 3.0 and Linux <= 4.15. >>> >>> In addition to that, the results for: >>> $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] >>> must be deterministic and expose exactly the same CPUID data even >>> if host hardware or software changes, as long as the QEMU >>> command-line is the same. >>> >>> Do you see a way to fulfill those two constraints while making >>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically? >>> >> Now(setting feature before x86_cpu_expand_features), enabling >> TOPOEXT appears to work fine. > What about the above constraints? Are you really fulfilling > them? > > This one is tricky: > > ] This works today with QEMU 2.12 + Linux <= 4.15: > ] $ $QEMU -machine pc -cpu EPYC,enforce -smp 8,sockets=2,cores=2,threads=2" > ] and must keep working with QEMU 3.0 and Linux <= 4.15. This works fine on kernel <= 4.15 with some warnings(-smp 8,sockets=2,cores=2,threads=2 -cpu EPYC). qemu-system-x86_64: warning: host doesn't support requested feature: CPUID.80000001H:EDX.rdtscp [bit 27] qemu-system-x86_64: warning: host doesn't support requested feature: CPUID.80000001H:ECX.topoext [bit 22] qemu-system-x86_64: This family of AMD CPU doesn't support hyperthreading(2). Please configure -smp options properly or try enabling topoext feature. continues.. > > If we enable TOPOEXT unconditionally, the command-line won't work > with Linux <= 4.15. > > If we enable TOPOEXT only if the kernel returns TOPOEXT on > GET_SUPPORTED_CPUID, we break the second constraint: > > ] The results for: > ] $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] > ] must be deterministic and expose exactly the same CPUID data even > ] if host hardware or software changes, as long as the QEMU > ] command-line is the same. > This fails on kernel <= 4.15 with following messages((-smp 8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce). qemu-system-x86_64: warning: host doesn't support requested feature: CPUID.80000001H:EDX.rdtscp [bit 27] qemu-system-x86_64: warning: host doesn't support requested feature: CPUID.80000001H:ECX.topoext [bit 22] qemu-system-x86_64: Host doesn't support requested features exits.. What do you think our options are here?
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Babu Moger > Sent: Tuesday, June 12, 2018 2:47 PM > To: Eduardo Habkost <ehabkost@redhat.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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > > > On 06/12/2018 02:05 PM, Eduardo Habkost wrote: > > On Tue, Jun 12, 2018 at 06:38:08PM +0000, Moger, Babu wrote: > > [...] > >>> I'm starting to think that enabling TOPOEXT automatically is > >>> adding too much complexity and compatibility problems, and it's > >>> better to leave this task to management software. > >>> > >>> The main problem here is: > >>> > >>> This works today with QEMU 2.12 + Linux <= 4.15: > >>> $ $QEMU -machine pc -cpu EPYC,enforce -smp > >>> 8,sockets=2,cores=2,threads=2" > >>> and must keep working with QEMU 3.0 and Linux <= 4.15. > >>> > >>> In addition to that, the results for: > >>> $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] > >>> must be deterministic and expose exactly the same CPUID data even > >>> if host hardware or software changes, as long as the QEMU > >>> command-line is the same. > >>> > >>> Do you see a way to fulfill those two constraints while making > >>> "-machine pc-q35-3.0 -cpu EPYC" enable TOPOEXT automatically? > >>> > >> Now(setting feature before x86_cpu_expand_features), enabling > >> TOPOEXT appears to work fine. > > What about the above constraints? Are you really fulfilling > > them? > > > > This one is tricky: > > > > ] This works today with QEMU 2.12 + Linux <= 4.15: > > ] $ $QEMU -machine pc -cpu EPYC,enforce -smp > 8,sockets=2,cores=2,threads=2" > > ] and must keep working with QEMU 3.0 and Linux <= 4.15. > This works fine on kernel <= 4.15 with some warnings(-smp > 8,sockets=2,cores=2,threads=2 -cpu EPYC). > > qemu-system-x86_64: warning: host doesn't support requested feature: > CPUID.80000001H:EDX.rdtscp [bit 27] > qemu-system-x86_64: warning: host doesn't support requested feature: > CPUID.80000001H:ECX.topoext [bit 22] > qemu-system-x86_64: This family of AMD CPU doesn't support > hyperthreading(2). Please configure -smp options properly or try > enabling topoext feature. > continues.. > > > > If we enable TOPOEXT unconditionally, the command-line won't work > > with Linux <= 4.15. > > > > If we enable TOPOEXT only if the kernel returns TOPOEXT on > > GET_SUPPORTED_CPUID, we break the second constraint: > > > > ] The results for: > > ] $ $QEMU -machine pc-q35-3.0 -cpu EPYC,enforce [...] > > ] must be deterministic and expose exactly the same CPUID data even > > ] if host hardware or software changes, as long as the QEMU > > ] command-line is the same. > > > This fails on kernel <= 4.15 with following messages((-smp > 8,sockets=2,cores=2,threads=2 -cpu EPYC,enforce). > qemu-system-x86_64: warning: host doesn't support requested feature: > CPUID.80000001H:EDX.rdtscp [bit 27] > qemu-system-x86_64: warning: host doesn't support requested feature: > CPUID.80000001H:ECX.topoext [bit 22] > qemu-system-x86_64: Host doesn't support requested features > exits.. > > What do you think our options are here? Should we drop automatic topoext completely and move forward? What are your thoughts?
On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: [...] > > What do you think our options are here? > > Should we drop automatic topoext completely and move forward? > What are your thoughts? Let's drop automatic topoext by now, and see if we find solutions later. I don't want to hold the rest of the patches because of this. I'm thinking we could simply make kvm_arch_get_supported_cpuid() always return TOPOEXT on AMD CPUs, because the feature flag don't really depend on any KVM code to work (is that correct?).
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Wednesday, June 13, 2018 12:18 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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > [...] > > > What do you think our options are here? > > > > Should we drop automatic topoext completely and move forward? > > What are your thoughts? > > Let's drop automatic topoext by now, and see if we find solutions > later. I don't want to hold the rest of the patches because of > this. Ok. I will drop topoext. > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > always return TOPOEXT on AMD CPUs, because the feature flag don't > really depend on any KVM code to work (is that correct?). Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver. Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. > > -- > Eduardo
> -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Moger, Babu > Sent: Wednesday, June 13, 2018 1:11 PM > To: Eduardo Habkost <ehabkost@redhat.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; Jiri > Denemark <jdenemar@redhat.com> > Subject: RE: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Wednesday, June 13, 2018 12:18 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; Jiri > > Denemark <jdenemar@redhat.com> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > CPU > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > > [...] > > > > What do you think our options are here? > > > > > > Should we drop automatic topoext completely and move forward? > > > What are your thoughts? > > > > Let's drop automatic topoext by now, and see if we find solutions > > later. I don't want to hold the rest of the patches because of > > this. > > Ok. I will drop topoext. Sorry, I mean automatic topoext. > > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > > always return TOPOEXT on AMD CPUs, because the feature flag don't > > really depend on any KVM code to work (is that correct?). > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM > driver. > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. > > > > > -- > > Eduardo
On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote: > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Wednesday, June 13, 2018 12:18 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; Jiri > > Denemark <jdenemar@redhat.com> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > CPU > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > > [...] > > > > What do you think our options are here? > > > > > > Should we drop automatic topoext completely and move forward? > > > What are your thoughts? > > > > Let's drop automatic topoext by now, and see if we find solutions > > later. I don't want to hold the rest of the patches because of > > this. > > Ok. I will drop topoext. > > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > > always return TOPOEXT on AMD CPUs, because the feature flag don't > > really depend on any KVM code to work (is that correct?). > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM driver. > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. Hmm, this could actually solve all of our problems, then: We can forget about auto-topoext: just add TOPOEXT in kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to the CPU models where you are interested into (EPYC only?), and add topoext=off to pc-2.12 compat_props. Sorry for not noticing that before. I was incorrectly assuming that TOPOEXT was safe to enable only if it was returned by GET_SUPPORTED_CPUID.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Wednesday, June 13, 2018 1:18 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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote: > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Wednesday, June 13, 2018 12:18 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; Jiri > > > Denemark <jdenemar@redhat.com> > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > > CPU > > > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > > > [...] > > > > > What do you think our options are here? > > > > > > > > Should we drop automatic topoext completely and move forward? > > > > What are your thoughts? > > > > > > Let's drop automatic topoext by now, and see if we find solutions > > > later. I don't want to hold the rest of the patches because of > > > this. > > > > Ok. I will drop topoext. > > > > > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > > > always return TOPOEXT on AMD CPUs, because the feature flag don't > > > really depend on any KVM code to work (is that correct?). > > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM > driver. > > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. > > Hmm, this could actually solve all of our problems, then: > > We can forget about auto-topoext: just add TOPOEXT in > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to > the CPU models where you are interested into (EPYC only?), and > add topoext=off to pc-2.12 compat_props. > Ok Sure. > Sorry for not noticing that before. I was incorrectly assuming No problem. > that TOPOEXT was safe to enable only if it was returned by > GET_SUPPORTED_CPUID. > > -- > Eduardo
On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote: > > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Wednesday, June 13, 2018 1:18 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; Jiri > > Denemark <jdenemar@redhat.com> > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > CPU > > > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote: > > > > -----Original Message----- > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Sent: Wednesday, June 13, 2018 12:18 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; Jiri > > > > Denemark <jdenemar@redhat.com> > > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > > > CPU > > > > > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > > > > [...] > > > > > > What do you think our options are here? > > > > > > > > > > Should we drop automatic topoext completely and move forward? > > > > > What are your thoughts? > > > > > > > > Let's drop automatic topoext by now, and see if we find solutions > > > > later. I don't want to hold the rest of the patches because of > > > > this. > > > > > > Ok. I will drop topoext. > > > > > > > > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > > > > always return TOPOEXT on AMD CPUs, because the feature flag don't > > > > really depend on any KVM code to work (is that correct?). > > > > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in KVM > > driver. > > > > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. > > > > Hmm, this could actually solve all of our problems, then: > > > > We can forget about auto-topoext: just add TOPOEXT in > > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to > > the CPU models where you are interested into (EPYC only?), and > > add topoext=off to pc-2.12 compat_props. > > > > Ok Sure. Sorry, I forgot we still need to decide what to do if TOPOEXT is enabled in the CPU model (or command-line) but the -smp options are not compatible with it. In other words, what should guest see on CPUID if using: "-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64" or: "-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64" I wonder what would happen if we just return zeroes on CPUID[0x800001E] if !topology_supports_topoext(), instead of trying to clear/set TOPOEXT depending on the -smp option? It would make things much simpler for QEMU and libvirt.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Wednesday, June 13, 2018 1:49 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; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > CPU > > On Wed, Jun 13, 2018 at 06:21:58PM +0000, Moger, Babu wrote: > > > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Wednesday, June 13, 2018 1:18 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; Jiri > > > Denemark <jdenemar@redhat.com> > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD EPYC > > > CPU > > > > > > On Wed, Jun 13, 2018 at 06:10:30PM +0000, Moger, Babu wrote: > > > > > -----Original Message----- > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > > Sent: Wednesday, June 13, 2018 12:18 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; > Jiri > > > > > Denemark <jdenemar@redhat.com> > > > > > Subject: Re: [PATCH v13 3/5] i386: Enable TOPOEXT feature on AMD > EPYC > > > > > CPU > > > > > > > > > > On Wed, Jun 13, 2018 at 04:52:18PM +0000, Moger, Babu wrote: > > > > > [...] > > > > > > > What do you think our options are here? > > > > > > > > > > > > Should we drop automatic topoext completely and move forward? > > > > > > What are your thoughts? > > > > > > > > > > Let's drop automatic topoext by now, and see if we find solutions > > > > > later. I don't want to hold the rest of the patches because of > > > > > this. > > > > > > > > Ok. I will drop topoext. > > > > > > > > > > > > > > I'm thinking we could simply make kvm_arch_get_supported_cpuid() > > > > > always return TOPOEXT on AMD CPUs, because the feature flag don't > > > > > really depend on any KVM code to work (is that correct?). > > > > > > > > Yes, that is correct. I don't see any dependent code on TOPOEXT in > KVM > > > driver. > > > > > > > > Ok. Let me add TOPOEXT flag for all the AMD cpus and see how it goes. > > > > > > Hmm, this could actually solve all of our problems, then: > > > > > > We can forget about auto-topoext: just add TOPOEXT in > > > kvm_arch_get_supported_cpuid(), add TOPOEXT unconditionally to > > > the CPU models where you are interested into (EPYC only?), and > > > add topoext=off to pc-2.12 compat_props. > > > > > > > Ok Sure. > > Sorry, I forgot we still need to decide what to do if TOPOEXT is > enabled in the CPU model (or command-line) but the -smp options > are not compatible with it. Yes. I have kept that check. But, I had to implement topology_supports_topoext bit differently. Reason for this is we need to disable this feature before the x86_cpu_expand_features. But problem is nr_cores and nr_threads are not populated at this time. It is populated in qemu_init_vcpus. Please take a look at topology_supports_topoext again. > > In other words, what should guest see on CPUID if using: > > "-machine pc-q35-3.0 -cpu EPYC -smp 64,cores=64" > or: > "-machine pc-q35-3.0 -cpu Opteron_G5,+topoext -smp 64,cores=64" > Tested both these cases. It works fine with some warning messages. > I wonder what would happen if we just return zeroes on > CPUID[0x800001E] if !topology_supports_topoext(), instead of > trying to clear/set TOPOEXT depending on the -smp option? It > would make things much simpler for QEMU and libvirt. I did not see that difference in behavior if we clear the bit versus return 0s. Sending new patches now. Please review. One note: I will going on vacation from June 20th for couple of weeks. If possible I would like to close this feature. If we cannot that is fine. Just an FYI. > > -- > Eduardo
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index d3411ed..4dd9a82 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -2574,6 +2574,7 @@ static X86CPUDefinition builtin_x86_defs[] = { .xlevel = 0x8000000A, .model_id = "AMD EPYC Processor", .cache_info = &epyc_cache_info, + .auto_topoext = 1, }, { .name = "EPYC-IBPB", @@ -2621,6 +2622,7 @@ static X86CPUDefinition builtin_x86_defs[] = { .xlevel = 0x8000000A, .model_id = "AMD EPYC Processor (with IBPB)", .cache_info = &epyc_cache_info, + .auto_topoext = 1, }, }; @@ -4672,6 +4674,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. TOPOEXT feature is disabled for legacy machines. Signed-off-by: Babu Moger <babu.moger@amd.com> --- target/i386/cpu.c | 7 +++++++ 1 file changed, 7 insertions(+)