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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2019 15:29, George Dunlap wrote:
> 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.

Trying to comment despite this having been an attachment:

>--- 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") ) {

The brace wants to move onto its own line.

>+            p->basic.htt = false;

I think everything below here indeed simply undoes what said old
commit did, but I can't match up this one. And together with the
question of whether instead leaving it alone, cmp_legacy then
would have the same question raised.

Jan

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