Re: [Qemu-devel] [PATCH v7 2/9] i386: Add cache information in X86CPUDefinition

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Monday, May 7, 2018 2:10 PM
> To: Moger, Babu <address@hidden>
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden
> 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 <address@hidden>
> > Tested-by: Geoffrey McRae <address@hidden>
> > ---
> >  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 <address@hidden>
> 
> --
> Eduardo