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

From: George Dunlap <george.dunlap@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Steven Haigh <netwiz@crc.id.au>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Andreas Kinzler <hfp@posteo.de>,
	IanJackson <ian.jackson@citrix.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH RFC] x86: Add hack to disable "Fake HT" mode
Date: Fri, 15 Nov 2019 14:29:35 +0000	[thread overview]
Message-ID: <0ec711d3-90d4-5fa0-7f16-8bf1ac8dc0ca@citrix.com> (raw)
In-Reply-To: <0f27d518-e6c4-a728-e59d-237acc92d493@suse.com>

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

On 11/15/19 2:18 PM, Jan Beulich wrote:
> On 15.11.2019 15:10, George Dunlap wrote:
>> On 11/15/19 2:06 PM, Andrew Cooper wrote:
>>> On 15/11/2019 14:04, George Dunlap wrote:
>>>>>> It's not entirely uncommon to (also) consider how the resulting
>>>>>> diff would look like when putting together a change. And besides
>>>>>> the review aspect, there's also the archeology one - "git blame"
>>>>>> yields much more helpful results when code doesn't get moved
>>>>>> around more than necessary. But yes, there's no very clear "this
>>>>>> is the better option" here. I've taken another look at the code
>>>>>> before your change though - everything is already nicely in one
>>>>>> place with Andrew's most recent code reorg. So I'm now having an
>>>>>> even harder time seeing why you want to move things around again.
>>>>> We don't.  I've recommend twice now to have a single "else if" hunk
>>>>> which is nearly empty, and much more obviously a gross "make it work for
>>>>> 4.13" bodge.
>>>> The results are a tiny bit better, but not much really (see attached).
>>>
>>> What I meant was:
>>>
>>>> diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
>>>> index 312c481f1e..bc088e45f0 100644
>>>> --- a/tools/libxc/xc_cpuid_x86.c
>>>> +++ b/tools/libxc/xc_cpuid_x86.c
>>>> @@ -579,52 +579,71 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
>>>>      }
>>>
>>> else if ( getenv() )
>>> {
>>>     ...
>>> }
>>>
>>>>      else
>>>>      {
>>>
>>> With no delta to this block at all.
>>
>> Then we have to duplicate this code in both blocks:
>>
>>         /*
>>          * These settings are necessary to cause earlier
>> HVM_PARAM_NESTEDHVM /
>>          * XEN_DOMCTL_disable_migrate settings to be reflected correctly in
>>          * CPUID.  Xen will discard these bits if configuration hasn't been
>>          * set for the domain.
>>          */
>>         p->extd.itsc = true;
>>         p->basic.vmx = true;
>>         p->extd.svm = true;
>>
>> I won't object if that's what you guys really want.
> 
> Personally I think the duplication is less bad than the far
> heavier original code churn, but to be honest, especially with
> this intended to go away again soon anyway, I'd not be opposed
> at all to
> 
>     ...
>     else if ( getenv() )
>         goto no_fake_ht;

This isn't correct, because you do need to clear htt and cmp_legacy, as
well as zeroing out cores_per_package and threads_per_cache on Intel.
(At least, that's what XenServer's patch does, and it's the best tested.)

>     else
>     {
>     ...
>  no_fake_ht:
>         /*
>          * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
>          * XEN_DOMCTL_disable_migrate settings to be reflected correctly in
>          * CPUID.  Xen will discard these bits if configuration hasn't been
>          * set for the domain.
>          */
>         p->extd.itsc = true;
>         p->basic.vmx = true;
>         p->extd.svm = true;
>     }
> 
> (despite my general dislike of goto).

Well I think gotos into other blocks is even worse. :-)

I think the result is a lot nicer to review for sure.

 -George

[-- Attachment #2: 0001-x86-Add-hack-to-disable-Fake-HT-mode.patch --]
[-- Type: text/x-patch, Size: 3759 bytes --]

From b83b7b6db04b0705878798cded2f4c6904cf6fb5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 14 Nov 2019 16:58:34 +0000
Subject: [PATCH] x86: Add hack to disable "Fake HT" mode

Changeset ca2eee92df44 ("x86, hvm: Expose host core/HT topology to HVM
guests") attempted to "fake up" a topology which would induce guest
operating systems to not treat vcpus as sibling hyperthreads.  This
involved (among other things) actually reporting hyperthreading as
available, but giving vcpus every other APICID.  The resulting cpu
featureset is invalid, but most operating systems on most hardware
managed to cope with it.

Unfortunately, Windows running on modern AMD hardware -- including
Ryzen 3xxx series processors, and reportedly EPYC "Rome" cpus -- gets
confused by the resulting contradictory feature bits and crashes
during installation.  (Linux guests have so far continued to cope.)

A "proper" fix is complicated and it's too late to fix it either for
4.13, or to backport to supported branches.  As a short-term fix,
implement an option to disable this "Fake HT" mode.  The resulting
topology reported will not be canonical, but experimentally continues
to work with Windows guests.

However, disabling this "Fake HT" mode has not been widely tested, and
will almost certainly break migration if applied inconsistently.

To minimize impact while allowing administrators to disable "Fake HT"
only on guests which are known not to work without it (i.e., Windows
guests) on affected hardware, add an environment variable which can be
set to disable the "Fake HT" mode on such hardware.

Reported-by: Steven Haigh <netwiz@crc.id.au>
Reported-by: Andreas Kinzler <hfp@posteo.de>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
This has been compile-tested only; I'm posting it early to get
feedback on the approach.

TODO: Prevent such guests from being migrated

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?

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Anthony Perard <anthony.perard@citrix.com>
---
 tools/libxc/xc_cpuid_x86.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 312c481f1e..029a1f1938 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -579,6 +579,26 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
     }
     else
     {
+        if ( getenv("XEN_LIBXC_DISABLE_FAKEHT") ) {
+            p->basic.htt = false;
+            p->extd.cmp_legacy = false;
+
+            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 = 0;
+                    p->cache.subleaf[i].threads_per_cache = 0;
+                }
+                break;
+            }
+
+            goto hvm_common;
+        }
+
+
         /*
          * 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.
@@ -627,6 +647,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
             break;
         }
 
+    hvm_common:
         /*
          * These settings are necessary to cause earlier HVM_PARAM_NESTEDHVM /
          * XEN_DOMCTL_disable_migrate settings to be reflected correctly in
-- 
2.24.0


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

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