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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel