Re: [Xen-devel] [PATCH 3/3] x86/levelling: Provide architectural OSXSAVE handling to masked native CPUID

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.16 at 19:26, <andrew.cooper3@xxxxxxxxxx> wrote:
> Contrary to c/s b2507fe7 "x86/domctl: Update PV domain cpumasks when setting
> cpuid policy", Intel CPUID masks are applied after fast forwarding hardware
> state, rather than before.  (All behaviour in this regard appears completely
> undocumented by both Intel and AMD).
> 
> Therefore, a set bit in the MSR causes hardware to be fast-forwarded, while a
> clear bit forces the guests view to 0, even if CR4.OSXSAVE is actually set.
> 
> This allows Xen to provide an architectural view of a guest kernels
> CR4.OSXSAVE setting to any CPUID instruction issused by guest kernel or
> userspace.

Perhaps worth saying "non-PV CPUID instruction issued"?

> The masking value defaults to 1 (if the guest has XSAVE available) to cause
> fast-forwarding to occur for the HVM and idle vcpus.

Is the latter part actually true? I don't think idle vCPU-s get through
update_domain_cpuid_info(), as that gets called solely from a domctl
handler.

> Repored-by: Jan Beulich <JBeulich@xxxxxxxx>

Reported-...

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -211,6 +211,24 @@ static void amd_ctxt_switch_levelling(const struct vcpu 
> *next)
>               (nextd && is_pv_domain(nextd) && 
> nextd->arch.pv_domain.cpuidmasks)
>               ? nextd->arch.pv_domain.cpuidmasks : &cpuidmask_defaults;
>  
> +     if ((levelling_caps & LCAP_1cd) == LCAP_1cd) {
> +             uint64_t val = masks->_1cd;
> +
> +             /*
> +              * OSXSAVE defaults to 1, which causes fast-forwarding of
> +              * Xen's real setting.  Clobber it if disabled by the guest

Tab inside the comment?

> +              * kernel.
> +              */
> +             if (next && is_pv_vcpu(next) &&
> +                 !(next->arch.pv_vcpu.ctrlreg[4] & X86_CR4_OSXSAVE))
> +                     val &= ~((uint64_t)cpufeat_mask(X86_FEATURE_OSXSAVE) << 
> 32);

I don't think idle vCPU-s would ever have their ctrlreg[4] updated, so
other than the description says you hide the flag here for them. Since
that shouldn't matter much except for the number of cases the
WRMSR below gets executed because the masks didn't match, I'm
not sure whether it's better to fix it here or to alter the description.
One might guess that fixing it would be better, as likely going forward
most (all?) guests will enable xsave, and hence we might save the
WRMSR in most of the cases if not needed for any other reason.

> @@ -218,6 +235,11 @@ static void __init noinline intel_init_levelling(void)
>               ecx &= opt_cpuid_mask_ecx;
>               edx &= opt_cpuid_mask_edx;
>  
> +             /* Fast-forward bits - Must be set. */
> +             if (ecx & cpufeat_mask(X86_FEATURE_XSAVE))
> +                     ecx |= cpufeat_mask(X86_FEATURE_OSXSAVE);
> +             edx |= cpufeat_mask(X86_FEATURE_APIC);
> +
>               cpuidmask_defaults._1cd &= ((u64)edx << 32) | ecx;
>       }

Do we really want to rely on the mask MSRs to have all interesting
bits (and namely the two ones relevant here) set by the firmware?
I.e. don't you want to instead OR in the two bits after the &=?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel