On 11/15/19 2:42 PM, Jan Beulich wrote:
> 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.
Ack
>
>> + 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.
This is based on a XenServer patch which reverts the entire commit, and
has been maintained in the patchqueue since the commit made it upstream,
AFAICT. So I'll let someone from that team comment on the wherefores;
but as I said, it's by far the best tested option we're going to get.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
