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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/09/2019 11:20, Jan Beulich wrote:
> On 20.09.2019 12:05, Andrew Cooper wrote:
>> On 19/09/2019 12:48, 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.
>>>
>>> Additionally don't even do any adjustment when the host topology already
>>> includes room for multiple threads per core.
>>>
>>> Furthermore don't double the Maximum Cores Per Package at all - by us
>>> introducing a fake HTT effect, the core count doesn't need to change.
>>> Instead adjust the Maximum Logical Processors Sharing Cache field, which
>>> so far was zapped altogether.
>>>
>>> Also zap leaf 4 (and at the same time leaf 2) EDX output for AMD.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> ---
>>> TBD: Using xc_physinfo() output here needs a better solution. The
>>>      threads_per_core value returned is the count of active siblings of
>>>      CPU 0, rather than a system wide applicable value (and constant
>>>      over the entire session). Using CPUID output (leaves 4 and
>>>      8000001e) doesn't look viable either, due to this not really being
>>>      the host values on PVH. Judging from the host feature set's HTT
>>>      flag also wouldn't tell us whether there actually are multiple
>>>      threads per core.
>> The key thing is that htt != "more than one thread per core".  HTT is
>> strictly a bit indicating that topology information is available in a
>> new form in the CPUID leaves (or in AMDs case, the same information
>> should be interpreted in a new way).  Just because HTT is set (and it
>> does get set in non-HT capable systems), doesn't mean there is space for
>> more than thread per core in topology information.
>>
>> For PV guests, my adjustment in the CPUID series shows (what I believe
>> to be) the only correct way of propagating the host HTT/CMP_LEGACY
>> settings through.
>>
>> For HVM guests, it really shouldn't really have anything to do with the
>> host setting.  We should be choosing how many threads/core to give to
>> the guest, then constructing the topology information from first principles.
>>
>> Ignore the PVH case.  It is totally broken for several other reasons as
>> well, and PVH Dom0 isn't a production-ready thing yet.
>>
>> This gets us back to the PV case where the host information is actually
>> in view, and (for backport purposes) can be trusted.
> Okay, this means I'll revive and finish the half cpuid() based attempt
> I had made initially. A fundamental question remains open though from
> your reply: Do you agree with the idea of avoiding the multiplication
> by 2 if the host topology already provides at least one bit of thread
> ID within the APIC ID?

In theory, yes.  In practice, I'd err on the side of not.

A further problem with CPUID handling is that it is recalculated from
scratch even after migrate.  Therefore, any changes to the algorithm
will cause inconsistencies to be seen in the guest across
migrate/upgrade.  This problem becomes substantially worse if the patch
is backported to stable trees.

Now that get_cpu_policy has existed for a little while, and
set_cpu_policy is imminent, fixing the "CPUID changes across migrate"
problem is almost doable, and is on the plan for toolstack work.

That said, ultimately, anything "pre 4.14" => "4.14" is going to hit a
discontinuity, because there is information discarded on the source side
which can't be reconstructed on the destination.

Overall, I would suggest doing the absolute minimum change required to
unbreak Rome CPUs.  Everything further is going to cause differences
across migrate.

In 4.14, I think we can reasonably fix all of:
1) CPUID data discarded for migrate
2) domain builder uses native CPUID
3) topology handling isn't consistent with SDM/APM

all of which is libxc/libxl work, once set_cpu_policy() is in place.

~Andrew

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