[PATCH] libxc: remove CPUID core information mangling

[PATCH] libxc: remove CPUID core information mangling

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

Hi,

c/s 18560:782599274bf9ae8857c55856c9c7fdf082967808 introduced CPUID 
mangling resulting in a doubled number of cores/processor exposed to the 
guest. According to comments in this patch the rationale behind this is 
to match the APIC numbering used by Xen.
In my understanding the CPUID leafs dealing with number of cores always 
talk about logical numbers and not APIC IDs. So we don't need to adjust 
the CPUID readout to match the APIC ID enumeration scheme.
If there were any serious reasons resulting in the old patch I'd love to 
hear them.

The attached patch fixes this and solves an issue I saw with certain 
NUMA guest configurations.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: fix_cpuid_core_mangling.patch --]
[-- Type: text/x-patch, Size: 1346 bytes --]

diff -r eccfdeb41b80 tools/libxc/xc_cpuid_x86.c
--- a/tools/libxc/xc_cpuid_x86.c	Tue Aug 24 18:42:59 2010 +0100
+++ b/tools/libxc/xc_cpuid_x86.c	Wed Aug 25 13:34:54 2010 +0200
@@ -117,9 +117,8 @@
     case 0x80000008:
         /*
          * ECX[15:12] is ApicIdCoreSize: ECX[7:0] is NumberOfCores (minus one).
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
          */
-        regs[2] = ((regs[2] & 0xf000u) + 1) | ((regs[2] & 0xffu) << 1) | 1u;
+        regs[2] &= 0xf0ffu;
         break;
     }
 }
@@ -134,11 +133,9 @@
     case 0x00000004:
         /*
          * EAX[31:26] is Maximum Cores Per Package (minus one).
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
          */
-        regs[0] = (((regs[0] & 0x7c000000u) << 1) | 0x04000000u |
-                   (regs[0] & 0x3ffu));
-        regs[3] &= 0x3ffu;
+        regs[0] &= 0xfc0003ffu;
+        regs[3] &= 0x00000003u;
         break;
 
     case 0x80000001: {
@@ -185,9 +182,8 @@
     case 0x00000001:
         /*
          * EBX[23:16] is Maximum Logical Processors Per Package.
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
          */
-        regs[1] = (regs[1] & 0x0000ffffu) | ((regs[1] & 0x007f0000u) << 1);
+        regs[1] & 0x00ffffffu;
 
         regs[2] &= (bitmaskof(X86_FEATURE_XMM3) |
                     bitmaskof(X86_FEATURE_SSSE3) |

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] libxc: remove CPUID core information mangling

[Keir Fraser]

On 25/08/2010 13:04, "Andre Przywara" <andre.przywara@amd.com> wrote:

> Hi,
> 
> c/s 18560:782599274bf9ae8857c55856c9c7fdf082967808 introduced CPUID
> mangling resulting in a doubled number of cores/processor exposed to the
> guest. According to comments in this patch the rationale behind this is
> to match the APIC numbering used by Xen.
> In my understanding the CPUID leafs dealing with number of cores always
> talk about logical numbers and not APIC IDs. So we don't need to adjust
> the CPUID readout to match the APIC ID enumeration scheme.
> If there were any serious reasons resulting in the old patch I'd love to
> hear them.
> 
> The attached patch fixes this and solves an issue I saw with certain
> NUMA guest configurations.

I think you shouldn't change handling of 80000008:ECX[15:12] since that does
explicitly refer to APIC ID arrangement. The rest of your changes could be
correct as far as I can tell from the reference manuals.

The intent by the way was to pass through host cores-per-package info so
that software licenses based on #packages would operate correctly in a
virtualised environment. It's pretty hacky though indeed.

 -- Keir

> Signed-off-by: Andre Przywara <andre.przywara@amd.com>

RE: Re: [PATCH] libxc: remove CPUID core information mangling

[Huang2, Wei]

Hi Keir,

Do you mean that we should leave 80000008:ECX[15:12] as zero or in old way (i.e. (regs[2] & 0xf000u) + 1))? These bits can't be zero, unless we want to use legacy method in multi-core calculation.

-Wei

========
I think you shouldn't change handling of 80000008:ECX[15:12] since that does
explicitly refer to APIC ID arrangement. The rest of your changes could be
correct as far as I can tell from the reference manuals.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: Re: [PATCH] libxc: remove CPUID core information mangling

[Keir Fraser]

I meant it should remain the old way, since HVM virtual APIC IDs are
vcpu_id*2.

 -- Keir

On 25/08/2010 16:28, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:

> Hi Keir,
> 
> Do you mean that we should leave 80000008:ECX[15:12] as zero or in old way
> (i.e. (regs[2] & 0xf000u) + 1))? These bits can't be zero, unless we want to
> use legacy method in multi-core calculation.
> 
> -Wei
> 
> ========
> I think you shouldn't change handling of 80000008:ECX[15:12] since that does
> explicitly refer to APIC ID arrangement. The rest of your changes could be
> correct as far as I can tell from the reference manuals.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

RE: Re: [PATCH] libxc: remove CPUID core information mangling

[Huang2, Wei]

OK. BTW, the old way seems wrong. The correct implementation should be (((regs[2] & 0xf000u) >> 12) + 1) << 12.

-----Original Message-----
From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] 
Sent: Wednesday, August 25, 2010 10:39 AM
To: Huang2, Wei; Przywara, Andre; Nitin Kamble
Cc: xen-devel
Subject: Re: [Xen-devel] Re: [PATCH] libxc: remove CPUID core information mangling

I meant it should remain the old way, since HVM virtual APIC IDs are
vcpu_id*2.

 -- Keir

On 25/08/2010 16:28, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:

> Hi Keir,
> 
> Do you mean that we should leave 80000008:ECX[15:12] as zero or in old way
> (i.e. (regs[2] & 0xf000u) + 1))? These bits can't be zero, unless we want to
> use legacy method in multi-core calculation.
> 
> -Wei
> 
> ========
> I think you shouldn't change handling of 80000008:ECX[15:12] since that does
> explicitly refer to APIC ID arrangement. The rest of your changes could be
> correct as far as I can tell from the reference manuals.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

Re: Re: [PATCH] libxc: remove CPUID core information mangling

[Keir Fraser]

Ah yes, I agree.


On 25/08/2010 16:53, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:

> OK. BTW, the old way seems wrong. The correct implementation should be
> (((regs[2] & 0xf000u) >> 12) + 1) << 12.
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Wednesday, August 25, 2010 10:39 AM
> To: Huang2, Wei; Przywara, Andre; Nitin Kamble
> Cc: xen-devel
> Subject: Re: [Xen-devel] Re: [PATCH] libxc: remove CPUID core information
> mangling
> 
> I meant it should remain the old way, since HVM virtual APIC IDs are
> vcpu_id*2.
> 
>  -- Keir
> 
> On 25/08/2010 16:28, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:
> 
>> Hi Keir,
>> 
>> Do you mean that we should leave 80000008:ECX[15:12] as zero or in old way
>> (i.e. (regs[2] & 0xf000u) + 1))? These bits can't be zero, unless we want to
>> use legacy method in multi-core calculation.
>> 
>> -Wei
>> 
>> ========
>> I think you shouldn't change handling of 80000008:ECX[15:12] since that does
>> explicitly refer to APIC ID arrangement. The rest of your changes could be
>> correct as far as I can tell from the reference manuals.
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>> 
>> 
> 
> 
> 
> 

Re: Re: [PATCH] libxc: remove CPUID core information mangling

[Andre Przywara]

Keir Fraser wrote:
> Ah yes, I agree.
> 
> 
> On 25/08/2010 16:53, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:
> 
>> OK. BTW, the old way seems wrong. The correct implementation should be
>> (((regs[2] & 0xf000u) >> 12) + 1) << 12.
>>
>> -----Original Message-----
>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>> Sent: Wednesday, August 25, 2010 10:39 AM
>> To: Huang2, Wei; Przywara, Andre; Nitin Kamble
>> Cc: xen-devel
>> Subject: Re: [Xen-devel] Re: [PATCH] libxc: remove CPUID core information
>> mangling
>>
>> I meant it should remain the old way, since HVM virtual APIC IDs are
>> vcpu_id*2.
I agree, that seems to be best way for the time being. Although this 
value is actually meant to tell different processors apart, so I guess 
it needs a revisit later.

FYI:
Real machines use different ways to assign APIC-IDs, for example my 
4-way Magny-Cours (4*12 cores) has this:
0x00-0x02: used for I/O-APICs, (could be 4-bit constrained)
     -0x0f: reserved for IOAPICs
0x10-0x1b: LAPIC-IDs for cores from the 1st processor
0x20-0x2b: LAPIC-IDs for cores from the 2nd processor
0x30-0x3b: LAPIC-IDs for cores from the 3rd processor
0x40-0x4b: LAPIC-IDs for cores from the 4th processor
The 80000008:ECX[15:12] value is 4, which means the lower 4 bits of the 
LAPIC ID indicate the core number within each package.
Obviously this scheme does not fit the Xen one's.

Thanks Wei for spotting the calculation error!

Regards,
Andre.

>>
>>  -- Keir
>>
>> On 25/08/2010 16:28, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:
>>
>>> Hi Keir,
>>>
>>> Do you mean that we should leave 80000008:ECX[15:12] as zero or in old way
>>> (i.e. (regs[2] & 0xf000u) + 1))? These bits can't be zero, unless we want to
>>> use legacy method in multi-core calculation.
>>>
>>> -Wei
>>>
>>> ========
>>> I think you shouldn't change handling of 80000008:ECX[15:12] since that does
>>> explicitly refer to APIC ID arrangement. The rest of your changes could be
>>> correct as far as I can tell from the reference manuals.
>>>


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

RE: [osrc-patches] [PATCH] libxc: remove CPUID core information mangling

[Huang2, Wei]

Few comments on this patch. 

* First, AMD CPUID:0x80000008:ECX[NC(7:0)] should match with the total number of cores (VCPUS) assigned to guest VM. Assigning 0xff to NC is a bit scary to me. According to AMD CPUID spec, NC represents the "actually implemented or enabled on the processor, as indicated by CPUID Fn8000_0008_ECX[NC]". Most OS doesn't enforce this check; but I saw our internal test program does that. 
* I am OK 80000008:ECX[15:12] change. But this change has an implication that all cores (VCPUS) are on the same physical processor. OS might not care about these bits. But this ought to be set correctly in my mind.

-----Original Message-----
From: osrc-patches-bounces@elbe.amd.com [mailto:osrc-patches-bounces@elbe.amd.com] On Behalf Of Andre Przywara
Sent: Wednesday, August 25, 2010 7:04 AM
To: Keir Fraser; Nitin Kamble
Cc: xen-devel
Subject: [osrc-patches] [PATCH] libxc: remove CPUID core information mangling

Hi,

c/s 18560:782599274bf9ae8857c55856c9c7fdf082967808 introduced CPUID mangling resulting in a doubled number of cores/processor exposed to the guest. According to comments in this patch the rationale behind this is to match the APIC numbering used by Xen.
In my understanding the CPUID leafs dealing with number of cores always talk about logical numbers and not APIC IDs. So we don't need to adjust the CPUID readout to match the APIC ID enumeration scheme.
If there were any serious reasons resulting in the old patch I'd love to hear them.

The attached patch fixes this and solves an issue I saw with certain NUMA guest configurations.

Signed-off-by: Andre Przywara <andre.przywara@amd.com>

--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12