| Message ID | 1524760009-24710-8-git-send-email-babu.moger@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 26, 2018 at 11:26:47AM -0500, Babu Moger wrote: > Populate threads/core_id/apic_ids/socket_id when CPUID_EXT3_TOPOEXT > feature is supported. This is required to support hyperthreading feature > on AMD CPUs. This is supported via CPUID_8000_001E extended functions. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > Tested-by: Geoffrey McRae <geoff@hostfission.com> > --- > target/i386/cpu.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 1024b09..1b15023 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -315,6 +315,12 @@ static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache) > (((CORES_IN_CMPLX - 1) * 2) + 1) : \ > (CORES_IN_CMPLX - 1)) > > +/* Definitions used on CPUID Leaf 0x8000001E */ > +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ > + ((threads) ? \ > + ((socket_id << 6) | (core_id << 1) | thread_id) : \ > + ((socket_id << 6) | core_id)) I suggest moving this to i386/topology.h. > + > /* > * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX > * @l3 can be NULL. > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > break; > } > break; > + case 0x8000001E: > + assert(cpu->core_id <= 255); Where's the code that ensures this assert() line can't be triggered by any command-line configuration? > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > + cpu->socket_id, cpu->core_id, cpu->thread_id); > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > + *ecx = cpu->socket_id; > + *edx = 0; > + break; > case 0xC0000000: > *eax = env->cpuid_xlevel2; > *ebx = 0; > -- > 2.7.4 > >
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Monday, May 7, 2018 2:40 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 7/9] i386: Add support for > CPUID_8000_001E for AMD > > On Thu, Apr 26, 2018 at 11:26:47AM -0500, Babu Moger wrote: > > Populate threads/core_id/apic_ids/socket_id when CPUID_EXT3_TOPOEXT > > feature is supported. This is required to support hyperthreading feature > > on AMD CPUs. This is supported via CPUID_8000_001E extended functions. > > > > Signed-off-by: Babu Moger <babu.moger@amd.com> > > Tested-by: Geoffrey McRae <geoff@hostfission.com> > > --- > > target/i386/cpu.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index 1024b09..1b15023 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -315,6 +315,12 @@ static uint32_t > encode_cache_cpuid80000005(CPUCacheInfo *cache) > > (((CORES_IN_CMPLX - 1) * 2) + 1) : \ > > (CORES_IN_CMPLX - 1)) > > > > +/* Definitions used on CPUID Leaf 0x8000001E */ > > +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ > > + ((threads) ? \ > > + ((socket_id << 6) | (core_id << 1) | thread_id) : \ > > + ((socket_id << 6) | core_id)) > > I suggest moving this to i386/topology.h. Ok. Will do that. > > > > + > > /* > > * Encode cache info for CPUID[0x80000006].ECX and > CPUID[0x80000006].EDX > > * @l3 can be NULL. > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > uint32_t index, uint32_t count, > > break; > > } > > break; > > + case 0x8000001E: > > + assert(cpu->core_id <= 255); > > Where's the code that ensures this assert() line can't be > triggered by any command-line configuration? I did not understand this. Can you please elaborate. Thanks > > > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > > + cpu->socket_id, cpu->core_id, cpu->thread_id); > > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > > + *ecx = cpu->socket_id; > > + *edx = 0; > > + break; > > case 0xC0000000: > > *eax = env->cpuid_xlevel2; > > *ebx = 0; > > -- > > 2.7.4 > > > > > > -- > Eduardo
On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote: [...] > > > + > > > /* > > > * Encode cache info for CPUID[0x80000006].ECX and > > CPUID[0x80000006].EDX > > > * @l3 can be NULL. > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > > uint32_t index, uint32_t count, > > > break; > > > } > > > break; > > > + case 0x8000001E: > > > + assert(cpu->core_id <= 255); > > > > Where's the code that ensures this assert() line can't be > > triggered by any command-line configuration? > > I did not understand this. Can you please elaborate. Thanks The user must not be able to trigger an assert(), so we need to ensure that core_id will never be larger than 255. Is there existing code that ensures that? > > > > > > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > > > + cpu->socket_id, cpu->core_id, cpu->thread_id); > > > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > > > + *ecx = cpu->socket_id; > > > + *edx = 0; > > > + break; > > > case 0xC0000000: > > > *eax = env->cpuid_xlevel2; > > > *ebx = 0; > > > -- > > > 2.7.4 > > > > > > > > > > -- > > Eduardo
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Tuesday, May 8, 2018 9:17 AM > 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 7/9] i386: Add support for > CPUID_8000_001E for AMD > > On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote: > [...] > > > > + > > > > /* > > > > * Encode cache info for CPUID[0x80000006].ECX and > > > CPUID[0x80000006].EDX > > > > * @l3 can be NULL. > > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > > > uint32_t index, uint32_t count, > > > > break; > > > > } > > > > break; > > > > + case 0x8000001E: > > > > + assert(cpu->core_id <= 255); > > > > > > Where's the code that ensures this assert() line can't be > > > triggered by any command-line configuration? > > > > I did not understand this. Can you please elaborate. Thanks > > The user must not be able to trigger an assert(), so we need to > ensure that core_id will never be larger than 255. Is there > existing code that ensures that? I see that max_cpus is set to 255 and also there are checks to make sure core_id does not go above 255. I verified it while testing. So, probably we don't need assert here. Radim asked me to add this assert. I can remove it if no abjections. > > > > > > > > > > + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), > > > > + cpu->socket_id, cpu->core_id, cpu->thread_id); > > > > + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; > > > > + *ecx = cpu->socket_id; > > > > + *edx = 0; > > > > + break; > > > > case 0xC0000000: > > > > *eax = env->cpuid_xlevel2; > > > > *ebx = 0; > > > > -- > > > > 2.7.4 > > > > > > > > > > > > > > -- > > > Eduardo > > -- > Eduardo
On Tue, May 08, 2018 at 03:02:07PM +0000, Moger, Babu wrote: > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Tuesday, May 8, 2018 9:17 AM > > 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 7/9] i386: Add support for > > CPUID_8000_001E for AMD > > > > On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote: > > [...] > > > > > + > > > > > /* > > > > > * Encode cache info for CPUID[0x80000006].ECX and > > > > CPUID[0x80000006].EDX > > > > > * @l3 can be NULL. > > > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > > > > uint32_t index, uint32_t count, > > > > > break; > > > > > } > > > > > break; > > > > > + case 0x8000001E: > > > > > + assert(cpu->core_id <= 255); > > > > > > > > Where's the code that ensures this assert() line can't be > > > > triggered by any command-line configuration? > > > > > > I did not understand this. Can you please elaborate. Thanks > > > > The user must not be able to trigger an assert(), so we need to > > ensure that core_id will never be larger than 255. Is there > > existing code that ensures that? > > I see that max_cpus is set to 255 and also there are checks to make sure core_id does not go above 255. > I verified it while testing. So, probably we don't need assert here. Radim asked me to add this assert. > I can remove it if no abjections. Sorry for not replying to this before: no objection to the assert(), especially considering it will trigger very early on initialization if we break it one day.
> -----Original Message----- > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > Sent: Friday, May 11, 2018 9:12 AM > 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 7/9] i386: Add support for > CPUID_8000_001E for AMD > > On Tue, May 08, 2018 at 03:02:07PM +0000, Moger, Babu wrote: > > > > > -----Original Message----- > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > Sent: Tuesday, May 8, 2018 9:17 AM > > > 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 7/9] i386: Add support for > > > CPUID_8000_001E for AMD > > > > > > On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote: > > > [...] > > > > > > + > > > > > > /* > > > > > > * Encode cache info for CPUID[0x80000006].ECX and > > > > > CPUID[0x80000006].EDX > > > > > > * @l3 can be NULL. > > > > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > > > > > uint32_t index, uint32_t count, > > > > > > break; > > > > > > } > > > > > > break; > > > > > > + case 0x8000001E: > > > > > > + assert(cpu->core_id <= 255); > > > > > > > > > > Where's the code that ensures this assert() line can't be > > > > > triggered by any command-line configuration? > > > > > > > > I did not understand this. Can you please elaborate. Thanks > > > > > > The user must not be able to trigger an assert(), so we need to > > > ensure that core_id will never be larger than 255. Is there > > > existing code that ensures that? > > > > I see that max_cpus is set to 255 and also there are checks to make sure > core_id does not go above 255. > > I verified it while testing. So, probably we don't need assert here. Radim > asked me to add this assert. > > I can remove it if no abjections. > > Sorry for not replying to this before: no objection to the > assert(), especially considering it will trigger very early on > initialization if we break it one day. Ok. No problem. I will add it back and send a v9 version. Please let me know if you have any other feedback for v8 version(sent yesterday). > > -- > Eduardo
On Fri, May 11, 2018 at 02:44:11PM +0000, Moger, Babu wrote: > > > > -----Original Message----- > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > Sent: Friday, May 11, 2018 9:12 AM > > 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 7/9] i386: Add support for > > CPUID_8000_001E for AMD > > > > On Tue, May 08, 2018 at 03:02:07PM +0000, Moger, Babu wrote: > > > > > > > -----Original Message----- > > > > From: Eduardo Habkost [mailto:ehabkost@redhat.com] > > > > Sent: Tuesday, May 8, 2018 9:17 AM > > > > 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 7/9] i386: Add support for > > > > CPUID_8000_001E for AMD > > > > > > > > On Mon, May 07, 2018 at 11:44:31PM +0000, Moger, Babu wrote: > > > > [...] > > > > > > > + > > > > > > > /* > > > > > > > * Encode cache info for CPUID[0x80000006].ECX and > > > > > > CPUID[0x80000006].EDX > > > > > > > * @l3 can be NULL. > > > > > > > @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, > > > > > > uint32_t index, uint32_t count, > > > > > > > break; > > > > > > > } > > > > > > > break; > > > > > > > + case 0x8000001E: > > > > > > > + assert(cpu->core_id <= 255); > > > > > > > > > > > > Where's the code that ensures this assert() line can't be > > > > > > triggered by any command-line configuration? > > > > > > > > > > I did not understand this. Can you please elaborate. Thanks > > > > > > > > The user must not be able to trigger an assert(), so we need to > > > > ensure that core_id will never be larger than 255. Is there > > > > existing code that ensures that? > > > > > > I see that max_cpus is set to 255 and also there are checks to make sure > > core_id does not go above 255. > > > I verified it while testing. So, probably we don't need assert here. Radim > > asked me to add this assert. > > > I can remove it if no abjections. > > > > Sorry for not replying to this before: no objection to the > > assert(), especially considering it will trigger very early on > > initialization if we break it one day. > > Ok. No problem. I will add it back and send a v9 version. > Please let me know if you have any other feedback for v8 version(sent yesterday). Thanks, I just saw v8 on my mailbox. No need to send v9 just because of the assert(), I can re-add it while committing if necessary.
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 1024b09..1b15023 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -315,6 +315,12 @@ static uint32_t encode_cache_cpuid80000005(CPUCacheInfo *cache) (((CORES_IN_CMPLX - 1) * 2) + 1) : \ (CORES_IN_CMPLX - 1)) +/* Definitions used on CPUID Leaf 0x8000001E */ +#define EXTENDED_APIC_ID(threads, socket_id, core_id, thread_id) \ + ((threads) ? \ + ((socket_id << 6) | (core_id << 1) | thread_id) : \ + ((socket_id << 6) | core_id)) + /* * Encode cache info for CPUID[0x80000006].ECX and CPUID[0x80000006].EDX * @l3 can be NULL. @@ -4105,6 +4111,14 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; } break; + case 0x8000001E: + assert(cpu->core_id <= 255); + *eax = EXTENDED_APIC_ID((cs->nr_threads - 1), + cpu->socket_id, cpu->core_id, cpu->thread_id); + *ebx = (cs->nr_threads - 1) << 8 | cpu->core_id; + *ecx = cpu->socket_id; + *edx = 0; + break; case 0xC0000000: *eax = env->cpuid_xlevel2; *ebx = 0;