| Message ID | 1524760009-24710-2-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > > 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@redhat.com> > Signed-off-by: Babu Moger <babu.moger@amd.com> > Tested-by: Geoffrey McRae <geoff@hostfission.com> [...] > -#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?
Eduardo, Thanks for all the comments. Will respond to each one separately. > -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > 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@redhat.com> > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > [...] > > -#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
On Mon, May 07, 2018 at 09:14:27PM +0000, Moger, Babu wrote: > Eduardo, > Thanks for all the comments. Will respond to each one separately. > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > > > 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@redhat.com> > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > [...] > > > -#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) If we want to change the associativity, we must keep the old values on older machine-types, which was the whole purpose of the "legacy-cache" property. I suggest using the new X86CPUDefinition::cache_info field if you want to make AMD CPUs report different associativity.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, May 7, 2018 4:27 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > information consistently > > On Mon, May 07, 2018 at 09:14:27PM +0000, Moger, Babu wrote: > > Eduardo, > > Thanks for all the comments. Will respond to each one separately. > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > > > > > 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@redhat.com> > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > > [...] > > > > -#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) > > If we want to change the associativity, we must keep the old > values on older machine-types, which was the whole purpose of the > "legacy-cache" property. > > I suggest using the new X86CPUDefinition::cache_info field if you > want to make AMD CPUs report different associativity. Ok. Sure. I will change it. Thanks > > -- > Eduardo
Hi Eduardo, One more comment below. > -----Original Message----- > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > On Behalf Of Moger, Babu > Sent: Monday, May 7, 2018 5:48 PM > To: Eduardo Habkost <ehabkost@redhat.com> > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > Subject: RE: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > information consistently > > > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Monday, May 7, 2018 4:27 PM > > To: Moger, Babu <Babu.Moger@amd.com> > > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > > information consistently > > > > On Mon, May 07, 2018 at 09:14:27PM +0000, Moger, Babu wrote: > > > Eduardo, > > > Thanks for all the comments. Will respond to each one separately. > > > > > > > -----Original Message----- > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > > > > > > > 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@redhat.com> > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > > > [...] > > > > > -#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? Keeping this to 16 will be a problem. It breaks the size rule(line size *associativity * partitions * sets). It asserts violating that rule. Now I remember. That is why I changed it to 8. I would think it would be better to fix it now. > > > > > > 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) > > > > If we want to change the associativity, we must keep the old > > values on older machine-types, which was the whole purpose of the > > "legacy-cache" property. > > > > I suggest using the new X86CPUDefinition::cache_info field if you > > want to make AMD CPUs report different associativity. > > Ok. Sure. I will change it. Thanks > > > > > -- > > Eduardo
On Tue, May 08, 2018 at 06:40:23PM +0000, Moger, Babu wrote: > Hi Eduardo, One more comment below. > > > -----Original Message----- > > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] > > On Behalf Of Moger, Babu > > Sent: Monday, May 7, 2018 5:48 PM > > To: Eduardo Habkost <ehabkost@redhat.com> > > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > > Subject: RE: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > > information consistently > > > > > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Monday, May 7, 2018 4:27 PM > > > To: Moger, Babu <Babu.Moger@amd.com> > > > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > > > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > > > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > > > information consistently > > > > > > On Mon, May 07, 2018 at 09:14:27PM +0000, Moger, Babu wrote: > > > > Eduardo, > > > > Thanks for all the comments. Will respond to each one separately. > > > > > > > > > -----Original Message----- > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > > > > > > > > > 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@redhat.com> > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > > > > [...] > > > > > > -#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? > > Keeping this to 16 will be a problem. It breaks the size rule(line size *associativity * partitions * sets). > It asserts violating that rule. Now I remember. That is why I changed it to 8. I would think it would be > better to fix it now. You can fix it, no problem. You just need to make sure CPUID[0x80000006] data for old machine-types without topoext will stay the same. Remember that l2_cache_amd is just for the legacy values enabled by legacy-cache=on. Non-legacy values with more consistent data can be set at the CPU model table. The current CPUID data is: #define L2_SIZE_KB_AMD 512 #define L2_ASSOCIATIVITY 16 #define L2_LINES_PER_TAG 1 #define L2_LINE_SIZE 64 [...] case 0x80000006: *ecx = (L2_SIZE_KB_AMD << 16) | \ (AMD_ENC_ASSOC(L2_ASSOCIATIVITY) << 12) | \ (L2_LINES_PER_TAG << 8) | (L2_LINE_SIZE); This means l2_cache_amd.size must be 512KiB, l2_cache_amd.associativity must be 16, l2_cache_amd.lines_per_tag must be 1, and l2_cache_amd.line_size must be 64. l2_cache_amd.partitions and l2_cache_amd.sets, on the other hand, can be set to any value you wish. Another option is to simply disable CPUID[0x8000001d] if legacy-cache=on, so we don't need to worry about consistency on legacy cache entries that are known to be inconsistent. If we want to be extra safe/consistent, we can automatically set legacy-cache=off if topoext is enabled, and prevent QEMU from starting if topoext is enabled on a CPU without X86CPUDefinition.cache_info. We have lots of freedom on what to do when topoext is enabled or when using new machine-types. We just can't change the CPUID data of old machine-types when topoext is disabled. > > > > > > > > > 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) > > > > > > If we want to change the associativity, we must keep the old > > > values on older machine-types, which was the whole purpose of the > > > "legacy-cache" property. > > > > > > I suggest using the new X86CPUDefinition::cache_info field if you > > > want to make AMD CPUs report different associativity. > > > > Ok. Sure. I will change it. Thanks > > > > > > > > -- > > > Eduardo
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Tuesday, May 8, 2018 2:08 PM > To: Moger, Babu <Babu.Moger@amd.com> > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > information consistently > > On Tue, May 08, 2018 at 06:40:23PM +0000, Moger, Babu wrote: > > Hi Eduardo, One more comment below. > > > > > -----Original Message----- > > > From: kvm-owner@vger.kernel.org [mailto:kvm- > owner@vger.kernel.org] > > > On Behalf Of Moger, Babu > > > Sent: Monday, May 7, 2018 5:48 PM > > > To: Eduardo Habkost <ehabkost@redhat.com> > > > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > > > kash@tripleback.net; mtosatti@redhat.com; qemu-devel@nongnu.org; > > > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > > > Subject: RE: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode cache > > > information consistently > > > > > > > > > > > > > -----Original Message----- > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Sent: Monday, May 7, 2018 4:27 PM > > > > To: Moger, Babu <Babu.Moger@amd.com> > > > > Cc: geoff@hostfission.com; kvm@vger.kernel.org; mst@redhat.com; > > > > kash@tripleback.net; mtosatti@redhat.com; qemu- > devel@nongnu.org; > > > > marcel@redhat.com; pbonzini@redhat.com; rth@twiddle.net > > > > Subject: Re: [Qemu-devel] [PATCH v7 1/9] i386: Helpers to encode > cache > > > > information consistently > > > > > > > > On Mon, May 07, 2018 at 09:14:27PM +0000, Moger, Babu wrote: > > > > > Eduardo, > > > > > Thanks for all the comments. Will respond to each one separately. > > > > > > > > > > > -----Original Message----- > > > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > > > Sent: Monday, May 7, 2018 2:05 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 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@redhat.com> > > > > > > > > > > > > > > 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@redhat.com> > > > > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > > > > > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > > > > > [...] > > > > > > > -#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? > > > > Keeping this to 16 will be a problem. It breaks the size rule(line size > *associativity * partitions * sets). > > It asserts violating that rule. Now I remember. That is why I changed it to 8. > I would think it would be > > better to fix it now. > > You can fix it, no problem. You just need to make sure > CPUID[0x80000006] data for old machine-types without topoext will > stay the same. > > Remember that l2_cache_amd is just for the legacy values enabled > by legacy-cache=on. Non-legacy values with more consistent data > can be set at the CPU model table. > > The current CPUID data is: > > #define L2_SIZE_KB_AMD 512 > #define L2_ASSOCIATIVITY 16 > #define L2_LINES_PER_TAG 1 > #define L2_LINE_SIZE 64 > [...] > case 0x80000006: > *ecx = (L2_SIZE_KB_AMD << 16) | \ > (AMD_ENC_ASSOC(L2_ASSOCIATIVITY) << 12) | \ > (L2_LINES_PER_TAG << 8) | (L2_LINE_SIZE); > > > This means l2_cache_amd.size must be 512KiB, > l2_cache_amd.associativity must be 16, l2_cache_amd.lines_per_tag > must be 1, and l2_cache_amd.line_size must be 64. > > l2_cache_amd.partitions and l2_cache_amd.sets, on the other hand, > can be set to any value you wish. Ok. Let me adjust sets and partitions. Thanks > > Another option is to simply disable CPUID[0x8000001d] if > legacy-cache=on, so we don't need to worry about consistency on > legacy cache entries that are known to be inconsistent. > > If we want to be extra safe/consistent, we can automatically set > legacy-cache=off if topoext is enabled, and prevent QEMU from > starting if topoext is enabled on a CPU without > X86CPUDefinition.cache_info. > > We have lots of freedom on what to do when topoext is enabled or > when using new machine-types. We just can't change the CPUID > data of old machine-types when topoext is disabled. > > > > > > > > > > > > > > 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) > > > > > > > > If we want to change the associativity, we must keep the old > > > > values on older machine-types, which was the whole purpose of the > > > > "legacy-cache" property. > > > > > > > > I suggest using the new X86CPUDefinition::cache_info field if you > > > > want to make AMD CPUs report different associativity. > > > > > > Ok. Sure. I will change it. Thanks > > > > > > > > > > > -- > > > > Eduardo > > -- > Eduardo
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index a20fe26..b6c1592 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -56,33 +56,240 @@ #include "disas/capstone.h" +/* Helpers for building CPUID[2] descriptors: */ + +struct CPUID2CacheDescriptorInfo { + enum CacheType type; + int level; + int size; + int line_size; + int associativity; +}; -/* Cache topology CPUID constants: */ +#define KiB 1024 +#define MiB (1024 * 1024) -/* CPUID Leaf 2 Descriptors */ +/* + * Known CPUID 2 cache descriptors. + * From Intel SDM Volume 2A, CPUID instruction + */ +struct CPUID2CacheDescriptorInfo cpuid2_cache_descriptors[] = { + [0x06] = { .level = 1, .type = ICACHE, .size = 8 * KiB, + .associativity = 4, .line_size = 32, }, + [0x08] = { .level = 1, .type = ICACHE, .size = 16 * KiB, + .associativity = 4, .line_size = 32, }, + [0x09] = { .level = 1, .type = ICACHE, .size = 32 * KiB, + .associativity = 4, .line_size = 64, }, + [0x0A] = { .level = 1, .type = DCACHE, .size = 8 * KiB, + .associativity = 2, .line_size = 32, }, + [0x0C] = { .level = 1, .type = DCACHE, .size = 16 * KiB, + .associativity = 4, .line_size = 32, }, + [0x0D] = { .level = 1, .type = DCACHE, .size = 16 * KiB, + .associativity = 4, .line_size = 64, }, + [0x0E] = { .level = 1, .type = DCACHE, .size = 24 * KiB, + .associativity = 6, .line_size = 64, }, + [0x1D] = { .level = 2, .type = UNIFIED_CACHE, .size = 128 * KiB, + .associativity = 2, .line_size = 64, }, + [0x21] = { .level = 2, .type = UNIFIED_CACHE, .size = 256 * KiB, + .associativity = 8, .line_size = 64, }, + /* lines per sector is not supported cpuid2_cache_descriptor(), + * so descriptors 0x22, 0x23 are not included + */ + [0x24] = { .level = 2, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 16, .line_size = 64, }, + /* lines per sector is not supported cpuid2_cache_descriptor(), + * so descriptors 0x25, 0x20 are not included + */ + [0x2C] = { .level = 1, .type = DCACHE, .size = 32 * KiB, + .associativity = 8, .line_size = 64, }, + [0x30] = { .level = 1, .type = ICACHE, .size = 32 * KiB, + .associativity = 8, .line_size = 64, }, + [0x41] = { .level = 2, .type = UNIFIED_CACHE, .size = 128 * KiB, + .associativity = 4, .line_size = 32, }, + [0x42] = { .level = 2, .type = UNIFIED_CACHE, .size = 256 * KiB, + .associativity = 4, .line_size = 32, }, + [0x43] = { .level = 2, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 4, .line_size = 32, }, + [0x44] = { .level = 2, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 4, .line_size = 32, }, + [0x45] = { .level = 2, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 4, .line_size = 32, }, + [0x46] = { .level = 3, .type = UNIFIED_CACHE, .size = 4 * MiB, + .associativity = 4, .line_size = 64, }, + [0x47] = { .level = 3, .type = UNIFIED_CACHE, .size = 8 * MiB, + .associativity = 8, .line_size = 64, }, + [0x48] = { .level = 2, .type = UNIFIED_CACHE, .size = 3 * MiB, + .associativity = 12, .line_size = 64, }, + /* Descriptor 0x49 depends on CPU family/model, so it is not included */ + [0x4A] = { .level = 3, .type = UNIFIED_CACHE, .size = 6 * MiB, + .associativity = 12, .line_size = 64, }, + [0x4B] = { .level = 3, .type = UNIFIED_CACHE, .size = 8 * MiB, + .associativity = 16, .line_size = 64, }, + [0x4C] = { .level = 3, .type = UNIFIED_CACHE, .size = 12 * MiB, + .associativity = 12, .line_size = 64, }, + [0x4D] = { .level = 3, .type = UNIFIED_CACHE, .size = 16 * MiB, + .associativity = 16, .line_size = 64, }, + [0x4E] = { .level = 2, .type = UNIFIED_CACHE, .size = 6 * MiB, + .associativity = 24, .line_size = 64, }, + [0x60] = { .level = 1, .type = DCACHE, .size = 16 * KiB, + .associativity = 8, .line_size = 64, }, + [0x66] = { .level = 1, .type = DCACHE, .size = 8 * KiB, + .associativity = 4, .line_size = 64, }, + [0x67] = { .level = 1, .type = DCACHE, .size = 16 * KiB, + .associativity = 4, .line_size = 64, }, + [0x68] = { .level = 1, .type = DCACHE, .size = 32 * KiB, + .associativity = 4, .line_size = 64, }, + [0x78] = { .level = 2, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 4, .line_size = 64, }, + /* lines per sector is not supported cpuid2_cache_descriptor(), + * so descriptors 0x79, 0x7A, 0x7B, 0x7C are not included. + */ + [0x7D] = { .level = 2, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 8, .line_size = 64, }, + [0x7F] = { .level = 2, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 2, .line_size = 64, }, + [0x80] = { .level = 2, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 8, .line_size = 64, }, + [0x82] = { .level = 2, .type = UNIFIED_CACHE, .size = 256 * KiB, + .associativity = 8, .line_size = 32, }, + [0x83] = { .level = 2, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 8, .line_size = 32, }, + [0x84] = { .level = 2, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 8, .line_size = 32, }, + [0x85] = { .level = 2, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 8, .line_size = 32, }, + [0x86] = { .level = 2, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 4, .line_size = 64, }, + [0x87] = { .level = 2, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 8, .line_size = 64, }, + [0xD0] = { .level = 3, .type = UNIFIED_CACHE, .size = 512 * KiB, + .associativity = 4, .line_size = 64, }, + [0xD1] = { .level = 3, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 4, .line_size = 64, }, + [0xD2] = { .level = 3, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 4, .line_size = 64, }, + [0xD6] = { .level = 3, .type = UNIFIED_CACHE, .size = 1 * MiB, + .associativity = 8, .line_size = 64, }, + [0xD7] = { .level = 3, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 8, .line_size = 64, }, + [0xD8] = { .level = 3, .type = UNIFIED_CACHE, .size = 4 * MiB, + .associativity = 8, .line_size = 64, }, + [0xDC] = { .level = 3, .type = UNIFIED_CACHE, .size = 1.5 * MiB, + .associativity = 12, .line_size = 64, }, + [0xDD] = { .level = 3, .type = UNIFIED_CACHE, .size = 3 * MiB, + .associativity = 12, .line_size = 64, }, + [0xDE] = { .level = 3, .type = UNIFIED_CACHE, .size = 6 * MiB, + .associativity = 12, .line_size = 64, }, + [0xE2] = { .level = 3, .type = UNIFIED_CACHE, .size = 2 * MiB, + .associativity = 16, .line_size = 64, }, + [0xE3] = { .level = 3, .type = UNIFIED_CACHE, .size = 4 * MiB, + .associativity = 16, .line_size = 64, }, + [0xE4] = { .level = 3, .type = UNIFIED_CACHE, .size = 8 * MiB, + .associativity = 16, .line_size = 64, }, + [0xEA] = { .level = 3, .type = UNIFIED_CACHE, .size = 12 * MiB, + .associativity = 24, .line_size = 64, }, + [0xEB] = { .level = 3, .type = UNIFIED_CACHE, .size = 18 * MiB, + .associativity = 24, .line_size = 64, }, + [0xEC] = { .level = 3, .type = UNIFIED_CACHE, .size = 24 * MiB, + .associativity = 24, .line_size = 64, }, +}; + +/* + * "CPUID leaf 2 does not report cache descriptor information, + * use CPUID leaf 4 to query cache parameters" + */ +#define CACHE_DESCRIPTOR_UNAVAILABLE 0xFF -#define CPUID_2_L1D_32KB_8WAY_64B 0x2c -#define CPUID_2_L1I_32KB_8WAY_64B 0x30 -#define CPUID_2_L2_2MB_8WAY_64B 0x7d -#define CPUID_2_L3_16MB_16WAY_64B 0x4d +/* + * Return a CPUID 2 cache descriptor for a given cache. + * If no known descriptor is found, return CACHE_DESCRIPTOR_UNAVAILABLE + */ +static uint8_t cpuid2_cache_descriptor(CPUCacheInfo *cache) +{ + int i; + + assert(cache->size > 0); + assert(cache->level > 0); + assert(cache->line_size > 0); + assert(cache->associativity > 0); + for (i = 0; i < ARRAY_SIZE(cpuid2_cache_descriptors); i++) { + struct CPUID2CacheDescriptorInfo *d = &cpuid2_cache_descriptors[i]; + if (d->level == cache->level && d->type == cache->type && + d->size == cache->size && d->line_size == cache->line_size && + d->associativity == cache->associativity) { + return i; + } + } + return CACHE_DESCRIPTOR_UNAVAILABLE; +} /* CPUID Leaf 4 constants: */ /* EAX: */ -#define CPUID_4_TYPE_DCACHE 1 -#define CPUID_4_TYPE_ICACHE 2 -#define CPUID_4_TYPE_UNIFIED 3 +#define CACHE_TYPE_D 1 +#define CACHE_TYPE_I 2 +#define CACHE_TYPE_UNIFIED 3 -#define CPUID_4_LEVEL(l) ((l) << 5) +#define CACHE_LEVEL(l) (l << 5) -#define CPUID_4_SELF_INIT_LEVEL (1 << 8) -#define CPUID_4_FULLY_ASSOC (1 << 9) +#define CACHE_SELF_INIT_LEVEL (1 << 8) /* EDX: */ -#define CPUID_4_NO_INVD_SHARING (1 << 0) -#define CPUID_4_INCLUSIVE (1 << 1) -#define CPUID_4_COMPLEX_IDX (1 << 2) +#define CACHE_NO_INVD_SHARING (1 << 0) +#define CACHE_INCLUSIVE (1 << 1) +#define CACHE_COMPLEX_IDX (1 << 2) + +/* Encode CacheType for CPUID[4].EAX */ +#define CACHE_TYPE(t) (((t) == DCACHE) ? CACHE_TYPE_D : \ + ((t) == ICACHE) ? CACHE_TYPE_I : \ + ((t) == UNIFIED_CACHE) ? CACHE_TYPE_UNIFIED : \ + 0 /* Invalid value */) + + +/* Encode cache info for CPUID[4] */ +static void encode_cache_cpuid4(CPUCacheInfo *cache, + int num_apic_ids, int num_cores, + uint32_t *eax, uint32_t *ebx, + uint32_t *ecx, uint32_t *edx) +{ + assert(cache->size == cache->line_size * cache->associativity * + cache->partitions * cache->sets); + + assert(num_apic_ids > 0); + *eax = CACHE_TYPE(cache->type) | + CACHE_LEVEL(cache->level) | + (cache->self_init ? CACHE_SELF_INIT_LEVEL : 0) | + ((num_cores - 1) << 26) | + ((num_apic_ids - 1) << 14); + + assert(cache->line_size > 0); + assert(cache->partitions > 0); + assert(cache->associativity > 0); + /* We don't implement fully-associative caches */ + assert(cache->associativity < cache->sets); + *ebx = (cache->line_size - 1) | + ((cache->partitions - 1) << 12) | + ((cache->associativity - 1) << 22); + + assert(cache->sets > 0); + *ecx = cache->sets - 1; + + *edx = (cache->no_invd_sharing ? CACHE_NO_INVD_SHARING : 0) | + (cache->inclusive ? CACHE_INCLUSIVE : 0) | + (cache->complex_indexing ? CACHE_COMPLEX_IDX : 0); +} + +/* Encode cache info for CPUID[0x80000005].ECX or CPUID[0x80000005].EDX */ +static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache) +{ + assert(cache->size % 1024 == 0); + assert(cache->lines_per_tag > 0); + assert(cache->associativity > 0); + assert(cache->line_size > 0); + return ((cache->size / 1024) << 24) | (cache->associativity << 16) | + (cache->lines_per_tag << 8) | (cache->line_size); +} #define ASSOC_FULL 0xFF @@ -100,57 +307,140 @@ a == ASSOC_FULL ? 0xF : \ 0 /* invalid value */) +/* + * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX + * @l3 can be NULL. + */ +static void encode_cache_cpuid80000006(CPUCacheInfo *l2, + CPUCacheInfo *l3, + uint32_t *ecx, uint32_t *edx) +{ + assert(l2->size % 1024 == 0); + assert(l2->associativity > 0); + assert(l2->lines_per_tag > 0); + assert(l2->line_size > 0); + *ecx = ((l2->size / 1024) << 16) | + (AMD_ENC_ASSOC(l2->associativity) << 12) | + (l2->lines_per_tag << 8) | (l2->line_size); + + if (l3) { + assert(l3->size % (512 * 1024) == 0); + assert(l3->associativity > 0); + assert(l3->lines_per_tag > 0); + assert(l3->line_size > 0); + *edx = ((l3->size / (512 * 1024)) << 18) | + (AMD_ENC_ASSOC(l3->associativity) << 12) | + (l3->lines_per_tag << 8) | (l3->line_size); + } else { + *edx = 0; + } +} /* Definitions of the hardcoded cache entries we expose: */ /* L1 data cache: */ -#define L1D_LINE_SIZE 64 -#define L1D_ASSOCIATIVITY 8 -#define L1D_SETS 64 -#define L1D_PARTITIONS 1 -/* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */ -#define L1D_DESCRIPTOR CPUID_2_L1D_32KB_8WAY_64B +static CPUCacheInfo l1d_cache = { + .type = DCACHE, + .level = 1, + .size = 32 * KiB, + .self_init = 1, + .line_size = 64, + .associativity = 8, + .sets = 64, + .partitions = 1, + .no_invd_sharing = true, +}; + /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */ -#define L1D_LINES_PER_TAG 1 -#define L1D_SIZE_KB_AMD 64 -#define L1D_ASSOCIATIVITY_AMD 2 +static CPUCacheInfo l1d_cache_amd = { + .type = DCACHE, + .level = 1, + .size = 64 * KiB, + .self_init = 1, + .line_size = 64, + .associativity = 2, + .sets = 512, + .partitions = 1, + .lines_per_tag = 1, + .no_invd_sharing = true, +}; /* L1 instruction cache: */ -#define L1I_LINE_SIZE 64 -#define L1I_ASSOCIATIVITY 8 -#define L1I_SETS 64 -#define L1I_PARTITIONS 1 -/* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 32KiB */ -#define L1I_DESCRIPTOR CPUID_2_L1I_32KB_8WAY_64B +static CPUCacheInfo l1i_cache = { + .type = ICACHE, + .level = 1, + .size = 32 * KiB, + .self_init = 1, + .line_size = 64, + .associativity = 8, + .sets = 64, + .partitions = 1, + .no_invd_sharing = true, +}; + /*FIXME: CPUID leaf 0x80000005 is inconsistent with leaves 2 & 4 */ -#define L1I_LINES_PER_TAG 1 -#define L1I_SIZE_KB_AMD 64 -#define L1I_ASSOCIATIVITY_AMD 2 +static CPUCacheInfo l1i_cache_amd = { + .type = ICACHE, + .level = 1, + .size = 64 * KiB, + .self_init = 1, + .line_size = 64, + .associativity = 2, + .sets = 512, + .partitions = 1, + .lines_per_tag = 1, + .no_invd_sharing = true, +}; /* Level 2 unified cache: */ -#define L2_LINE_SIZE 64 -#define L2_ASSOCIATIVITY 16 -#define L2_SETS 4096 -#define L2_PARTITIONS 1 -/* Size = LINE_SIZE*ASSOCIATIVITY*SETS*PARTITIONS = 4MiB */ +static CPUCacheInfo l2_cache = { + .type = UNIFIED_CACHE, + .level = 2, + .size = 4 * MiB, + .self_init = 1, + .line_size = 64, + .associativity = 16, + .sets = 4096, + .partitions = 1, + .no_invd_sharing = true, +}; + /*FIXME: CPUID leaf 2 descriptor is inconsistent with CPUID leaf 4 */ -#define L2_DESCRIPTOR CPUID_2_L2_2MB_8WAY_64B +static CPUCacheInfo l2_cache_cpuid2 = { + .type = UNIFIED_CACHE, + .level = 2, + .size = 2 * MiB, + .line_size = 64, + .associativity = 8, +}; + + /*FIXME: CPUID leaf 0x80000006 is inconsistent with leaves 2 & 4 */ -#define L2_LINES_PER_TAG 1 -#define L2_SIZE_KB_AMD 512 +static CPUCacheInfo l2_cache_amd = { + .type = UNIFIED_CACHE, + .level = 2, + .size = 512 * KiB, + .line_size = 64, + .lines_per_tag = 1, + .associativity = 8, + .sets = 1024, + .partitions = 1, +}; /* Level 3 unified cache: */ -#define L3_SIZE_KB 0 /* disabled */ -#define L3_ASSOCIATIVITY 0 /* disabled */ -#define L3_LINES_PER_TAG 0 /* disabled */ -#define L3_LINE_SIZE 0 /* disabled */ -#define L3_N_LINE_SIZE 64 -#define L3_N_ASSOCIATIVITY 16 -#define L3_N_SETS 16384 -#define L3_N_PARTITIONS 1 -#define L3_N_DESCRIPTOR CPUID_2_L3_16MB_16WAY_64B -#define L3_N_LINES_PER_TAG 1 -#define L3_N_SIZE_KB_AMD 16384 +static CPUCacheInfo l3_cache = { + .type = UNIFIED_CACHE, + .level = 3, + .size = 16 * MiB, + .line_size = 64, + .associativity = 16, + .sets = 16384, + .partitions = 1, + .lines_per_tag = 1, + .self_init = true, + .inclusive = true, + .complex_indexing = true, +}; /* TLB definitions: */ @@ -3301,85 +3591,53 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, if (!cpu->enable_l3_cache) { *ecx = 0; } else { - *ecx = L3_N_DESCRIPTOR; + *ecx = cpuid2_cache_descriptor(&l3_cache); } - *edx = (L1D_DESCRIPTOR << 16) | \ - (L1I_DESCRIPTOR << 8) | \ - (L2_DESCRIPTOR); + *edx = (cpuid2_cache_descriptor(&l1d_cache) << 16) | + (cpuid2_cache_descriptor(&l1i_cache) << 8) | + (cpuid2_cache_descriptor(&l2_cache_cpuid2)); break; case 4: /* cache info: needed for Core compatibility */ if (cpu->cache_info_passthrough) { host_cpuid(index, count, eax, ebx, ecx, edx); + /* QEMU gives out its own APIC IDs, never pass down bits 31..26. */ *eax &= ~0xFC000000; + if ((*eax & 31) && cs->nr_cores > 1) { + *eax |= (cs->nr_cores - 1) << 26; + } } else { *eax = 0; switch (count) { case 0: /* L1 dcache info */ - *eax |= CPUID_4_TYPE_DCACHE | \ - CPUID_4_LEVEL(1) | \ - CPUID_4_SELF_INIT_LEVEL; - *ebx = (L1D_LINE_SIZE - 1) | \ - ((L1D_PARTITIONS - 1) << 12) | \ - ((L1D_ASSOCIATIVITY - 1) << 22); - *ecx = L1D_SETS - 1; - *edx = CPUID_4_NO_INVD_SHARING; + encode_cache_cpuid4(&l1d_cache, + 1, cs->nr_cores, + eax, ebx, ecx, edx); break; case 1: /* L1 icache info */ - *eax |= CPUID_4_TYPE_ICACHE | \ - CPUID_4_LEVEL(1) | \ - CPUID_4_SELF_INIT_LEVEL; - *ebx = (L1I_LINE_SIZE - 1) | \ - ((L1I_PARTITIONS - 1) << 12) | \ - ((L1I_ASSOCIATIVITY - 1) << 22); - *ecx = L1I_SETS - 1; - *edx = CPUID_4_NO_INVD_SHARING; + encode_cache_cpuid4(&l1i_cache, + 1, cs->nr_cores, + eax, ebx, ecx, edx); break; case 2: /* L2 cache info */ - *eax |= CPUID_4_TYPE_UNIFIED | \ - CPUID_4_LEVEL(2) | \ - CPUID_4_SELF_INIT_LEVEL; - if (cs->nr_threads > 1) { - *eax |= (cs->nr_threads - 1) << 14; - } - *ebx = (L2_LINE_SIZE - 1) | \ - ((L2_PARTITIONS - 1) << 12) | \ - ((L2_ASSOCIATIVITY - 1) << 22); - *ecx = L2_SETS - 1; - *edx = CPUID_4_NO_INVD_SHARING; + encode_cache_cpuid4(&l2_cache, + cs->nr_threads, cs->nr_cores, + eax, ebx, ecx, edx); break; case 3: /* L3 cache info */ - if (!cpu->enable_l3_cache) { - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads); + if (cpu->enable_l3_cache) { + encode_cache_cpuid4(&l3_cache, + (1 << pkg_offset), cs->nr_cores, + eax, ebx, ecx, edx); break; } - *eax |= CPUID_4_TYPE_UNIFIED | \ - CPUID_4_LEVEL(3) | \ - CPUID_4_SELF_INIT_LEVEL; - pkg_offset = apicid_pkg_offset(cs->nr_cores, cs->nr_threads); - *eax |= ((1 << pkg_offset) - 1) << 14; - *ebx = (L3_N_LINE_SIZE - 1) | \ - ((L3_N_PARTITIONS - 1) << 12) | \ - ((L3_N_ASSOCIATIVITY - 1) << 22); - *ecx = L3_N_SETS - 1; - *edx = CPUID_4_INCLUSIVE | CPUID_4_COMPLEX_IDX; - break; + /* fall through */ default: /* end of info */ - *eax = 0; - *ebx = 0; - *ecx = 0; - *edx = 0; + *eax = *ebx = *ecx = *edx = 0; break; } } - - /* QEMU gives out its own APIC IDs, never pass down bits 31..26. */ - if ((*eax & 31) && cs->nr_cores > 1) { - *eax |= (cs->nr_cores - 1) << 26; - } break; case 5: /* mwait info: needed for Core compatibility */ @@ -3583,10 +3841,8 @@ 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 = (L1D_SIZE_KB_AMD << 24) | (L1D_ASSOCIATIVITY_AMD << 16) | \ - (L1D_LINES_PER_TAG << 8) | (L1D_LINE_SIZE); - *edx = (L1I_SIZE_KB_AMD << 24) | (L1I_ASSOCIATIVITY_AMD << 16) | \ - (L1I_LINES_PER_TAG << 8) | (L1I_LINE_SIZE); + *ecx = encode_cache_cpuid80000005(&l1d_cache_amd); + *edx = encode_cache_cpuid80000005(&l1i_cache_amd); break; case 0x80000006: /* cache info (L2 cache) */ @@ -3602,18 +3858,9 @@ 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); - *ecx = (L2_SIZE_KB_AMD << 16) | \ - (AMD_ENC_ASSOC(L2_ASSOCIATIVITY) << 12) | \ - (L2_LINES_PER_TAG << 8) | (L2_LINE_SIZE); - if (!cpu->enable_l3_cache) { - *edx = ((L3_SIZE_KB / 512) << 18) | \ - (AMD_ENC_ASSOC(L3_ASSOCIATIVITY) << 12) | \ - (L3_LINES_PER_TAG << 8) | (L3_LINE_SIZE); - } else { - *edx = ((L3_N_SIZE_KB_AMD / 512) << 18) | \ - (AMD_ENC_ASSOC(L3_N_ASSOCIATIVITY) << 12) | \ - (L3_N_LINES_PER_TAG << 8) | (L3_N_LINE_SIZE); - } + encode_cache_cpuid80000006(&l2_cache_amd, + cpu->enable_l3_cache ? &l3_cache : NULL, + ecx, edx); break; case 0x80000007: *eax = 0; diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 1b219fa..fa03e2c 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1044,6 +1044,59 @@ typedef enum TPRAccess { TPR_ACCESS_WRITE, } TPRAccess; +/* Cache information data structures: */ + +enum CacheType { + DCACHE, + ICACHE, + UNIFIED_CACHE +}; + +typedef struct CPUCacheInfo { + enum CacheType type; + uint8_t level; + /* Size in bytes */ + uint32_t size; + /* Line size, in bytes */ + uint16_t line_size; + /* + * Associativity. + * Note: representation of fully-associative caches is not implemented + */ + uint8_t associativity; + /* Physical line partitions. CPUID[0x8000001D].EBX, CPUID[4].EBX */ + uint8_t partitions; + /* Number of sets. CPUID[0x8000001D].ECX, CPUID[4].ECX */ + uint32_t sets; + /* + * Lines per tag. + * AMD-specific: CPUID[0x80000005], CPUID[0x80000006]. + * (Is this synonym to @partitions?) + */ + uint8_t lines_per_tag; + + /* Self-initializing cache */ + bool self_init; + /* + * WBINVD/INVD is not guaranteed to act upon lower level caches of + * non-originating threads sharing this cache. + * CPUID[4].EDX[bit 0], CPUID[0x8000001D].EDX[bit 0] + */ + bool no_invd_sharing; + /* + * Cache is inclusive of lower cache levels. + * CPUID[4].EDX[bit 1], CPUID[0x8000001D].EDX[bit 1]. + */ + bool inclusive; + /* + * A complex function is used to index the cache, potentially using all + * address bits. CPUID[4].EDX[bit 2]. + */ + bool complex_indexing; +} CPUCacheInfo; + + + typedef struct CPUX86State { /* standard registers */ target_ulong regs[CPU_NB_REGS];