> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@xxxxxxxxxx] > Sent: Tuesday, June 12, 2018 12:40 PM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; pbonzini@xxxxxxxxxx; > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; qemu-devel@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; kash@xxxxxxxxxxxxxx; geoff@xxxxxxxxxxxxxxx; Jiri > Denemark <jdenemar@xxxxxxxxxx> > 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
