On 11/15/19 2:04 PM, George Dunlap wrote: > On 11/15/19 1:55 PM, Andrew Cooper wrote: >> 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 results are a tiny bit better, but not much really (see attached).
I mean, if we *really* wanted to optimize for diff readability, we could use `goto`s instead, but I thought that would be going a bit too far. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel