Re: [Xen-devel] [PATCH RFC] x86/amd: Avoid cpu_has_hypervisor evaluating true on native hardware

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 27, 2020 at 08:21:21PM +0000, Andrew Cooper wrote:
> Currently when booting native on AMD hardware, cpuidmask_defaults._1cd gets
> configured with the HYPERVISOR bit before native CPUID is scanned for feature
> bits.
> 
> This results in cpu_has_hypervisor becoming set as part of identify_cpu(), and
> ends up appearing in the raw and host CPU policies.  Nothing has really cared
> in the past.
> 
> Alter amd_init_levelling() to exclude the HYPERVISOR bit from
> cpumask_defaults, and update domain_cpu_policy_changed() to allow it to be
> explicitly forwarded.
> 
> This in turn highlighted that dom0 construction was asymetric with domU
> construction, by not having any calls to domain_cpu_policy_changed().  Extend
> arch_domain_create() to always call domain_cpu_policy_changed().
> 
> Reported-by: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> 
> Without this fix, there is apparently a problem with Roger's "[PATCH v3 7/7]
> x86/tlb: use Xen L0 assisted TLB flush when available" on native AMD hardware.
> I haven't investgiated the issue with that patch specifically, because
> cpu_has_hypervisor being wrong is obviously a bug.

I've tested the series on one AMD box and it worked for me. Even if
cpu_has_hypervisor is set on real hardware the added call to
hypervisor_flush_tlb should be fine as ops is NULL in that case and
would just be a dummy (this obviously needs to be fixed so
cpu_has_hypervisor isn't true when not running virtualized).

> 
> This is one of two possible approaches, and both have their downsides.  This
> one takes an extra hit on context switches between PV vcpus and idle/hvm, as
> they will usually differ in HYPERVISOR bit.
> 
> The other approach is to order things more carefully so levelling is
> configured after scanning for cpuid bits, but that has the downside that it is
> very easy to regress.
> 
> Thoughts on which is the least-bad approach to take?  Having written this
> patch, I'm now erring on the side of doing it the other way.
> ---
>  xen/arch/x86/cpu/amd.c       | 3 ---
>  xen/arch/x86/domain.c        | 2 ++
>  xen/arch/x86/domctl.c        | 9 ++++++++-
>  xen/include/asm-x86/domain.h | 2 ++
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 8b5f0f2e4c..0906b23582 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -297,9 +297,6 @@ static void __init noinline amd_init_levelling(void)
>                       ecx |= cpufeat_mask(X86_FEATURE_OSXSAVE);
>               edx |= cpufeat_mask(X86_FEATURE_APIC);
>  
> -             /* Allow the HYPERVISOR bit to be set via guest policy. */
> -             ecx |= cpufeat_mask(X86_FEATURE_HYPERVISOR);

We also seem to force X86_FEATURE_APIC into the policy, which seems
wrong?

I guess all AMD hardware Xen boots on has the APIC feature, so this
isn't a real issue, but still seems quite weird.

>               cpuidmask_defaults._1cd = ((uint64_t)ecx << 32) | edx;
>       }
>  
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 28fefa1f81..316b801597 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -665,6 +665,8 @@ int arch_domain_create(struct domain *d,
>       */
>      d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
>  
> +    domain_cpu_policy_changed(d);
> +
>      return 0;
>  
>   fail:
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 5ed63ac10a..0627eb4e06 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -48,7 +48,7 @@ static int gdbsx_guest_mem_io(domid_t domid, struct 
> xen_domctl_gdbsx_memio *iop)
>  }
>  #endif
>  
> -static void domain_cpu_policy_changed(struct domain *d)
> +void domain_cpu_policy_changed(struct domain *d)
>  {
>      const struct cpuid_policy *p = d->arch.cpuid;
>      struct vcpu *v;
> @@ -106,6 +106,13 @@ static void domain_cpu_policy_changed(struct domain *d)
>                      ecx = 0;
>                  edx = cpufeat_mask(X86_FEATURE_APIC);
>  
> +                /*
> +                 * If the Hypervisor bit is set in the policy, we can also
> +                 * forward it into real CPUID.
> +                 */
> +                if ( p->basic.hypervisor )
> +                    ecx |= cpufeat_mask(X86_FEATURE_HYPERVISOR);

Since the hypervisor bit will be part of both the HVM and PV max
policies, why do you need to explicitly allow it here?

Won't it be naturally added to the guest policy as the rest of
features?

Thanks, Roger.

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