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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
