| Message ID | 1524760009-24710-3-git-send-email-babu.moger@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Enable TOPOEXT to support hyperthreading on AMD CPU | expand |
On Thu, Apr 26, 2018 at 11:26:42AM -0500, Babu Moger wrote: > Add cache information in X86CPUDefinition and CPUX86State. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > Tested-by: Geoffrey McRae <geoff@hostfission.com> > --- > target/i386/cpu.c | 4 ++++ > target/i386/cpu.h | 8 ++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b6c1592..a518a0f 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1105,6 +1105,7 @@ struct X86CPUDefinition { > int stepping; > FeatureWordArray features; > const char *model_id; > + CPUCaches cache_info; > }; > > static X86CPUDefinition builtin_x86_defs[] = { > @@ -3242,6 +3243,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) > env->features[w] = def->features[w]; > } > > + /* Load Cache information from the X86CPUDefinition */ > + memcpy(&env->cache_info, &def->cache_info, sizeof(CPUCaches)); > + > /* Special cases not set in the X86CPUDefinition structs: */ > /* TODO: in-kernel irqchip for hvf */ > if (kvm_enabled()) { > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index fa03e2c..1213bb7 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1096,6 +1096,13 @@ typedef struct CPUCacheInfo { > } CPUCacheInfo; > > > +typedef struct CPUCaches { > + bool valid; > + CPUCacheInfo l1d_cache; > + CPUCacheInfo l1i_cache; > + CPUCacheInfo l2_cache; > + CPUCacheInfo l3_cache; > +} CPUCaches; > > typedef struct CPUX86State { > /* standard registers */ > @@ -1282,6 +1289,7 @@ typedef struct CPUX86State { > /* Features that were explicitly enabled/disabled */ > FeatureWordArray user_features; > uint32_t cpuid_model[12]; > + CPUCaches cache_info; Suggestion for a follow-up patch, or in case there's need to respin this series: what about making both X86CPUDefinition::cache_info and CPUX86State::cache_info pointers instead of embedded structs? This way you won't need the 'valid' field (you can just check if the pointer is NULL), and won't need the memcpy() above. This shouldn't block the series, though: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, May 7, 2018 2:10 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: mst@redhat.com; marcel@redhat.com; pbonzini@redhat.com; > rth@twiddle.net; mtosatti@redhat.com; geoff@hostfission.com; > kash@tripleback.net; qemu-devel@nongnu.org; kvm@vger.kernel.org > Subject: Re: [Qemu-devel] [PATCH v7 2/9] i386: Add cache information in > X86CPUDefinition > > On Thu, Apr 26, 2018 at 11:26:42AM -0500, Babu Moger wrote: > > Add cache information in X86CPUDefinition and CPUX86State. > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > --- > > target/i386/cpu.c | 4 ++++ > > target/i386/cpu.h | 8 ++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index b6c1592..a518a0f 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1105,6 +1105,7 @@ struct X86CPUDefinition { > > int stepping; > > FeatureWordArray features; > > const char *model_id; > > + CPUCaches cache_info; > > }; > > > > static X86CPUDefinition builtin_x86_defs[] = { > > @@ -3242,6 +3243,9 @@ static void x86_cpu_load_def(X86CPU *cpu, > X86CPUDefinition *def, Error **errp) > > env->features[w] = def->features[w]; > > } > > > > + /* Load Cache information from the X86CPUDefinition */ > > + memcpy(&env->cache_info, &def->cache_info, sizeof(CPUCaches)); > > + > > /* Special cases not set in the X86CPUDefinition structs: */ > > /* TODO: in-kernel irqchip for hvf */ > > if (kvm_enabled()) { > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index fa03e2c..1213bb7 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -1096,6 +1096,13 @@ typedef struct CPUCacheInfo { > > } CPUCacheInfo; > > > > > > +typedef struct CPUCaches { > > + bool valid; > > + CPUCacheInfo l1d_cache; > > + CPUCacheInfo l1i_cache; > > + CPUCacheInfo l2_cache; > > + CPUCacheInfo l3_cache; > > +} CPUCaches; > > > > typedef struct CPUX86State { > > /* standard registers */ > > @@ -1282,6 +1289,7 @@ typedef struct CPUX86State { > > /* Features that were explicitly enabled/disabled */ > > FeatureWordArray user_features; > > uint32_t cpuid_model[12]; > > + CPUCaches cache_info; > > Suggestion for a follow-up patch, or in case there's need to > respin this series: what about making both > X86CPUDefinition::cache_info and CPUX86State::cache_info pointers > instead of embedded structs? This way you won't need the 'valid' > field (you can just check if the pointer is NULL), and won't need > the memcpy() above. Sure. We can do that. Basically, initialize a static cache_info structure and use the address wherever applicable. Will try it. > > This shouldn't block the series, though: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > -- > Eduardo
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b6c1592..a518a0f 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1105,6 +1105,7 @@ struct X86CPUDefinition { int stepping; FeatureWordArray features; const char *model_id; + CPUCaches cache_info; }; static X86CPUDefinition builtin_x86_defs[] = { @@ -3242,6 +3243,9 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) env->features[w] = def->features[w]; } + /* Load Cache information from the X86CPUDefinition */ + memcpy(&env->cache_info, &def->cache_info, sizeof(CPUCaches)); + /* Special cases not set in the X86CPUDefinition structs: */ /* TODO: in-kernel irqchip for hvf */ if (kvm_enabled()) { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index fa03e2c..1213bb7 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1096,6 +1096,13 @@ typedef struct CPUCacheInfo { } CPUCacheInfo; +typedef struct CPUCaches { + bool valid; + CPUCacheInfo l1d_cache; + CPUCacheInfo l1i_cache; + CPUCacheInfo l2_cache; + CPUCacheInfo l3_cache; +} CPUCaches; typedef struct CPUX86State { /* standard registers */ @@ -1282,6 +1289,7 @@ typedef struct CPUX86State { /* Features that were explicitly enabled/disabled */ FeatureWordArray user_features; uint32_t cpuid_model[12]; + CPUCaches cache_info; /* MTRRs */ uint64_t mtrr_fixed[11];