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

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

On Tue, Jan 28, 2020 at 11:21:14AM +0000, Andrew Cooper wrote:
> 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.

Would it be enough to populate boot_cpu_data before setting
cpuidmask_defaults?

So that at least the data in boot_cpu_data is not tainted with the
bits in cpuidmask_defaults?

I'm certainly not an expert on that area, so your judgment on the
least bad approach is likely more accurate than mine.

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

Oh, so on AMD cpuidmask_defaults is not a mask, it can also force
features to appear on cpuid, even when not present on the hardware
cpuid.

Would it make sense to and cpuidmask_defaults with the real hardware
policy before writing it to the MSR in order to prevent things not
present on the hardware policy from appearing magically?

Thanks, Roger.

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