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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2019 12:58, George Dunlap wrote:
> On 11/15/19 11:12 AM, Jan Beulich wrote:
>> On 15.11.2019 11:57, George Dunlap wrote:
>>> --- a/tools/libxc/xc_cpuid_x86.c
>>> +++ b/tools/libxc/xc_cpuid_x86.c
>>> @@ -579,52 +579,68 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
>>> domid,
>>>      }
>>>      else
>>>      {
>>> -        /*
>>> -         * 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.
>>> -         */
>>> -        p->basic.htt = true;
>>> +        p->basic.htt = false;
>>>          p->extd.cmp_legacy = false;
>>>  
>>> -        /*
>>> -         * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
>>> -         * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to 
>>> avoid
>>> -         * overflow.
>>> -         */
>>> -        if ( !(p->basic.lppp & 0x80) )
>>> -            p->basic.lppp *= 2;
>>> -
>>
>> I appreciate you wanting to put all adjustments in a central place, but
>> at least it makes patch review more difficult. How about you latch
>> !getenv("XEN_LIBXC_DISABLE_FAKEHT") into a local boolean at the top of
>> the function and then the above would become
>>
>>         if ( !(p->basic.lppp & 0x80) )
>>             p->basic.lppp <<= fakeht;
>>
>> and e.g. ...
>>
>>>          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 =
>>> -                    (p->cache.subleaf[i].cores_per_package << 1) | 1;
>>
>> ... this
>>
>>                 p->cache.subleaf[i].cores_per_package =
>>                     (p->cache.subleaf[i].cores_per_package << fakeht) | 
>> fakeht;
> 
> I'm afraid I think the code itself would then become more difficult to
> read;

Slightly, but yes.

> and it seems a bit strange to be architecting our code based on
> limitations of the diff algorithm and/or diff viewer used.

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.

Jan

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