Re: [Xen-devel] [PATCH RFC] x86: Add hack to disable "Fake HT" mode

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 11/15/19 11:12 AM, Jan Beulich wrote:
> On 15.11.2019 11:57, George Dunlap wrote:
>> Open questions:
>>
>> - Is this the right place to put the `getenv` check?
>>
>> - Is there any way we can make migration work, at least in some cases?
>>
>> - Can we check for known-problematic models, and at least report a
>>   more useful error?
> 
> Checking for specific models should be straightforward, but I wonder
> how sensible it is to compile a likely ever growing list into here.

The hope is that this would be a short-term fix, replaced by something
more robust in 4.14.

> 
> As to the reporting of an error - you saying "more useful" suggests
> there is some error already being reported. But I don't see any here,
> nor does any come to mind.

At the moment, if someone buys a Ryzen 3xxx box, installs Xen, and tries
to boot a Windows guest, Windows will simply crash (as I understand it).
 (That is, the issue is "reported" by a BSoD.)  That person will then
google around, and hopefully run across the various threads here, and
see that she should add "export XEN_LIBXC_DISABLE_FAKEHT=1" when
starting their Windows guests.

Suppose instead that after buying a Ryzen 3xxx box, and installing Xen,
when starting a guest, they got the following error message:

"WARNING: This system may be affected by Xen bug $FOO.  Please see
$DOCUMENTATION and set XEN_LIBXC_DISABLE_FAKEHT appropriately."

It would mean annoying people not running Windows unnecessarily; but it
seems like a better experience than having a guest crash and searching
for a solution.

(And to be clear, the reason it's RFC is to ask what people think of
this idea, rather than to argue that this is what we should do.)

> 
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -579,52 +579,68 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>> domid,
>>      }
>>      else
>>      {
>> -        /*
>> -         * Topology for HVM guests is entirely controlled by Xen.  For now, 
>> we
>> -         * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
>> -         */
>> -        p->basic.htt = true;
>> +        p->basic.htt = false;
>>          p->extd.cmp_legacy = false;
>>  
>> -        /*
>> -         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
>> -         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
>> -         * overflow.
>> -         */
>> -        if ( !(p->basic.lppp & 0x80) )
>> -            p->basic.lppp *= 2;
>> -
> 
> I appreciate you wanting to put all adjustments in a central place, but
> at least it makes patch review more difficult. How about you latch
> !getenv("XEN_LIBXC_DISABLE_FAKEHT") into a local boolean at the top of
> the function and then the above would become
> 
>         if ( !(p->basic.lppp & 0x80) )
>             p->basic.lppp <<= fakeht;
> 
> and e.g. ...
> 
>>          switch ( p->x86_vendor )
>>          {
>>          case X86_VENDOR_INTEL:
>>              for ( i = 0; (p->cache.subleaf[i].type &&
>>                            i < ARRAY_SIZE(p->cache.raw)); ++i )
>>              {
>> -                p->cache.subleaf[i].cores_per_package =
>> -                    (p->cache.subleaf[i].cores_per_package << 1) | 1;
> 
> ... this
> 
>                 p->cache.subleaf[i].cores_per_package =
>                     (p->cache.subleaf[i].cores_per_package << fakeht) | 
> fakeht;

I'm afraid I think the code itself would then become more difficult to
read; and it seems a bit strange to be architecting our code based on
limitations of the diff algorithm and/or diff viewer used.

I could break the patch down into two if you think that would make it
easier to review.  You might also try a diff viewer which shows both
before and after; I use 'meld'.

>> +                p->cache.subleaf[i].cores_per_package = 0;
> 
> This doesn't look correct - you need to leave alone the field if
> the adjustment moved down is supposed to have any effect. This
> is an example of why the bigger code movement you do is risky.

Ah yes, I didn't notice the calculation was self-referential.  Let me
take a look.

 -George

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