Eduardo, Thanks for all the comments. Will respond to each one separately. > -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@xxxxxxxxxx] > Sent: Monday, May 7, 2018 2:05 PM > To: Moger, Babu <Babu.Moger@xxxxxxx> > Cc: mst@xxxxxxxxxx; marcel@xxxxxxxxxx; pbonzini@xxxxxxxxxx; > rth@xxxxxxxxxxx; mtosatti@xxxxxxxxxx; geoff@xxxxxxxxxxxxxxx; > kash@xxxxxxxxxxxxxx; qemu-devel@xxxxxxxxxx; kvm@xxxxxxxxxxxxxxx > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > information consistently > > Hi, > > I was about to apply this because I assumed it was the same patch > I sent in March, but then I found this: > > On Thu, Apr 26, 2018 at 11:26:41AM -0500, Babu Moger wrote: > > From: Eduardo Habkost <ehabkost@xxxxxxxxxx> > > > > Instead of having a collection of macros that need to be used in > > complex expressions to build CPUID data, define a CPUCacheInfo > > struct that can hold information about a given cache. Helper > > functions will take a CPUCacheInfo struct as input to encode > > CPUID leaves for a cache. > > > > This will help us ensure consistency between cache information > > CPUID leaves, and make the existing inconsistencies in CPUID info > > more visible. > > > > Signed-off-by: Eduardo Habkost <ehabkost@xxxxxxxxxx> > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > > Tested-by: Geoffrey McRae <geoff@xxxxxxxxxxxxxxx> > [...] > > -#define L2_ASSOCIATIVITY 16 > [...] > > /*FIXME: CPUID leaf 0x80000006 is inconsistent with leaves 2 & 4 */ > > +static CPUCacheInfo l2_cache_amd = { > [...] > > + .associativity = 8, > [...] > > +}; > [...] > > case 0x80000006: > [...] > > - *ecx = (L2_SIZE_KB_AMD << 16) | \ > > - (AMD_ENC_ASSOC(L2_ASSOCIATIVITY) << 12) | \ > > - (L2_LINES_PER_TAG << 8) | (L2_LINE_SIZE); > [...] > > + encode_cache_cpuid80000006(&l2_cache_amd, > > + cpu->enable_l3_cache ? &l3_cache : NULL, > > + ecx, edx); > [...] > > The structs added by this patch are supposed to represent the > legacy cache sizes, and must match the old code. My original > patch set l2_cache_amd.associativity=16 because of that. > > This patch changes 0x80000006 from associativity=16 to > associativity=8. Why? The original code had a bug here. The associativity should have been 8. My earlier response from the thread http://patchwork.ozlabs.org/patch/884880/ This should have been 8-way. This is a bug. Will fix. This should have been (AMD_ENC_ASSOC(L2_ASSOCIATIVITY_AMD) << 12) > > -- > Eduardo
