[PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3770 bytes --]

User mode code generally cannot be expected to invoke the PV-enabled
CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
(as well as even nowadays on levelling incapable hardware) such CPUID
invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
in other CPUID handling.

To make guest CR4 changes immediately visible via CPUID, also invoke
ctxt_switch_levelling() from the CR4 write path.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -206,17 +206,30 @@ static void __init noinline probe_maskin
 static void amd_ctxt_switch_levelling(const struct domain *nextd)
 {
 	struct cpuidmasks *these_masks = &this_cpu(cpuidmasks);
-	const struct cpuidmasks *masks =
-		(nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.cpuidmasks)
-		? nextd->arch.pv_domain.cpuidmasks : &cpuidmask_defaults;
+	const struct cpuidmasks *masks = NULL;
+	unsigned long cr4;
+	uint64_t val__1cd = 0, val_e1cd = 0, val__7ab0 = 0, val__6c = 0;
+
+	if (nextd && is_pv_domain(nextd) && !is_idle_domain(nextd)) {
+		cr4 = current->arch.pv_vcpu.ctrlreg[4];
+		masks = nextd->arch.pv_domain.cpuidmasks;
+	} else
+		cr4 = read_cr4();
+
+	if (cr4 & X86_CR4_OSXSAVE)
+		val__1cd |= (uint64_t)cpufeat_mask(X86_FEATURE_OSXSAVE) << 32;
+
+	if (!masks)
+		masks = &cpuidmask_defaults;
 
 #define LAZY(cap, msr, field)						\
 	({								\
-		if (unlikely(these_masks->field != masks->field) &&	\
+		val_##field |= masks->field;				\
+		if (unlikely(these_masks->field != val_##field) &&	\
 		    ((levelling_caps & cap) == cap))			\
 		{							\
-			wrmsr_amd(msr, masks->field);			\
-			these_masks->field = masks->field;		\
+			wrmsr_amd(msr, val_##field);			\
+			these_masks->field = val_##field;		\
 		}							\
 	})
 
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -154,7 +154,9 @@ static void __init probe_masking_msrs(vo
 static void intel_ctxt_switch_levelling(const struct domain *nextd)
 {
 	struct cpuidmasks *these_masks = &this_cpu(cpuidmasks);
-	const struct cpuidmasks *masks;
+	const struct cpuidmasks *masks = NULL;
+	unsigned long cr4;
+	uint64_t val__1cd = 0, val_e1cd = 0, val_Da1 = 0;
 
 	if (cpu_has_cpuid_faulting) {
 		/*
@@ -178,16 +180,27 @@ static void intel_ctxt_switch_levelling(
 		return;
 	}
 
-	masks = (nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.cpuidmasks)
-		? nextd->arch.pv_domain.cpuidmasks : &cpuidmask_defaults;
+	if (nextd && is_pv_domain(nextd) && !is_idle_domain(nextd)) {
+		cr4 = current->arch.pv_vcpu.ctrlreg[4];
+		masks = nextd->arch.pv_domain.cpuidmasks;
+	} else
+		cr4 = read_cr4();
+
+	/* OSXSAVE cleared by pv_featureset.  Fast-forward CR4 back in. */
+	if (cr4 & X86_CR4_OSXSAVE)
+		val__1cd |= cpufeat_mask(X86_FEATURE_OSXSAVE);
+
+	if (!masks)
+		masks = &cpuidmask_defaults;
 
 #define LAZY(msr, field)						\
 	({								\
-		if (unlikely(these_masks->field != masks->field) &&	\
+		val_##field |= masks->field;				\
+		if (unlikely(these_masks->field != val_##field) &&	\
 		    (msr))						\
 		{							\
-			wrmsrl((msr), masks->field);			\
-			these_masks->field = masks->field;		\
+			wrmsrl((msr), val_##field);			\
+			these_masks->field = val_##field;		\
 		}							\
 	})
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2733,6 +2733,7 @@ static int emulate_privileged_op(struct
         case 4: /* Write CR4 */
             v->arch.pv_vcpu.ctrlreg[4] = pv_guest_cr4_fixup(v, *reg);
             write_cr4(pv_guest_cr4_to_real_cr4(v));
+            ctxt_switch_levelling(currd);
             break;
 
         default:




[-- Attachment #2: x86-PV-OS-features.patch --]
[-- Type: text/plain, Size: 3815 bytes --]

x86/PV: don't hide CPUID.OSXSAVE from user mode

User mode code generally cannot be expected to invoke the PV-enabled
CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
(as well as even nowadays on levelling incapable hardware) such CPUID
invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
in other CPUID handling.

To make guest CR4 changes immediately visible via CPUID, also invoke
ctxt_switch_levelling() from the CR4 write path.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -206,17 +206,30 @@ static void __init noinline probe_maskin
 static void amd_ctxt_switch_levelling(const struct domain *nextd)
 {
 	struct cpuidmasks *these_masks = &this_cpu(cpuidmasks);
-	const struct cpuidmasks *masks =
-		(nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.cpuidmasks)
-		? nextd->arch.pv_domain.cpuidmasks : &cpuidmask_defaults;
+	const struct cpuidmasks *masks = NULL;
+	unsigned long cr4;
+	uint64_t val__1cd = 0, val_e1cd = 0, val__7ab0 = 0, val__6c = 0;
+
+	if (nextd && is_pv_domain(nextd) && !is_idle_domain(nextd)) {
+		cr4 = current->arch.pv_vcpu.ctrlreg[4];
+		masks = nextd->arch.pv_domain.cpuidmasks;
+	} else
+		cr4 = read_cr4();
+
+	if (cr4 & X86_CR4_OSXSAVE)
+		val__1cd |= (uint64_t)cpufeat_mask(X86_FEATURE_OSXSAVE) << 32;
+
+	if (!masks)
+		masks = &cpuidmask_defaults;
 
 #define LAZY(cap, msr, field)						\
 	({								\
-		if (unlikely(these_masks->field != masks->field) &&	\
+		val_##field |= masks->field;				\
+		if (unlikely(these_masks->field != val_##field) &&	\
 		    ((levelling_caps & cap) == cap))			\
 		{							\
-			wrmsr_amd(msr, masks->field);			\
-			these_masks->field = masks->field;		\
+			wrmsr_amd(msr, val_##field);			\
+			these_masks->field = val_##field;		\
 		}							\
 	})
 
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -154,7 +154,9 @@ static void __init probe_masking_msrs(vo
 static void intel_ctxt_switch_levelling(const struct domain *nextd)
 {
 	struct cpuidmasks *these_masks = &this_cpu(cpuidmasks);
-	const struct cpuidmasks *masks;
+	const struct cpuidmasks *masks = NULL;
+	unsigned long cr4;
+	uint64_t val__1cd = 0, val_e1cd = 0, val_Da1 = 0;
 
 	if (cpu_has_cpuid_faulting) {
 		/*
@@ -178,16 +180,27 @@ static void intel_ctxt_switch_levelling(
 		return;
 	}
 
-	masks = (nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.cpuidmasks)
-		? nextd->arch.pv_domain.cpuidmasks : &cpuidmask_defaults;
+	if (nextd && is_pv_domain(nextd) && !is_idle_domain(nextd)) {
+		cr4 = current->arch.pv_vcpu.ctrlreg[4];
+		masks = nextd->arch.pv_domain.cpuidmasks;
+	} else
+		cr4 = read_cr4();
+
+	/* OSXSAVE cleared by pv_featureset.  Fast-forward CR4 back in. */
+	if (cr4 & X86_CR4_OSXSAVE)
+		val__1cd |= cpufeat_mask(X86_FEATURE_OSXSAVE);
+
+	if (!masks)
+		masks = &cpuidmask_defaults;
 
 #define LAZY(msr, field)						\
 	({								\
-		if (unlikely(these_masks->field != masks->field) &&	\
+		val_##field |= masks->field;				\
+		if (unlikely(these_masks->field != val_##field) &&	\
 		    (msr))						\
 		{							\
-			wrmsrl((msr), masks->field);			\
-			these_masks->field = masks->field;		\
+			wrmsrl((msr), val_##field);			\
+			these_masks->field = val_##field;		\
 		}							\
 	})
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2733,6 +2733,7 @@ static int emulate_privileged_op(struct
         case 4: /* Write CR4 */
             v->arch.pv_vcpu.ctrlreg[4] = pv_guest_cr4_fixup(v, *reg);
             write_cr4(pv_guest_cr4_to_real_cr4(v));
+            ctxt_switch_levelling(currd);
             break;
 
         default:

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[Andrew Cooper]

On 16/08/16 16:20, Jan Beulich wrote:
> User mode code generally cannot be expected to invoke the PV-enabled
> CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
> (as well as even nowadays on levelling incapable hardware) such CPUID
> invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
> view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
> in other CPUID handling.

How does this work?  The OSXSAVE is a fast-forwarded bit, not a regular bit.

There is nothing you can do to control it on Intel, as the MSRs are
strictly and AND mask, applied before OSXSAVE and APIC are fast
forwarded from real hardware state.

On AMD, you can force it to zero by clearing the OSXSAVE bit, but you
can never cause it to appear set if Xen has it cleared in CR4.

~Andrew

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

Re: [PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[Jan Beulich]

>>> On 16.08.16 at 17:41, <andrew.cooper3@citrix.com> wrote:
> On 16/08/16 16:20, Jan Beulich wrote:
>> User mode code generally cannot be expected to invoke the PV-enabled
>> CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
>> (as well as even nowadays on levelling incapable hardware) such CPUID
>> invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
>> view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
>> in other CPUID handling.
> 
> How does this work?  The OSXSAVE is a fast-forwarded bit, not a regular bit.
> 
> There is nothing you can do to control it on Intel, as the MSRs are
> strictly and AND mask, applied before OSXSAVE and APIC are fast
> forwarded from real hardware state.

Considering that the change works (and things didn't work before) I
assume the AND-ing happens after the fast forwarding.

> On AMD, you can force it to zero by clearing the OSXSAVE bit, but you
> can never cause it to appear set if Xen has it cleared in CR4.

We don't allow guests to use XSAVE (and hence set the bit in CR4) if
we don't enable it ourselves. Hence if it's off in Xen, it'll be off
everywhere else (and that's what we want); i.e. in the consideration
of how this works, please assume CR4.OSXSAVE=1 for the raw
hardware reg.

Jan


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

Re: [PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[Andrew Cooper]

On 16/08/16 17:00, Jan Beulich wrote:
>>>> On 16.08.16 at 17:41, <andrew.cooper3@citrix.com> wrote:
>> On 16/08/16 16:20, Jan Beulich wrote:
>>> User mode code generally cannot be expected to invoke the PV-enabled
>>> CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
>>> (as well as even nowadays on levelling incapable hardware) such CPUID
>>> invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
>>> view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
>>> in other CPUID handling.
>> How does this work?  The OSXSAVE is a fast-forwarded bit, not a regular bit.
>>
>> There is nothing you can do to control it on Intel, as the MSRs are
>> strictly and AND mask, applied before OSXSAVE and APIC are fast
>> forwarded from real hardware state.
> Considering that the change works (and things didn't work before) I
> assume the AND-ing happens after the fast forwarding.

That is specifically contrary to my findings.  What hardware is this
on?  (Given the undocumented state of the rest of masking, I wouldn't be
surprised if it differed across models).

>
>> On AMD, you can force it to zero by clearing the OSXSAVE bit, but you
>> can never cause it to appear set if Xen has it cleared in CR4.
> We don't allow guests to use XSAVE (and hence set the bit in CR4) if
> we don't enable it ourselves. Hence if it's off in Xen, it'll be off
> everywhere else (and that's what we want); i.e. in the consideration
> of how this works, please assume CR4.OSXSAVE=1 for the raw
> hardware reg.

And I presume the usecase is to hide it from guest userspace if it is
not enabled in the guest kernel?

On the AMD side, this is a simple two-liner

/* Force OSXSAVE to zero if not enabled by the guest kernel. */
if (masks != &cpuidmask_defaults && !(current->arch.pv_vcpu.ctrlreg[4] &
X86_CR4_OSXSAVE))
    masks->_1cd &= ~cpufeat_mask(X86_FEATURE_OSXSAVE);

to counteract the default set up in update_domain_cpuid_info().

~Andrew

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

Re: [PATCH] x86/PV: don't hide CPUID.OSXSAVE from user mode

[Jan Beulich]

>>> On 16.08.16 at 18:13, <andrew.cooper3@citrix.com> wrote:
> On 16/08/16 17:00, Jan Beulich wrote:
>>>>> On 16.08.16 at 17:41, <andrew.cooper3@citrix.com> wrote:
>>> On 16/08/16 16:20, Jan Beulich wrote:
>>>> User mode code generally cannot be expected to invoke the PV-enabled
>>>> CPUID Xen supports, and prior to the CPUID levelling changes for 4.7
>>>> (as well as even nowadays on levelling incapable hardware) such CPUID
>>>> invocations actually saw the host CR4.OSXSAVE value. Fold in the guest
>>>> view of CR4.OSXSAVE when setting the levelling MSRs, just like we do
>>>> in other CPUID handling.
>>> How does this work?  The OSXSAVE is a fast-forwarded bit, not a regular bit.
>>>
>>> There is nothing you can do to control it on Intel, as the MSRs are
>>> strictly and AND mask, applied before OSXSAVE and APIC are fast
>>> forwarded from real hardware state.
>> Considering that the change works (and things didn't work before) I
>> assume the AND-ing happens after the fast forwarding.
> 
> That is specifically contrary to my findings.  What hardware is this
> on?  (Given the undocumented state of the rest of masking, I wouldn't be
> surprised if it differed across models).

Sandybridge.

>>> On AMD, you can force it to zero by clearing the OSXSAVE bit, but you
>>> can never cause it to appear set if Xen has it cleared in CR4.
>> We don't allow guests to use XSAVE (and hence set the bit in CR4) if
>> we don't enable it ourselves. Hence if it's off in Xen, it'll be off
>> everywhere else (and that's what we want); i.e. in the consideration
>> of how this works, please assume CR4.OSXSAVE=1 for the raw
>> hardware reg.
> 
> And I presume the usecase is to hide it from guest userspace if it is
> not enabled in the guest kernel?

It's actually both ways: Hide it when not enabled in the guest kernel
(I think that case already works, simply because without the patch
it's always hidden, and the patch sets the flag only when guest CR4
has it on) and expose it when the guest kernel has it enabled (that's
the case which didn't work so far, easily exposed by running the
x86emul test tool on a PV guest, e.g. Dom0).

> On the AMD side, this is a simple two-liner
> 
> /* Force OSXSAVE to zero if not enabled by the guest kernel. */
> if (masks != &cpuidmask_defaults && !(current->arch.pv_vcpu.ctrlreg[4] &
> X86_CR4_OSXSAVE))
>     masks->_1cd &= ~cpufeat_mask(X86_FEATURE_OSXSAVE);
> 
> to counteract the default set up in update_domain_cpuid_info().

Oh, indeed, I didn't notice update_domain_cpuid_info() inverting
the sense already for AMD. Exactly what you suggest above
won't work, but I'll have to modify the AMD case of the patch to
AND out the bit instead of OR-ing it in. I guess I'll wait with v2
until I can actually test it on my AMD box.

Jan


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