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@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] > > On Behalf Of Babu Moger > > Sent: Friday, June 8, 2018 5:56 PM > > To: mst@xxxxxxxxxx; marcel.apfelbaum@xxxxxxxxx; pbonzini@xxxxxxxxxx; > > rth@xxxxxxxxxxx; ehabkost@xxxxxxxxxx; mtosatti@xxxxxxxxxx > > Cc: qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; Moger, Babu > > <Babu.Moger@xxxxxxx>; kash@xxxxxxxxxxxxxx; geoff@xxxxxxxxxxxxxxx > > 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@xxxxxxx> > > --- > > 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 > -- Eduardo
