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

[Jan Beulich <jbeulich@xxxxxxxx>]

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;
    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).

Jan

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