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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/11/2019 12:39, Jan Beulich wrote:
> 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.

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 only thing which is an open question (IMO) is if/how to trigger this
quirk mode.  There are no good options.  We just need to agree on the
least bad one.

~Andrew

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