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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2019 11:57, George Dunlap wrote:
> Open questions:
> 
> - Is this the right place to put the `getenv` check?
> 
> - Is there any way we can make migration work, at least in some cases?
> 
> - Can we check for known-problematic models, and at least report a
>   more useful error?

Checking for specific models should be straightforward, but I wonder
how sensible it is to compile a likely ever growing list into here.

As to the reporting of an error - you saying "more useful" suggests
there is some error already being reported. But I don't see any here,
nor does any come to mind.

> --- 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;

> +                p->cache.subleaf[i].cores_per_package = 0;

This doesn't look correct - you need to leave alone the field if
the adjustment moved down is supposed to have any effect. This
is an example of why the bigger code movement you do is risky.

Jan

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