Re: [Qemu-devel] [PATCH v7 5/9] i386: Use the statically loaded cache definitions

[Moger, Babu]

> -----Original Message-----
> From: Eduardo Habkost [mailto:address@hidden
> Sent: Tuesday, May 8, 2018 9:12 AM
> 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 5/9] i386: Use the statically loaded
> cache definitions
> 
> On Mon, May 07, 2018 at 11:39:45PM +0000, Moger, Babu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Eduardo Habkost [mailto:address@hidden
> > > Sent: Monday, May 7, 2018 2:38 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 5/9] i386: Use the statically loaded
> > > cache definitions
> > >
> > > On Thu, Apr 26, 2018 at 11:26:45AM -0500, Babu Moger wrote:
> > > > Use the statically loaded cache definitions if available
> > > > and legacy-cache parameter is not set.
> > > >
> > > > Signed-off-by: Babu Moger <address@hidden>
> > > > Tested-by: Geoffrey McRae <address@hidden>
> > >
> > > Now that I'm looking at the rest of the code, this seems
> > > incomplete:
> > >
> > > What about CPUID[2], CPUID[4]?  They are still referring to the
> > > old legacy cache structs.
> >
> > There is no change in CPUID[2], CPUID[4] behavior. It is intel specific. It 
> > did
> not change. So, we don't need to change that.
> > In case it changes in future, we can add it later. AMD does not use CPUID[2]
> and CPUID[4].
> 
> Right, now I see that CPUID 2h-4h are reserved on AMD.
> 
> However, X86CPUDefinition::cache_info is not documented as
> AMD-specific, so developers changing the code CPU model table in
> the future would expect it to affect all cache CPUID leaves.
> 
> We could document it as AMD-specific, but it's simpler to just
> make it affect all CPUID leaves.

Ok. Will make those changes. Will check for cache_info pointer and legacy_cache 
like we do for 0x80000005. 
> 
> >
> > >
> > > If their corresponding code isn't updated to use env->cache_info,
> > > those leaves will be inconsistent with CPUID[0x8000005] and
> > > CPUID[0x80000006].
> > >
> > > > ---
> > > >  target/i386/cpu.c | 22 +++++++++++++++++-----
> > > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index a27b658..56d2f0b 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -3941,8 +3941,13 @@ void cpu_x86_cpuid(CPUX86State *env,
> > > uint32_t index, uint32_t count,
> > > >                 (L1_ITLB_2M_ASSOC <<  8) | (L1_ITLB_2M_ENTRIES);
> > > >          *ebx = (L1_DTLB_4K_ASSOC << 24) | (L1_DTLB_4K_ENTRIES << 16)
> | \
> > > >                 (L1_ITLB_4K_ASSOC <<  8) | (L1_ITLB_4K_ENTRIES);
> > > > -        *ecx = encode_cache_cpuid80000005(&l1d_cache_amd);
> > > > -        *edx = encode_cache_cpuid80000005(&l1i_cache_amd);
> > > > +        if (env->cache_info.valid && !cpu->legacy_cache) {
> > > > +            *ecx = encode_cache_cpuid80000005(&env-
> > > >cache_info.l1d_cache);
> > > > +            *edx = encode_cache_cpuid80000005(&env-
> > > >cache_info.l1i_cache);
> > > > +        } else {
> > > > +            *ecx = encode_cache_cpuid80000005(&l1d_cache_amd);
> > > > +            *edx = encode_cache_cpuid80000005(&l1i_cache_amd);
> > > > +        }
> > > >          break;
> > > >      case 0x80000006:
> > > >          /* cache info (L2 cache) */
> > > > @@ -3958,9 +3963,16 @@ void cpu_x86_cpuid(CPUX86State *env,
> > > uint32_t index, uint32_t count,
> > > >                 (L2_DTLB_4K_ENTRIES << 16) | \
> > > >                 (AMD_ENC_ASSOC(L2_ITLB_4K_ASSOC) << 12) | \
> > > >                 (L2_ITLB_4K_ENTRIES);
> > > > -        encode_cache_cpuid80000006(&l2_cache_amd,
> > > > -                                   cpu->enable_l3_cache ? &l3_cache : 
> > > > NULL,
> > > > -                                   ecx, edx);
> > > > +        if (env->cache_info.valid && !cpu->legacy_cache) {
> > > > +            encode_cache_cpuid80000006(&env->cache_info.l2_cache,
> > > > +                                       cpu->enable_l3_cache ?
> > > > +                                       &env->cache_info.l3_cache : 
> > > > NULL,
> > > > +                                       ecx, edx);
> > > > +        } else {
> > > > +            encode_cache_cpuid80000006(&l2_cache_amd,
> > > > +                                       cpu->enable_l3_cache ? 
> > > > &l3_cache : NULL,
> > > > +                                       ecx, edx);
> > > > +        }
> > > >          break;
> > > >      case 0x80000007:
> > > >          *eax = 0;
> > > > --
> > > > 2.7.4
> > > >
> > > >
> > >
> > > --
> > > Eduardo
> 
> --
> Eduardo