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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/01/2020 10:39, Roger Pau Monné wrote:
>
>> 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.

The comment just out of context explains why.

The APIC bit is special (fast forwarded from MSR_APIC_BASE.EN) and for
the fast-forwarding to work correctly, the bit needs to remain
unconditionally set in the "mask" MSR.

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

cpuidmask_defaults serves as an and-mask over any content the guest
policy selects.

This is because, in the AMD case, these are actually override MSRs
rather than mask MSRs.  Care has to be taken not to advertise any
features the pipeline can't deliver on, but it is also the properly
which allows us to advertise the HYPERVISOR bit in general.

Previously, HYPERVISOR was within the and mask, so if the guest policy
requests it (which it will by default, but can be turned with sufficient
cpuid= configuration), it would get included.  With this change,
HYPERVISOR is now clear in the mask, so needs explicitly adding back in.

~Andrew

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