[Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Jan Beulich]

Recent AMD processors may report up to 128 logical processors in CPUID
leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
as the respective field is only 8 bits wide. Suppress doubling the value
(and its leaf 0x80000008 counterpart) in such a case.

Note that while there's a similar overflow in intel_xc_cpuid_policy(),
that one is being left alone for now.

Note further that while it was considered to suppress the multiplication
by 2 altogether if the host topology already provides at least one bit
of thread ID within APIC IDs, it was decided to avoid more change here
than really needed at this point.

Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
should have been from the beginning.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop use of physinfo. Drop Intel-only leaf 4 change. Increment
    ApicIdCoreSize only when doubling NumberOfCores.

--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -385,7 +385,7 @@ static void amd_xc_cpuid_policy(const st
     {
     case 0x00000002:
     case 0x00000004:
-        regs[0] = regs[1] = regs[2] = 0;
+        regs[0] = regs[1] = regs[2] = regs[3] = 0;
         break;
 
     case 0x80000000:
@@ -395,11 +395,20 @@ static void amd_xc_cpuid_policy(const st
 
     case 0x80000008:
         /*
-         * ECX[15:12] is ApicIdCoreSize: ECX[7:0] is NumberOfCores (minus one).
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
+         * ECX[15:12] is ApicIdCoreSize.
+         * ECX[7:0] is NumberOfCores (minus one).
+         * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But make sure to avoid
+         * - overflow,
+         * - going out of sync with leaf 1 EBX[23:16],
+         * - incrementing ApicIdCoreSize when it's zero (which changes the
+         *   meaning of bits 7:0).
          */
-        regs[2] = ((regs[2] + (1u << 12)) & 0xf000u) |
-                  ((regs[2] & 0xffu) << 1) | 1u;
+        if ( (regs[2] & 0x7fu) < 0x7fu )
+        {
+            if ( (regs[2] & 0xf000u) && (regs[2] & 0xf000u) != 0xf000u )
+                regs[2] = ((regs[2] + 0x1000u) & 0xf000u) | (regs[2] & 0xffu);
+            regs[2] = (regs[2] & 0xf000u) | ((regs[2] & 0x7fu) << 1) | 1u;
+        }
         break;
 
     case 0x8000000a: {
@@ -478,9 +487,13 @@ static void xc_cpuid_hvm_policy(const st
     case 0x00000001:
         /*
          * EBX[23:16] is Maximum Logical Processors Per Package.
-         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
+         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
+         * overflow.
          */
-        regs[1] = (regs[1] & 0x0000ffffu) | ((regs[1] & 0x007f0000u) << 1);
+        if ( !(regs[1] & 0x00800000u) )
+            regs[1] = (regs[1] & 0x0000ffffu) | ((regs[1] & 0x007f0000u) << 1);
+        else
+            regs[1] &= 0x00ffffffu;
 
         regs[2] = info->featureset[featureword_of(X86_FEATURE_SSE3)];
         regs[3] = (info->featureset[featureword_of(X86_FEATURE_FPU)] |

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Jan Beulich]

On 20.09.2019 15:54, Jan Beulich wrote:
> Recent AMD processors may report up to 128 logical processors in CPUID
> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
> as the respective field is only 8 bits wide. Suppress doubling the value
> (and its leaf 0x80000008 counterpart) in such a case.
> 
> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
> that one is being left alone for now.
> 
> Note further that while it was considered to suppress the multiplication
> by 2 altogether if the host topology already provides at least one bit
> of thread ID within APIC IDs, it was decided to avoid more change here
> than really needed at this point.

Given this I've just changed the title to "avoid certain overflows in
CPUID APIC ID adjustments".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Jan Beulich]

On 20.09.2019 15:54, Jan Beulich wrote:
> Recent AMD processors may report up to 128 logical processors in CPUID
> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
> as the respective field is only 8 bits wide. Suppress doubling the value
> (and its leaf 0x80000008 counterpart) in such a case.
> 
> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
> that one is being left alone for now.
> 
> Note further that while it was considered to suppress the multiplication
> by 2 altogether if the host topology already provides at least one bit
> of thread ID within APIC IDs, it was decided to avoid more change here
> than really needed at this point.
> 
> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
> should have been from the beginning.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Drop use of physinfo. Drop Intel-only leaf 4 change. Increment
>     ApicIdCoreSize only when doubling NumberOfCores.

Thinking about it some more, dropping the leaf 4 change seems at least
somewhat risky to me. This being just a 6-bit field (and effectively
already saturating in a way, at least when power-of-two maximum core
counts are involved), and hence there being 2 bits of "playing room"
between this and the involved leaf 1 field, the calculation there not
getting adjusted is still a latent risk imo with guest side calculations
like this one

	smp_num_siblings = smp_num_siblings / c->x86_max_cores;

found in basically all versions of Linux (where certain functions,
e.g. apic_id_is_primary_thread(), won't cope with smp_num_siblings
ending up as zero, while others, e.g. topology_smt_supported(), do).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Wei Liu]

On Fri, Sep 20, 2019 at 03:54:12PM +0200, Jan Beulich wrote:
> Recent AMD processors may report up to 128 logical processors in CPUID
> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
> as the respective field is only 8 bits wide. Suppress doubling the value
> (and its leaf 0x80000008 counterpart) in such a case.
> 
> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
> that one is being left alone for now.
> 
> Note further that while it was considered to suppress the multiplication
> by 2 altogether if the host topology already provides at least one bit
> of thread ID within APIC IDs, it was decided to avoid more change here
> than really needed at this point.
> 
> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
> should have been from the beginning.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm going to defer this patch to Andrew.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Andrew Cooper]

On 20/09/2019 14:54, Jan Beulich wrote:
> Recent AMD processors may report up to 128 logical processors in CPUID
> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
> as the respective field is only 8 bits wide. Suppress doubling the value
> (and its leaf 0x80000008 counterpart) in such a case.
>
> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
> that one is being left alone for now.
>
> Note further that while it was considered to suppress the multiplication
> by 2 altogether if the host topology already provides at least one bit
> of thread ID within APIC IDs, it was decided to avoid more change here
> than really needed at this point.
>
> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
> should have been from the beginning.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll rebase my CPUID series over this change.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Andrew Cooper]

On 23/09/2019 09:29, Jan Beulich wrote:
> On 20.09.2019 15:54, Jan Beulich wrote:
>> Recent AMD processors may report up to 128 logical processors in CPUID
>> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
>> as the respective field is only 8 bits wide. Suppress doubling the value
>> (and its leaf 0x80000008 counterpart) in such a case.
>>
>> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
>> that one is being left alone for now.
>>
>> Note further that while it was considered to suppress the multiplication
>> by 2 altogether if the host topology already provides at least one bit
>> of thread ID within APIC IDs, it was decided to avoid more change here
>> than really needed at this point.
>>
>> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
>> should have been from the beginning.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Drop use of physinfo. Drop Intel-only leaf 4 change. Increment
>>     ApicIdCoreSize only when doubling NumberOfCores.
> Thinking about it some more, dropping the leaf 4 change seems at least
> somewhat risky to me. This being just a 6-bit field (and effectively
> already saturating in a way, at least when power-of-two maximum core
> counts are involved), and hence there being 2 bits of "playing room"
> between this and the involved leaf 1 field, the calculation there not
> getting adjusted is still a latent risk imo with guest side calculations
> like this one
>
> 	smp_num_siblings = smp_num_siblings / c->x86_max_cores;
>
> found in basically all versions of Linux (where certain functions,
> e.g. apic_id_is_primary_thread(), won't cope with smp_num_siblings
> ending up as zero, while others, e.g. topology_smt_supported(), do).

None of these adjustments are correct.  The topology should be
constructed from first principles.

What matters is having as few alterations to the algorithm as necessary,
until we can fix the many toolstack side issues.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Jan Beulich]

On 23.09.2019 19:33, Andrew Cooper wrote:
> On 23/09/2019 09:29, Jan Beulich wrote:
>> On 20.09.2019 15:54, Jan Beulich wrote:
>>> Recent AMD processors may report up to 128 logical processors in CPUID
>>> leaf 1. Doubling this value produces 0 (which OSes sincerely dislike),
>>> as the respective field is only 8 bits wide. Suppress doubling the value
>>> (and its leaf 0x80000008 counterpart) in such a case.
>>>
>>> Note that while there's a similar overflow in intel_xc_cpuid_policy(),
>>> that one is being left alone for now.
>>>
>>> Note further that while it was considered to suppress the multiplication
>>> by 2 altogether if the host topology already provides at least one bit
>>> of thread ID within APIC IDs, it was decided to avoid more change here
>>> than really needed at this point.
>>>
>>> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD, as it
>>> should have been from the beginning.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v2: Drop use of physinfo. Drop Intel-only leaf 4 change. Increment
>>>     ApicIdCoreSize only when doubling NumberOfCores.
>> Thinking about it some more, dropping the leaf 4 change seems at least
>> somewhat risky to me. This being just a 6-bit field (and effectively
>> already saturating in a way, at least when power-of-two maximum core
>> counts are involved), and hence there being 2 bits of "playing room"
>> between this and the involved leaf 1 field, the calculation there not
>> getting adjusted is still a latent risk imo with guest side calculations
>> like this one
>>
>> 	smp_num_siblings = smp_num_siblings / c->x86_max_cores;
>>
>> found in basically all versions of Linux (where certain functions,
>> e.g. apic_id_is_primary_thread(), won't cope with smp_num_siblings
>> ending up as zero, while others, e.g. topology_smt_supported(), do).
> 
> None of these adjustments are correct.  The topology should be
> constructed from first principles.

Fully agree.

> What matters is having as few alterations to the algorithm as necessary,
> until we can fix the many toolstack side issues.

But if an alteration is needed, we should also try to limit the
risk of it introducing regressions elsewhere. I wrote the above reply
simply because I'm uncertain whether the larger risk is to leave
leaf 4 handling unchanged, or to change it along the lines of the
other two adjustments done here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Andrew Cooper]

On 20/09/2019 14:54, Jan Beulich wrote:
> @@ -395,11 +395,20 @@ static void amd_xc_cpuid_policy(const st
>  
>      case 0x80000008:
>          /*
> -         * ECX[15:12] is ApicIdCoreSize: ECX[7:0] is NumberOfCores (minus one).
> -         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
> +         * ECX[15:12] is ApicIdCoreSize.
> +         * ECX[7:0] is NumberOfCores (minus one).
> +         * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But make sure to avoid
> +         * - overflow,
> +         * - going out of sync with leaf 1 EBX[23:16],
> +         * - incrementing ApicIdCoreSize when it's zero (which changes the
> +         *   meaning of bits 7:0).
>           */
> -        regs[2] = ((regs[2] + (1u << 12)) & 0xf000u) |
> -                  ((regs[2] & 0xffu) << 1) | 1u;
> +        if ( (regs[2] & 0x7fu) < 0x7fu )

In attempting to rebase my series, there is a bug here.  It should be &
0xff otherwise the top bit isn't included in the comparison, and a value
of 128 will still be doubled.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] libxc/x86: avoid overflow in CPUID APIC ID adjustments

[Jan Beulich]

On 24.09.2019 19:58, Andrew Cooper wrote:
> On 20/09/2019 14:54, Jan Beulich wrote:
>> @@ -395,11 +395,20 @@ static void amd_xc_cpuid_policy(const st
>>   
>>       case 0x80000008:
>>           /*
>> -         * ECX[15:12] is ApicIdCoreSize: ECX[7:0] is NumberOfCores (minus one).
>> -         * Update to reflect vLAPIC_ID = vCPU_ID * 2.
>> +         * ECX[15:12] is ApicIdCoreSize.
>> +         * ECX[7:0] is NumberOfCores (minus one).
>> +         * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But make sure to avoid
>> +         * - overflow,
>> +         * - going out of sync with leaf 1 EBX[23:16],
>> +         * - incrementing ApicIdCoreSize when it's zero (which changes the
>> +         *   meaning of bits 7:0).
>>            */
>> -        regs[2] = ((regs[2] + (1u << 12)) & 0xf000u) |
>> -                  ((regs[2] & 0xffu) << 1) | 1u;
>> +        if ( (regs[2] & 0x7fu) < 0x7fu )
> 
> In attempting to rebase my series, there is a bug here.  It should be &
> 0xff otherwise the top bit isn't included in the comparison, and a value
> of 128 will still be doubled.

Oh, indeed, thanks for spotting. Will send a fixup patch once in the 
office, unless you did so already.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel