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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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: 0001-x86-Add-hack-to-disable-Fake-HT-mode.patch
Description: Text Data

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