[PATCH 0/8] [RFC] SMAP handling improvements for 32bit PV guests on

[PATCH 0/8] [RFC] SMAP handling improvements for 32bit PV guests on

[Andrew Cooper]

The purpose of this series is to fix the fact that you cannot boot 32bit PV
guests older than Linux 3.8ish on Broadwell processors when Xen is using
SMAP.

It should be noted that the same problem exists with SMEP and 32bit PV guests,
but I have no sympathy for PV kernels which start executing code via user
mappings, or compunction to work around the issue.

This series is RFC because there are still outstanding tests to narrow down
whether a windows BSOD is caused by this series (unlikely I hope, as nothing
HVM-related has changed), or whether it is from recent changes in staging
(more likely, but still not obvious what).

Either way, the series is good enough for review so is presented here.

Andrew Cooper (8):
  common/vsprintf: Special-case DOMID_IDLE handling for %pv
  x86/traps: Avoid using current too early on boot
  x86/setup: Initialise CR4 before creating idle_vcpu[0]
  xen/x86: Clean up CR4 definitions
  xen/x86: Drop PSE from XEN_MINIMAL_CR4
  xen/x86: Calculate PV CR4 masks at boot
  xen/x86: Rework CR4 handling for PV guests
  xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

 docs/misc/xen-command-line.markdown |   25 +++++++-
 xen/arch/x86/domain.c               |  119 +++++++++++++++++++++++++++++++----
 xen/arch/x86/setup.c                |   59 +++++++++++------
 xen/arch/x86/traps.c                |   47 +++++++++-----
 xen/arch/x86/x86_64/traps.c         |    2 +-
 xen/common/vsprintf.c               |    5 +-
 xen/include/asm-x86/domain.h        |   19 +-----
 xen/include/asm-x86/processor.h     |   49 +++++++++------
 8 files changed, 239 insertions(+), 86 deletions(-)

[PATCH 1/8] common/vsprintf: Special-case DOMID_IDLE handling for %pv

[Andrew Cooper]

Prints dIDLEv0 as opposed to d32767v0

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/common/vsprintf.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 065cc42..ed76f24 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -338,7 +338,10 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
         ++*fmt_ptr;
         if ( str < end )
             *str = 'd';
-        str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
+        if ( unlikely(v->domain->domain_id == DOMID_IDLE) )
+            str = string(str + 1, end, "IDLE", -1, -1, 0);
+        else
+            str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
         if ( str < end )
             *str = 'v';
         return number(str + 1, end, v->vcpu_id, 10, -1, -1, 0);
-- 
1.7.10.4

[PATCH 2/8] x86/traps: Avoid using current too early on boot

[Andrew Cooper]

Early on boot, current has the sentinel value 0xfffff000.  Blindly using it in
show_registers() causes a nested failure and no useful information printed
from an early crash.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/x86_64/traps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 117a133..61bd053 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -86,7 +86,7 @@ void show_registers(const struct cpu_user_regs *regs)
     struct cpu_user_regs fault_regs = *regs;
     unsigned long fault_crs[8];
     enum context context;
-    struct vcpu *v = current;
+    struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
 
     if ( guest_mode(regs) && has_hvm_container_vcpu(v) )
     {
-- 
1.7.10.4

[PATCH 3/8] x86/setup: Initialise CR4 before creating idle_vcpu[0]

[Andrew Cooper]

PV vcpu initialise has CR4 seeded from mmu_cr4_features.  Adjust the order of
basic CR4 setup and creation of the idle domain, such that idle_vcpu[0] is not
wildly different from the other idle vcpus.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/setup.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c32e49f..ff34670 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1284,20 +1284,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     timer_init();
 
-    init_idle_domain();
-
-    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
-                                           &this_cpu(stubs).mfn);
-    BUG_ON(!this_cpu(stubs.addr));
-
-    trap_init();
-
-    rcu_init();
-    
-    early_time_init();
-
-    arch_init_memory();
-
     identify_cpu(&boot_cpu_data);
 
     if ( cpu_has_fxsr )
@@ -1318,6 +1304,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( cpu_has_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
 
+    init_idle_domain();
+
+    this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
+                                           &this_cpu(stubs).mfn);
+    BUG_ON(!this_cpu(stubs.addr));
+
+    trap_init();
+
+    rcu_init();
+
+    early_time_init();
+
+    arch_init_memory();
+
     alternative_instructions();
 
     local_irq_enable();
-- 
1.7.10.4

[PATCH 4/8] xen/x86: Clean up CR4 definitions

[Andrew Cooper]

And add PKE as it is architecturally defined now.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/processor.h |   37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index cb36f0c..0255ee6 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -71,24 +71,25 @@
 /*
  * Intel CPU features in CR4
  */
-#define X86_CR4_VME		0x0001	/* enable vm86 extensions */
-#define X86_CR4_PVI		0x0002	/* virtual interrupts flag enable */
-#define X86_CR4_TSD		0x0004	/* disable time stamp at ipl 3 */
-#define X86_CR4_DE		0x0008	/* enable debugging extensions */
-#define X86_CR4_PSE		0x0010	/* enable page size extensions */
-#define X86_CR4_PAE		0x0020	/* enable physical address extensions */
-#define X86_CR4_MCE		0x0040	/* Machine check enable */
-#define X86_CR4_PGE		0x0080	/* enable global pages */
-#define X86_CR4_PCE		0x0100	/* enable performance counters at ipl 3 */
-#define X86_CR4_OSFXSR		0x0200	/* enable fast FPU save and restore */
-#define X86_CR4_OSXMMEXCPT	0x0400	/* enable unmasked SSE exceptions */
-#define X86_CR4_VMXE		0x2000  /* enable VMX */
-#define X86_CR4_SMXE		0x4000  /* enable SMX */
-#define X86_CR4_FSGSBASE	0x10000 /* enable {rd,wr}{fs,gs}base */
-#define X86_CR4_PCIDE		0x20000 /* enable PCID */
-#define X86_CR4_OSXSAVE	0x40000 /* enable XSAVE/XRSTOR */
-#define X86_CR4_SMEP		0x100000/* enable SMEP */
-#define X86_CR4_SMAP		0x200000/* enable SMAP */
+#define X86_CR4_VME             0x00000001 /* enable vm86 extensions */
+#define X86_CR4_PVI             0x00000002 /* virtual interrupts flag enable */
+#define X86_CR4_TSD             0x00000004 /* disable time stamp at ipl 3 */
+#define X86_CR4_DE              0x00000008 /* enable debugging extensions */
+#define X86_CR4_PSE             0x00000010 /* enable page size extensions */
+#define X86_CR4_PAE             0x00000020 /* enable physical address extensions */
+#define X86_CR4_MCE             0x00000040 /* Machine check enable */
+#define X86_CR4_PGE             0x00000080 /* enable global pages */
+#define X86_CR4_PCE             0x00000100 /* enable performance counters at ipl 3 */
+#define X86_CR4_OSFXSR          0x00000200 /* enable fast FPU save and restore */
+#define X86_CR4_OSXMMEXCPT      0x00000400 /* enable unmasked SSE exceptions */
+#define X86_CR4_VMXE            0x00002000 /* enable VMX */
+#define X86_CR4_SMXE            0x00004000 /* enable SMX */
+#define X86_CR4_FSGSBASE        0x00010000 /* enable {rd,wr}{fs,gs}base */
+#define X86_CR4_PCIDE           0x00020000 /* enable PCID */
+#define X86_CR4_OSXSAVE         0x00040000 /* enable XSAVE/XRSTOR */
+#define X86_CR4_SMEP            0x00100000 /* enable SMEP */
+#define X86_CR4_SMAP            0x00200000 /* enable SMAP */
+#define X86_CR4_PKE             0x00400000 /* enable PKE */
 
 /*
  * Trap/fault mnemonics.
-- 
1.7.10.4

[PATCH 5/8] xen/x86: Drop PSE from XEN_MINIMAL_CR4

[Andrew Cooper]

CR4.PSE is ignored if CR4.PAE is set, which will unconditionally will be.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/processor.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 0255ee6..5fb340f 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -143,7 +143,7 @@
 #define PFEC_page_paged     (1U<<5)
 #define PFEC_page_shared    (1U<<6)
 
-#define XEN_MINIMAL_CR4 (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE)
+#define XEN_MINIMAL_CR4 (X86_CR4_PGE | X86_CR4_PAE)
 
 #define XEN_SYSCALL_MASK (X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|    \
                           X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
-- 
1.7.10.4

[PATCH 6/8] xen/x86: Calculate PV CR4 masks at boot

[Andrew Cooper]

... rather than on every time a guest sets CR4.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domain.c |   41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ba28f38..f805724 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -682,24 +682,47 @@ void arch_domain_unpause(struct domain *d)
         viridian_time_ref_count_thaw(d);
 }
 
-unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
+/*
+ * These are the masks of CR4 bits (subject to hardware availability) which a
+ * PV guest may not legitimiately attempt to modify.
+ */
+static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
+
+static int __init init_pv_cr4_masks(void)
 {
-    unsigned long hv_cr4_mask, hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
+    unsigned long common_mask = ~X86_CR4_TSD;
 
-    hv_cr4_mask = ~X86_CR4_TSD;
+    /*
+     * All PV guests may attempt to modify TSD, DE and OSXSAVE.
+     */
     if ( cpu_has_de )
-        hv_cr4_mask &= ~X86_CR4_DE;
-    if ( cpu_has_fsgsbase && !is_pv_32bit_domain(v->domain) )
-        hv_cr4_mask &= ~X86_CR4_FSGSBASE;
+        common_mask &= ~X86_CR4_DE;
     if ( cpu_has_xsave )
-        hv_cr4_mask &= ~X86_CR4_OSXSAVE;
+        common_mask &= ~X86_CR4_OSXSAVE;
+
+    pv_cr4_mask = compat_pv_cr4_mask = common_mask;
+
+    /*
+     * 64bit PV guests may attempt to modify FSGSBASE.
+     */
+    if ( cpu_has_fsgsbase )
+        pv_cr4_mask &= ~X86_CR4_FSGSBASE;
+
+    return 0;
+}
+__initcall(init_pv_cr4_masks);
+
+unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
+{
+    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
+    unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
 
-    if ( (guest_cr4 & hv_cr4_mask) != (hv_cr4 & hv_cr4_mask) )
+    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
         printk(XENLOG_G_WARNING
                "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
                current->domain->domain_id, v, hv_cr4, guest_cr4);
 
-    return (hv_cr4 & hv_cr4_mask) | (guest_cr4 & ~hv_cr4_mask);
+    return (hv_cr4 & mask) | (guest_cr4 & ~mask);
 }
 
 #define xen_vcpu_guest_context vcpu_guest_context
-- 
1.7.10.4

Re: [PATCH 6/8] xen/x86: Calculate PV CR4 masks at boot

[Jan Beulich]

>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -682,24 +682,47 @@ void arch_domain_unpause(struct domain *d)
>          viridian_time_ref_count_thaw(d);
>  }
>  
> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
> +/*
> + * These are the masks of CR4 bits (subject to hardware availability) which a
> + * PV guest may not legitimiately attempt to modify.
> + */
> +static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;

The patch generally being fine, I still wonder why you chose to use
"pv" in the names instead of the previous "hv": To me, the latter
makes more sense: "the bits the hypervisor controls" instead of "the
bits pv guests do not control".

Jan

Re: [PATCH 6/8] xen/x86: Calculate PV CR4 masks at boot

[Andrew Cooper]

On 25/06/15 14:08, Jan Beulich wrote:
>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -682,24 +682,47 @@ void arch_domain_unpause(struct domain *d)
>>          viridian_time_ref_count_thaw(d);
>>  }
>>  
>> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
>> +/*
>> + * These are the masks of CR4 bits (subject to hardware availability) which a
>> + * PV guest may not legitimiately attempt to modify.
>> + */
>> +static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
> The patch generally being fine, I still wonder why you chose to use
> "pv" in the names instead of the previous "hv": To me, the latter
> makes more sense: "the bits the hypervisor controls" instead of "the
> bits pv guests do not control".

It is the set of bits Xen doesn't mind the guest attempting to modify,
which is specifically different from the bits Xen actually controls, and
different from the set of bits shadowed in a guests CR4.

The masks do represent a superset of the shadowed bits, (clamped by
hardware support).  Bits such as PGE and FSGSBASE are deemed ok for a
guest to attempt to modify, but are not shadowed and the guests
interests are completely ignored.

~Andrew

Re: [PATCH 6/8] xen/x86: Calculate PV CR4 masks at boot

[Jan Beulich]

>>> On 25.06.15 at 15:31, <andrew.cooper3@citrix.com> wrote:
> On 25/06/15 14:08, Jan Beulich wrote:
>>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -682,24 +682,47 @@ void arch_domain_unpause(struct domain *d)
>>>          viridian_time_ref_count_thaw(d);
>>>  }
>>>  
>>> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
>>> +/*
>>> + * These are the masks of CR4 bits (subject to hardware availability) which a
>>> + * PV guest may not legitimiately attempt to modify.
>>> + */
>>> +static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
>> The patch generally being fine, I still wonder why you chose to use
>> "pv" in the names instead of the previous "hv": To me, the latter
>> makes more sense: "the bits the hypervisor controls" instead of "the
>> bits pv guests do not control".
> 
> It is the set of bits Xen doesn't mind the guest attempting to modify,

It's the inverse of that set of bits really, isn't it?

Jan

> which is specifically different from the bits Xen actually controls, and
> different from the set of bits shadowed in a guests CR4.
> 
> The masks do represent a superset of the shadowed bits, (clamped by
> hardware support).  Bits such as PGE and FSGSBASE are deemed ok for a
> guest to attempt to modify, but are not shadowed and the guests
> interests are completely ignored.
> 
> ~Andrew

Re: [PATCH 6/8] xen/x86: Calculate PV CR4 masks at boot

[Andrew Cooper]

On 25/06/15 14:40, Jan Beulich wrote:
>>>> On 25.06.15 at 15:31, <andrew.cooper3@citrix.com> wrote:
>> On 25/06/15 14:08, Jan Beulich wrote:
>>>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -682,24 +682,47 @@ void arch_domain_unpause(struct domain *d)
>>>>          viridian_time_ref_count_thaw(d);
>>>>  }
>>>>  
>>>> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
>>>> +/*
>>>> + * These are the masks of CR4 bits (subject to hardware availability) which a
>>>> + * PV guest may not legitimiately attempt to modify.
>>>> + */
>>>> +static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
>>> The patch generally being fine, I still wonder why you chose to use
>>> "pv" in the names instead of the previous "hv": To me, the latter
>>> makes more sense: "the bits the hypervisor controls" instead of "the
>>> bits pv guests do not control".
>> It is the set of bits Xen doesn't mind the guest attempting to modify,
> It's the inverse of that set of bits really, isn't it?

Correct, given the way it is used.

~Andrew

>
> Jan
>
>> which is specifically different from the bits Xen actually controls, and
>> different from the set of bits shadowed in a guests CR4.
>>
>> The masks do represent a superset of the shadowed bits, (clamped by
>> hardware support).  Bits such as PGE and FSGSBASE are deemed ok for a
>> guest to attempt to modify, but are not shadowed and the guests
>> interests are completely ignored.
>>
>> ~Andrew
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

[PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Andrew Cooper]

PV CR4 settings are now based on mmu_cr4_features, rather than the current
contents of CR4.  This causes dom0 to be consistent with domUs, despite being
constructed in a region with CR4.SMAP purposefully disabled.

The major change here is that v->arch.pv_vcpu.ctrlreg[4] now contains only CR4
bits which Xen wishes to shadow, rather than containing a mix of host and
guest bits.  This is likely to elicit a warning when migrating a PV guest from
an older version of Xen, although no function problems as a result.

A second change is that visible set of CR4 bits is different.  In particular,
VMXE and MCE are not leaked into a guest, while SMAP and SMEP are exposed.

In addition, add PGE to the set of bits we don't care about a guest attempting
to modify.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domain.c        |   79 +++++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/traps.c         |    4 +--
 xen/include/asm-x86/domain.h |   19 ++--------
 3 files changed, 75 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f805724..18c3c62 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -479,7 +479,7 @@ int vcpu_initialise(struct vcpu *v)
         v->arch.cr3           = __pa(idle_pg_table);
     }
 
-    v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+    pv_cr4_write(v, 0);
 
     rc = is_pv_32on64_domain(d) ? setup_compat_l4(v) : 0;
  done:
@@ -683,6 +683,26 @@ void arch_domain_unpause(struct domain *d)
 }
 
 /*
+ * CR4 bits Xen will shadow on behalf of the guest.
+ *  - TSD for vtsc
+ *  - DE for %%dr4/5 emulation
+ *  - OSXSAVE for xsetbv emulation
+ */
+#define PV_CR4_SHADOW                           \
+    (X86_CR4_TSD | X86_CR4_DE | X86_CR4_OSXSAVE)
+
+/* CR4 bits a guest controls. */
+#define PV_CR4_GUEST (X86_CR4_TSD)
+
+/*
+ * Host CR4 bits which a guest may observe.  A guest always observes its own
+ * settings for the shadowed bits, irrespective of host settings.
+ */
+#define PV_CR4_READ                                                     \
+    (X86_CR4_PAE | X86_CR4_PGE |X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT |   \
+     X86_CR4_FSGSBASE | X86_CR4_SMEP | X86_CR4_SMAP)
+
+/*
  * These are the masks of CR4 bits (subject to hardware availability) which a
  * PV guest may not legitimiately attempt to modify.
  */
@@ -699,6 +719,8 @@ static int __init init_pv_cr4_masks(void)
         common_mask &= ~X86_CR4_DE;
     if ( cpu_has_xsave )
         common_mask &= ~X86_CR4_OSXSAVE;
+    if ( cpu_has_pge )
+        common_mask &= ~X86_CR4_PGE;
 
     pv_cr4_mask = compat_pv_cr4_mask = common_mask;
 
@@ -708,21 +730,61 @@ static int __init init_pv_cr4_masks(void)
     if ( cpu_has_fsgsbase )
         pv_cr4_mask &= ~X86_CR4_FSGSBASE;
 
+    BUILD_BUG_ON(PV_CR4_SHADOW & PV_CR4_READ);
+
     return 0;
 }
 __initcall(init_pv_cr4_masks);
 
-unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
+void pv_cr4_write(struct vcpu *v, unsigned long guest_cr4)
 {
-    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
+    unsigned long host_cr4 = mmu_cr4_features & (PV_CR4_READ | PV_CR4_SHADOW);
     unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
 
-    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
+    if ( guest_cr4 == 0 )
+    {
+        /* Default - all shadowed bits to 0. */
+        guest_cr4 = mmu_cr4_features & ~PV_CR4_SHADOW;
+    }
+    else if ( (guest_cr4 & ~PV_CR4_SHADOW) == 0 )
+    {
+        /*
+         * Only setting shadowed bits.  This will be the case on restore, so
+         * avoid warning about losing readable host bits.
+         */
+    }
+    else if ( (guest_cr4 & mask) != (host_cr4 & mask) )
+    {
         printk(XENLOG_G_WARNING
-               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
-               current->domain->domain_id, v, hv_cr4, guest_cr4);
+               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx "
+               "(bad +%#lx, -%#lx)\n",
+               current->domain->domain_id, v, host_cr4, guest_cr4,
+               (host_cr4 ^ guest_cr4) & guest_cr4 & mask,
+               (host_cr4 ^ guest_cr4) & ~guest_cr4 & mask);
+    }
+
+    v->arch.pv_vcpu.ctrlreg[4] =
+        ((mmu_cr4_features & mask) | (guest_cr4 & ~mask)) & PV_CR4_SHADOW;
+}
+
+unsigned long pv_cr4_read(const struct vcpu *v)
+{
+    unsigned long cr4 = read_cr4();
+
+    return (cr4 & PV_CR4_READ & ~PV_CR4_SHADOW) |
+        (v->arch.pv_vcpu.ctrlreg[4] & PV_CR4_SHADOW);
+}
+
+unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v)
+{
+    unsigned long cr4 =
+        (mmu_cr4_features & ~PV_CR4_GUEST) |
+        (v->arch.pv_vcpu.ctrlreg[4] & PV_CR4_GUEST);
+
+    if ( v->domain->arch.vtsc )
+        cr4 |= X86_CR4_TSD;
 
-    return (hv_cr4 & mask) | (guest_cr4 & ~mask);
+    return cr4;
 }
 
 #define xen_vcpu_guest_context vcpu_guest_context
@@ -935,8 +997,7 @@ int arch_set_info_guest(
     v->arch.pv_vcpu.ctrlreg[0] |= read_cr0() & ~X86_CR0_TS;
 
     cr4 = v->arch.pv_vcpu.ctrlreg[4];
-    v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
-        real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+    pv_cr4_write(v, cr4);
 
     memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
     for ( i = 0; i < 8; i++ )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c634008..1ab870d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2402,7 +2402,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
         break;
 
         case 4: /* Read CR4 */
-            *reg = v->arch.pv_vcpu.ctrlreg[4];
+            *reg = pv_cr4_read(v);
             break;
 
         default:
@@ -2476,7 +2476,7 @@ static int emulate_privileged_op(struct cpu_user_regs *regs)
         }
 
         case 4: /* Write CR4 */
-            v->arch.pv_vcpu.ctrlreg[4] = pv_guest_cr4_fixup(v, *reg);
+            pv_cr4_write(v, *reg);
             write_cr4(pv_guest_cr4_to_real_cr4(v));
             break;
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 5eb6832..963071b 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -527,22 +527,9 @@ bool_t update_secondary_system_time(struct vcpu *,
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
-/* Clean up CR4 bits that are not under guest control. */
-unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
-
-/* Convert between guest-visible and real CR4 values. */
-#define pv_guest_cr4_to_real_cr4(v)                         \
-    (((v)->arch.pv_vcpu.ctrlreg[4]                          \
-      | (mmu_cr4_features                                   \
-         & (X86_CR4_PGE | X86_CR4_PSE | X86_CR4_SMEP |      \
-            X86_CR4_SMAP | X86_CR4_OSXSAVE |                \
-            X86_CR4_FSGSBASE))                              \
-      | ((v)->domain->arch.vtsc ? X86_CR4_TSD : 0))         \
-     & ~X86_CR4_DE)
-#define real_cr4_to_pv_guest_cr4(c)                         \
-    ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
-             X86_CR4_OSXSAVE | X86_CR4_SMEP |               \
-             X86_CR4_FSGSBASE | X86_CR4_SMAP))
+void pv_cr4_write(struct vcpu *v, unsigned long guest_cr4);
+unsigned long pv_cr4_read(const struct vcpu *v);
+unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
 
 void domain_cpuid(struct domain *d,
                   unsigned int  input,
-- 
1.7.10.4

Re: [PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Jan Beulich]

>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
> PV CR4 settings are now based on mmu_cr4_features, rather than the current
> contents of CR4.  This causes dom0 to be consistent with domUs, despite 
> being
> constructed in a region with CR4.SMAP purposefully disabled.

That'll be fine as long as we're keeping CR4 stable (except for
guest controlled bits). Agreed, once that's no longer the case,
mmu_cr4_features would no longer be very useful either.
Still it would seem more reasonable to me to base things on
current values than on some not guaranteed to be up to date.

> The major change here is that v->arch.pv_vcpu.ctrlreg[4] now contains only CR4
> bits which Xen wishes to shadow, rather than containing a mix of host and
> guest bits.  This is likely to elicit a warning when migrating a PV guest 
> from
> an older version of Xen, although no function problems as a result.

For one, I'm having a hard time guessing what the part of the sentence
after the comma is meant to tell me. And then, from a support
perspective, such warnings aren't helpful, as they incur questions. Plus
finally, storing neither the current guest view nor the current hypervisor
view in that field seems prone to cause headaches in the future. Otoh
this gets it in line with CR0 handling as it looks.

> A second change is that visible set of CR4 bits is different.  In 
> particular,
> VMXE and MCE are not leaked into a guest, while SMAP and SMEP are exposed.

Not leaking VMXE into the guest is fine. Not exposing MCE even to
Dom0 is questionable. And exposing SMEP and SMAP without
exposing their CPUID flags seems bogus.

> In addition, add PGE to the set of bits we don't care about a guest 
> attempting to modify.

I agree from a functionality pov this is okay, but do we really want
to allow the guest to turn this off from a performance pov?

> @@ -683,6 +683,26 @@ void arch_domain_unpause(struct domain *d)
>  }
>  
>  /*
> + * CR4 bits Xen will shadow on behalf of the guest.
> + *  - TSD for vtsc
> + *  - DE for %%dr4/5 emulation

DR4/5 handling is surely secondary here: I/O breakpoints are the
main feature to be controlled by this bit.

> @@ -699,6 +719,8 @@ static int __init init_pv_cr4_masks(void)
>          common_mask &= ~X86_CR4_DE;
>      if ( cpu_has_xsave )
>          common_mask &= ~X86_CR4_OSXSAVE;
> +    if ( cpu_has_pge )
> +        common_mask &= ~X86_CR4_PGE;

Following the earlier comment, shouldn't this bit then also be added
to PV_CR4_SHADOW?

> @@ -708,21 +730,61 @@ static int __init init_pv_cr4_masks(void)
>      if ( cpu_has_fsgsbase )
>          pv_cr4_mask &= ~X86_CR4_FSGSBASE;
>  
> +    BUILD_BUG_ON(PV_CR4_SHADOW & PV_CR4_READ);
> +
>      return 0;
>  }
>  __initcall(init_pv_cr4_masks);
>  
> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
> +void pv_cr4_write(struct vcpu *v, unsigned long guest_cr4)
>  {
> -    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
> +    unsigned long host_cr4 = mmu_cr4_features & (PV_CR4_READ | PV_CR4_SHADOW);
>      unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
>  
> -    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
> +    if ( guest_cr4 == 0 )
> +    {
> +        /* Default - all shadowed bits to 0. */
> +        guest_cr4 = mmu_cr4_features & ~PV_CR4_SHADOW;
> +    }
> +    else if ( (guest_cr4 & ~PV_CR4_SHADOW) == 0 )
> +    {
> +        /*
> +         * Only setting shadowed bits.  This will be the case on restore, so
> +         * avoid warning about losing readable host bits.
> +         */
> +    }
> +    else if ( (guest_cr4 & mask) != (host_cr4 & mask) )
> +    {
>          printk(XENLOG_G_WARNING
> -               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
> -               current->domain->domain_id, v, hv_cr4, guest_cr4);
> +               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx "
> +               "(bad +%#lx, -%#lx)\n",

No mixture of 0x-prefixed and not-0x-prefixed hex numbers please.
I anyway wonder whether printing four numbers here is necessary
when printing just three (host_cr4, guest_cr4, and mask) would
suffice.

> +               current->domain->domain_id, v, host_cr4, guest_cr4,
> +               (host_cr4 ^ guest_cr4) & guest_cr4 & mask,
> +               (host_cr4 ^ guest_cr4) & ~guest_cr4 & mask);
> +    }
> +
> +    v->arch.pv_vcpu.ctrlreg[4] =
> +        ((mmu_cr4_features & mask) | (guest_cr4 & ~mask)) & PV_CR4_SHADOW;

With FSGSBASE not in PV_CR4_SHADOW, how would (as the comment
in init_pv_cr4_masks() says) the guest manage to observe the flag in
its desired state after a migration?

> +unsigned long pv_cr4_read(const struct vcpu *v)
> +{
> +    unsigned long cr4 = read_cr4();
> +
> +    return (cr4 & PV_CR4_READ & ~PV_CR4_SHADOW) |
> +        (v->arch.pv_vcpu.ctrlreg[4] & PV_CR4_SHADOW);

I don't understand either of the two uses of PV_CR4_SHADOW here:
The first seems pointless as earlier an you already assert that it
and PV_CR4_READ are disjoint, i.e. the negation of the former
surely has no less bits set than the latter. And the second seems
pointless since you only ever store bits in PV_CR4_SHADOW into the
field.

Jan

Re: [PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Andrew Cooper]

On 25/06/15 15:41, Jan Beulich wrote:
>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>> PV CR4 settings are now based on mmu_cr4_features, rather than the current
>> contents of CR4.  This causes dom0 to be consistent with domUs, despite 
>> being
>> constructed in a region with CR4.SMAP purposefully disabled.
> That'll be fine as long as we're keeping CR4 stable (except for
> guest controlled bits). Agreed, once that's no longer the case,
> mmu_cr4_features would no longer be very useful either.
> Still it would seem more reasonable to me to base things on
> current values than on some not guaranteed to be up to date.

mmu_cr4_features is supposed to be the runtime value in cr4.

At any point, SMAP/SMEP/PGE might be temporarily disabled, which is why
I avoided using the current value of cr4.

>
>> The major change here is that v->arch.pv_vcpu.ctrlreg[4] now contains only CR4
>> bits which Xen wishes to shadow, rather than containing a mix of host and
>> guest bits.  This is likely to elicit a warning when migrating a PV guest 
>> from
>> an older version of Xen, although no function problems as a result.
> For one, I'm having a hard time guessing what the part of the sentence
> after the comma is meant to tell me.

Before this patch, v->arch.pv_vcpu.ctrlreg[4] contained an unqualified
combination of host and guest controlled bits.

After this patch, it strictly contains the bits Xen chooses to shadow.

> And then, from a support
> perspective, such warnings aren't helpful, as they incur questions.

It was useful for debugging, but can see your point.  I could reword it
to "ignoring $FOO's attempt to update $BAR's CR4".  I would hesitate
from removing it completely however.

> Plus
> finally, storing neither the current guest view nor the current hypervisor
> view in that field seems prone to cause headaches in the future. Otoh
> this gets it in line with CR0 handling as it looks.

My logic was that the set of shadowed bits will never decrease on newer
versions of Xen, but Xen's choice of CR4 bits may change.  As a result,
neither "guests view" nor "hypervisors view" is appropriate in an
upgrade case.

>
>> A second change is that visible set of CR4 bits is different.  In 
>> particular,
>> VMXE and MCE are not leaked into a guest, while SMAP and SMEP are exposed.
> Not leaking VMXE into the guest is fine. Not exposing MCE even to
> Dom0 is questionable.

I would prefer not to special case the hardware domain if possible, and
I suppose MCE is not too much of an issue to leak.  We already are,
after all.

> And exposing SMEP and SMAP without
> exposing their CPUID flags seems bogus.

I was hoping not to get bogged down in CPUID given my upcoming work to
fix feature levelling.

After recent consideration, I am still not sure if we want to support
SMAP in 32bit PV guests or not.  The trapping of stac/clac would be a
high overhead, although the guest could modify EFLAGS.AC itself using popf.

>
>> In addition, add PGE to the set of bits we don't care about a guest 
>> attempting to modify.
> I agree from a functionality pov this is okay, but do we really want
> to allow the guest to turn this off from a performance pov?

This change doesn't shadow PGE.  It just prevents the warning from
firing if the guest attempts to clear PGE.

>
>> @@ -683,6 +683,26 @@ void arch_domain_unpause(struct domain *d)
>>  }
>>  
>>  /*
>> + * CR4 bits Xen will shadow on behalf of the guest.
>> + *  - TSD for vtsc
>> + *  - DE for %%dr4/5 emulation
> DR4/5 handling is surely secondary here: I/O breakpoints are the
> main feature to be controlled by this bit.

True - I will expand the comment.

>
>> @@ -699,6 +719,8 @@ static int __init init_pv_cr4_masks(void)
>>          common_mask &= ~X86_CR4_DE;
>>      if ( cpu_has_xsave )
>>          common_mask &= ~X86_CR4_OSXSAVE;
>> +    if ( cpu_has_pge )
>> +        common_mask &= ~X86_CR4_PGE;
> Following the earlier comment, shouldn't this bit then also be added
> to PV_CR4_SHADOW?

I don't think so.  A guest has to defer to Xen to perform TLB flushes. 
I just wished to avoid the warning.

>
>> @@ -708,21 +730,61 @@ static int __init init_pv_cr4_masks(void)
>>      if ( cpu_has_fsgsbase )
>>          pv_cr4_mask &= ~X86_CR4_FSGSBASE;
>>  
>> +    BUILD_BUG_ON(PV_CR4_SHADOW & PV_CR4_READ);
>> +
>>      return 0;
>>  }
>>  __initcall(init_pv_cr4_masks);
>>  
>> -unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
>> +void pv_cr4_write(struct vcpu *v, unsigned long guest_cr4)
>>  {
>> -    unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
>> +    unsigned long host_cr4 = mmu_cr4_features & (PV_CR4_READ | PV_CR4_SHADOW);
>>      unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
>>  
>> -    if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
>> +    if ( guest_cr4 == 0 )
>> +    {
>> +        /* Default - all shadowed bits to 0. */
>> +        guest_cr4 = mmu_cr4_features & ~PV_CR4_SHADOW;
>> +    }
>> +    else if ( (guest_cr4 & ~PV_CR4_SHADOW) == 0 )
>> +    {
>> +        /*
>> +         * Only setting shadowed bits.  This will be the case on restore, so
>> +         * avoid warning about losing readable host bits.
>> +         */
>> +    }
>> +    else if ( (guest_cr4 & mask) != (host_cr4 & mask) )
>> +    {
>>          printk(XENLOG_G_WARNING
>> -               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
>> -               current->domain->domain_id, v, hv_cr4, guest_cr4);
>> +               "d%d attempted to change %pv's CR4 flags %08lx -> %08lx "
>> +               "(bad +%#lx, -%#lx)\n",
> No mixture of 0x-prefixed and not-0x-prefixed hex numbers please.

Oops yes.

> I anyway wonder whether printing four numbers here is necessary
> when printing just three (host_cr4, guest_cr4, and mask) would
> suffice.

It is substantially harder to read.

>
>> +               current->domain->domain_id, v, host_cr4, guest_cr4,
>> +               (host_cr4 ^ guest_cr4) & guest_cr4 & mask,
>> +               (host_cr4 ^ guest_cr4) & ~guest_cr4 & mask);
>> +    }
>> +
>> +    v->arch.pv_vcpu.ctrlreg[4] =
>> +        ((mmu_cr4_features & mask) | (guest_cr4 & ~mask)) & PV_CR4_SHADOW;
> With FSGSBASE not in PV_CR4_SHADOW, how would (as the comment
> in init_pv_cr4_masks() says) the guest manage to observe the flag in
> its desired state after a migration?

The guest does not get any control of FSGSBASE.  Xen unilaterally has it
enabled, and uses them on vcpu context switch so it is not safe to ever
disable.

>
>> +unsigned long pv_cr4_read(const struct vcpu *v)
>> +{
>> +    unsigned long cr4 = read_cr4();
>> +
>> +    return (cr4 & PV_CR4_READ & ~PV_CR4_SHADOW) |
>> +        (v->arch.pv_vcpu.ctrlreg[4] & PV_CR4_SHADOW);
> I don't understand either of the two uses of PV_CR4_SHADOW here:
> The first seems pointless as earlier an you already assert that it
> and PV_CR4_READ are disjoint, i.e. the negation of the former
> surely has no less bits set than the latter. And the second seems
> pointless since you only ever store bits in PV_CR4_SHADOW into the
> field.

This bit of code has failed to change over the development of the
series.  I will adjust it.

~Andrew

Re: [PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Jan Beulich]

>>> On 25.06.15 at 18:22, <andrew.cooper3@citrix.com> wrote:
> On 25/06/15 15:41, Jan Beulich wrote:
>>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>>> The major change here is that v->arch.pv_vcpu.ctrlreg[4] now contains only CR4
>>> bits which Xen wishes to shadow, rather than containing a mix of host and
>>> guest bits.  This is likely to elicit a warning when migrating a PV guest 
>>> from
>>> an older version of Xen, although no function problems as a result.
>> For one, I'm having a hard time guessing what the part of the sentence
>> after the comma is meant to tell me.
> 
> Before this patch, v->arch.pv_vcpu.ctrlreg[4] contained an unqualified
> combination of host and guest controlled bits.
> 
> After this patch, it strictly contains the bits Xen chooses to shadow.

With this you're re-stating what the first sentence said after its comma.
The question was on the second one though.

>> And then, from a support
>> perspective, such warnings aren't helpful, as they incur questions.
> 
> It was useful for debugging, but can see your point.  I could reword it
> to "ignoring $FOO's attempt to update $BAR's CR4".  I would hesitate
> from removing it completely however.

Yes, the warning in general should stay of course (there is one
after all right now). I realize I slightly mis-read the sentence
though - looks like the implication is that the possible warning is
from the mixture of bits previously in .ctrlreg[4]? Could we flag
the write originating from a migration, issuing the warning only
for guest initiated updates?

>> Plus
>> finally, storing neither the current guest view nor the current hypervisor
>> view in that field seems prone to cause headaches in the future. Otoh
>> this gets it in line with CR0 handling as it looks.
> 
> My logic was that the set of shadowed bits will never decrease on newer
> versions of Xen, but Xen's choice of CR4 bits may change.  As a result,
> neither "guests view" nor "hypervisors view" is appropriate in an
> upgrade case.

Okay, namely together with getting it in line with CR0 handling this
makes sense.

>>> A second change is that visible set of CR4 bits is different.  In 
>>> particular,
>>> VMXE and MCE are not leaked into a guest, while SMAP and SMEP are exposed.
>> Not leaking VMXE into the guest is fine. Not exposing MCE even to
>> Dom0 is questionable.
> 
> I would prefer not to special case the hardware domain if possible, and
> I suppose MCE is not too much of an issue to leak.  We already are,
> after all.
> 
>> And exposing SMEP and SMAP without
>> exposing their CPUID flags seems bogus.
> 
> I was hoping not to get bogged down in CPUID given my upcoming work to
> fix feature levelling.
> 
> After recent consideration, I am still not sure if we want to support
> SMAP in 32bit PV guests or not.  The trapping of stac/clac would be a
> high overhead, although the guest could modify EFLAGS.AC itself using popf.

If it technically works (which it looks like it does) I see no reason
leaving the decision to guests.

>>> In addition, add PGE to the set of bits we don't care about a guest 
>>> attempting to modify.
>> I agree from a functionality pov this is okay, but do we really want
>> to allow the guest to turn this off from a performance pov?
> 
> This change doesn't shadow PGE.  It just prevents the warning from
> firing if the guest attempts to clear PGE.

I which case I think the wording you chose is slightly misleading.

>>> @@ -699,6 +719,8 @@ static int __init init_pv_cr4_masks(void)
>>>          common_mask &= ~X86_CR4_DE;
>>>      if ( cpu_has_xsave )
>>>          common_mask &= ~X86_CR4_OSXSAVE;
>>> +    if ( cpu_has_pge )
>>> +        common_mask &= ~X86_CR4_PGE;
>> Following the earlier comment, shouldn't this bit then also be added
>> to PV_CR4_SHADOW?
> 
> I don't think so.  A guest has to defer to Xen to perform TLB flushes. 
> I just wished to avoid the warning.

Maybe add a comment to that effect then, avoiding future readers
to feel tempted to correct this apparent inconsistency, just like it
happened to me?

>> With FSGSBASE not in PV_CR4_SHADOW, how would (as the comment
>> in init_pv_cr4_masks() says) the guest manage to observe the flag in
>> its desired state after a migration?
> 
> The guest does not get any control of FSGSBASE.  Xen unilaterally has it
> enabled, and uses them on vcpu context switch so it is not safe to ever
> disable.

Again - is the comment the misleading? I took it to say that the
guest can control its _view_ of the flag without controlling the
hardware setting. I.e. when it wrote it as zero, it would read
back zero, despite the flag being set in hardware at all times.

Jan

Re: [PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Andrew Cooper]

On 26/06/2015 07:22, Jan Beulich wrote:
>>>> On 25.06.15 at 18:22, <andrew.cooper3@citrix.com> wrote:
>> On 25/06/15 15:41, Jan Beulich wrote:
>>>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>>>> The major change here is that v->arch.pv_vcpu.ctrlreg[4] now contains only CR4
>>>> bits which Xen wishes to shadow, rather than containing a mix of host and
>>>> guest bits.  This is likely to elicit a warning when migrating a PV guest 
>>>> from
>>>> an older version of Xen, although no function problems as a result.
>>> For one, I'm having a hard time guessing what the part of the sentence
>>> after the comma is meant to tell me.
>> Before this patch, v->arch.pv_vcpu.ctrlreg[4] contained an unqualified
>> combination of host and guest controlled bits.
>>
>> After this patch, it strictly contains the bits Xen chooses to shadow.
> With this you're re-stating what the first sentence said after its comma.
> The question was on the second one though.

Ah.  That was referring to the fact that even if the warning fires, Xen
ignores the results and chooses its own set of bits in CR4.

>
>>> And then, from a support
>>> perspective, such warnings aren't helpful, as they incur questions.
>> It was useful for debugging, but can see your point.  I could reword it
>> to "ignoring $FOO's attempt to update $BAR's CR4".  I would hesitate
>> from removing it completely however.
> Yes, the warning in general should stay of course (there is one
> after all right now). I realize I slightly mis-read the sentence
> though - looks like the implication is that the possible warning is
> from the mixture of bits previously in .ctrlreg[4]?

That was the purpose of the comment.

> Could we flag
> the write originating from a migration, issuing the warning only
> for guest initiated updates?

I can put a current check in (although it will have to be carefully to
avoid issues when creating the idle vcpus).

I wasn't sure how much to care about bad values appearing as a result of
toolstack actions, but I suppose this is usually only restore, so
probably not that important.

>
>>> Plus
>>> finally, storing neither the current guest view nor the current hypervisor
>>> view in that field seems prone to cause headaches in the future. Otoh
>>> this gets it in line with CR0 handling as it looks.
>> My logic was that the set of shadowed bits will never decrease on newer
>> versions of Xen, but Xen's choice of CR4 bits may change.  As a result,
>> neither "guests view" nor "hypervisors view" is appropriate in an
>> upgrade case.
> Okay, namely together with getting it in line with CR0 handling this
> makes sense.
>
>>>> A second change is that visible set of CR4 bits is different.  In 
>>>> particular,
>>>> VMXE and MCE are not leaked into a guest, while SMAP and SMEP are exposed.
>>> Not leaking VMXE into the guest is fine. Not exposing MCE even to
>>> Dom0 is questionable.
>> I would prefer not to special case the hardware domain if possible, and
>> I suppose MCE is not too much of an issue to leak.  We already are,
>> after all.
>>
>>> And exposing SMEP and SMAP without
>>> exposing their CPUID flags seems bogus.
>> I was hoping not to get bogged down in CPUID given my upcoming work to
>> fix feature levelling.
>>
>> After recent consideration, I am still not sure if we want to support
>> SMAP in 32bit PV guests or not.  The trapping of stac/clac would be a
>> high overhead, although the guest could modify EFLAGS.AC itself using popf.
> If it technically works (which it looks like it does) I see no reason
> leaving the decision to guests.

True.  For now, I will leave SMAP/SMEP unexposed and leave the CPUID
side of things to my levelling work.

>
>>>> In addition, add PGE to the set of bits we don't care about a guest 
>>>> attempting to modify.
>>> I agree from a functionality pov this is okay, but do we really want
>>> to allow the guest to turn this off from a performance pov?
>> This change doesn't shadow PGE.  It just prevents the warning from
>> firing if the guest attempts to clear PGE.
> I which case I think the wording you chose is slightly misleading.
>
>>>> @@ -699,6 +719,8 @@ static int __init init_pv_cr4_masks(void)
>>>>          common_mask &= ~X86_CR4_DE;
>>>>      if ( cpu_has_xsave )
>>>>          common_mask &= ~X86_CR4_OSXSAVE;
>>>> +    if ( cpu_has_pge )
>>>> +        common_mask &= ~X86_CR4_PGE;
>>> Following the earlier comment, shouldn't this bit then also be added
>>> to PV_CR4_SHADOW?
>> I don't think so.  A guest has to defer to Xen to perform TLB flushes. 
>> I just wished to avoid the warning.
> Maybe add a comment to that effect then, avoiding future readers
> to feel tempted to correct this apparent inconsistency, just like it
> happened to me?
>
>>> With FSGSBASE not in PV_CR4_SHADOW, how would (as the comment
>>> in init_pv_cr4_masks() says) the guest manage to observe the flag in
>>> its desired state after a migration?
>> The guest does not get any control of FSGSBASE.  Xen unilaterally has it
>> enabled, and uses them on vcpu context switch so it is not safe to ever
>> disable.
> Again - is the comment the misleading? I took it to say that the
> guest can control its _view_ of the flag without controlling the
> hardware setting. I.e. when it wrote it as zero, it would read
> back zero, despite the flag being set in hardware at all times.

I shall see about improving the comments to explain that the these masks
are unrelated to whether Xen chooses to shadow the individual bits or not.

~Andrew

Re: [PATCH 7/8] xen/x86: Rework CR4 handling for PV guests

[Jan Beulich]

>>> On 26.06.15 at 10:10, <andrew.cooper3@citrix.com> wrote:
> On 26/06/2015 07:22, Jan Beulich wrote:
>>>>> On 25.06.15 at 18:22, <andrew.cooper3@citrix.com> wrote:
>>> After recent consideration, I am still not sure if we want to support
>>> SMAP in 32bit PV guests or not.  The trapping of stac/clac would be a
>>> high overhead, although the guest could modify EFLAGS.AC itself using popf.
>> If it technically works (which it looks like it does) I see no reason
>> leaving the decision to guests.
> 
> True.  For now, I will leave SMAP/SMEP unexposed and leave the CPUID
> side of things to my levelling work.

In which case there's no point assuming an "aware" guest - with the
CPUID flag unexposed, writes to CR4.SMAP are simply invalid.

Jan

[PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[Andrew Cooper]

Experimentally, older Linux guests perform construction of `init` with user
pagetable mappings.  This is fine for native systems as such a guest would not
set CR4.SMAP itself.

However if Xen uses SMAP itself, 32bit PV guests (whose kernels run in ring1)
are also affected.  Older Linux guests end up spinning in a loop assuming that
the SMAP violation pagefaults are spurious, and make no further progress.

One option is to disable SMAP completely, but this is unreasonable.  A better
alternative is to disable SMAP only in the context of 32bit PV guests, but
reduces the effectiveness SMAP security.  A 3rd option is for Xen to fix up
behind a 32bit guest if it were SMAP-aware.  It is a heuristic, and does
result in a guest-visible state change, but allows Xen to keep CR4.SMAP
unconditionally enabled.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 docs/misc/xen-command-line.markdown |   25 ++++++++++++++++++--
 xen/arch/x86/domain.c               |   16 +++++++++++--
 xen/arch/x86/setup.c                |   31 +++++++++++++++++++++----
 xen/arch/x86/traps.c                |   43 ++++++++++++++++++++++++-----------
 xen/include/asm-x86/processor.h     |   10 ++++++++
 5 files changed, 103 insertions(+), 22 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index aa684c0..a63d2b2 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
 Flag to enable Supervisor Mode Execution Protection
 
 ### smap
-> `= <boolean>`
+> `= <boolean> | compat | fixup`
 
 > Default: `true`
 
-Flag to enable Supervisor Mode Access Prevention
+Handling of Supervisor Mode Access Prevention.
+
+32bit PV guest kernels qualify as supervisor code, as they execute in ring 1.
+If Xen uses SMAP protection itself, a PV guest which is not SMAP aware may
+suffer unexpected pagefaults which it cannot handle. (Experimentally, there
+are 32bit PV guests which fall foul of SMAP enforcement and spin in an
+infinite loop taking pagefaults early on boot.)
+
+Two further SMAP modes are introduced to work around buggy 32bit PV guests to
+prevent functional regressions of VMs on newer hardware.  At any point if the
+guest sets `CR4.SMAP` itself, it is deemed aware, and **compat/fixup** cease
+to apply.
+
+A SMAP mode of **compat** causes Xen to disable `CR4.SMAP` in the context of
+an unaware 32bit PV guest.  This prevents the guest from being subject to SMAP
+enforcement, but also prevents Xen from benefiting from the added security
+checks.
+
+A SMAP mode of **fixup** causes Xen to set `EFLAGS.AC` when discovering a SMAP
+pagefault in the context of an unaware 32bit PV guest.  This allows Xen to
+retain the added security from SMAP checks, but results in a guest-visible
+state change which it might object to.
 
 ### snb\_igd\_quirk
 > `= <boolean> | cap | <integer>`
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 18c3c62..045a2b9 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -687,9 +687,10 @@ void arch_domain_unpause(struct domain *d)
  *  - TSD for vtsc
  *  - DE for %%dr4/5 emulation
  *  - OSXSAVE for xsetbv emulation
+ *  - SMAP for smap_mode_{compat,fixup} handling
  */
 #define PV_CR4_SHADOW                           \
-    (X86_CR4_TSD | X86_CR4_DE | X86_CR4_OSXSAVE)
+    (X86_CR4_TSD | X86_CR4_DE | X86_CR4_OSXSAVE | X86_CR4_SMAP)
 
 /* CR4 bits a guest controls. */
 #define PV_CR4_GUEST (X86_CR4_TSD)
@@ -700,7 +701,7 @@ void arch_domain_unpause(struct domain *d)
  */
 #define PV_CR4_READ                                                     \
     (X86_CR4_PAE | X86_CR4_PGE |X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT |   \
-     X86_CR4_FSGSBASE | X86_CR4_SMEP | X86_CR4_SMAP)
+     X86_CR4_FSGSBASE | X86_CR4_SMEP)
 
 /*
  * These are the masks of CR4 bits (subject to hardware availability) which a
@@ -730,6 +731,12 @@ static int __init init_pv_cr4_masks(void)
     if ( cpu_has_fsgsbase )
         pv_cr4_mask &= ~X86_CR4_FSGSBASE;
 
+    /*
+     * 32bit PV guests may attempt to modify SMAP.
+     */
+    if ( cpu_has_smap )
+        compat_pv_cr4_mask &= ~X86_CR4_SMAP;
+
     BUILD_BUG_ON(PV_CR4_SHADOW & PV_CR4_READ);
 
     return 0;
@@ -784,6 +791,11 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v)
     if ( v->domain->arch.vtsc )
         cr4 |= X86_CR4_TSD;
 
+    /* Disable SMAP behind unaware 32bit PV guests. */
+    if ( (smap_mode == smap_mode_compat) && is_pv_32bit_vcpu(v) &&
+         ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) )
+        cr4 &= ~X86_CR4_SMAP;
+
     return cr4;
 }
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ff34670..36fce42 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -63,10 +63,6 @@
 static bool_t __initdata disable_smep;
 invbool_param("smep", disable_smep);
 
-/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata disable_smap;
-invbool_param("smap", disable_smap);
-
 /* Boot dom0 in pvh mode */
 static bool_t __initdata opt_dom0pvh;
 boolean_param("dom0pvh", opt_dom0pvh);
@@ -138,6 +134,29 @@ static void __init parse_acpi_param(char *s)
     }
 }
 
+enum xen_smap_mode smap_mode __read_mostly = smap_mode_enable;
+static void __init parse_smap(char *s)
+{
+    switch ( parse_bool(s) )
+    {
+    case 1:
+        smap_mode = smap_mode_enable;
+        break;
+
+    case 0:
+        smap_mode = smap_mode_disable;
+        break;
+
+    default:
+        if ( !strcmp(s, "compat") )
+            smap_mode = smap_mode_compat;
+        else if ( !strcmp(s, "fixup") )
+            smap_mode = smap_mode_fixup;
+        break;
+    }
+}
+custom_param("smap", parse_smap);
+
 static const module_t *__initdata initial_images;
 static unsigned int __initdata nr_initial_images;
 
@@ -1296,10 +1315,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( cpu_has_smep )
         set_in_cr4(X86_CR4_SMEP);
 
-    if ( disable_smap )
+    if ( smap_mode == smap_mode_disable )
         setup_clear_cpu_cap(X86_FEATURE_SMAP);
     if ( cpu_has_smap )
         set_in_cr4(X86_CR4_SMAP);
+    else
+        smap_mode = smap_mode_disable;
 
     if ( cpu_has_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1ab870d..149a255 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1437,21 +1437,38 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
         return 0;
     }
 
-    if ( guest_kernel_mode(v, regs) &&
-         !(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
-         (regs->error_code & PFEC_write_access) )
-    {
-        if ( VM_ASSIST(d, writable_pagetables) &&
-             /* Do not check if access-protection fault since the page may
-                legitimately be not present in shadow page tables */
-             (paging_mode_enabled(d) ||
-              (regs->error_code & PFEC_page_present)) &&
-             ptwr_do_page_fault(v, addr, regs) )
-            return EXCRET_fault_fixed;
+    if ( guest_kernel_mode(v, regs) )
+    {
+        if (!(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
+            (regs->error_code & PFEC_write_access) )
+        {
+            if ( VM_ASSIST(d, writable_pagetables) &&
+                 /* Do not check if access-protection fault since the page may
+                    legitimately be not present in shadow page tables */
+                 (paging_mode_enabled(d) ||
+                  (regs->error_code & PFEC_page_present)) &&
+                 ptwr_do_page_fault(v, addr, regs) )
+                return EXCRET_fault_fixed;
 
-        if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
-             mmio_ro_do_page_fault(v, addr, regs) )
+            if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
+                 mmio_ro_do_page_fault(v, addr, regs) )
+                return EXCRET_fault_fixed;
+        }
+
+        /*
+         * SMAP violation behind an unaware 32bit PV guest kernel? Set
+         * EFLAGS.AC behind its back and try again.
+         */
+        if ( (smap_mode == smap_mode_fixup) && is_pv_32bit_domain(d) &&
+             ((regs->error_code &
+               (PFEC_insn_fetch | PFEC_reserved_bit |
+                PFEC_user_mode | PFEC_page_present)) == PFEC_page_present) &&
+             ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) &&
+             ((regs->eflags & X86_EFLAGS_AC) == 0) )
+        {
+            regs->eflags |= X86_EFLAGS_AC;
             return EXCRET_fault_fixed;
+        }
     }
 
     /* For non-external shadowed guests, we fix up both their own 
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 5fb340f..22d5cfa 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -581,6 +581,16 @@ enum get_cpu_vendor {
 int get_cpu_vendor(const char vendor_id[], enum get_cpu_vendor);
 void pv_cpuid(struct cpu_user_regs *regs);
 
+enum xen_smap_mode {
+    smap_mode_enable,  /* Use SMAP.                                       */
+    smap_mode_disable, /* Don't use SMAP.                                 */
+    smap_mode_compat,  /* Context switch CR4.SMAP behind an unaware 32bit */
+                       /* PV guest.                                       */
+    smap_mode_fixup,   /* Set EFLAGAS.AC on a SMAP fault behind an        */
+                       /* unaware 32bit PV guest.                         */
+};
+extern enum xen_smap_mode smap_mode;
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_X86_PROCESSOR_H */
-- 
1.7.10.4

Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[David Vrabel]

On 24/06/15 17:31, Andrew Cooper wrote:
> Experimentally, older Linux guests perform construction of `init` with user
> pagetable mappings.  This is fine for native systems as such a guest would not
> set CR4.SMAP itself.
> 
> However if Xen uses SMAP itself, 32bit PV guests (whose kernels run in ring1)
> are also affected.  Older Linux guests end up spinning in a loop assuming that
> the SMAP violation pagefaults are spurious, and make no further progress.
> 
> One option is to disable SMAP completely, but this is unreasonable.  A better
> alternative is to disable SMAP only in the context of 32bit PV guests, but
> reduces the effectiveness SMAP security.  A 3rd option is for Xen to fix up
> behind a 32bit guest if it were SMAP-aware.  It is a heuristic, and does
> result in a guest-visible state change, but allows Xen to keep CR4.SMAP
> unconditionally enabled.
[...]
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
>  Flag to enable Supervisor Mode Execution Protection
>  
>  ### smap
> -> `= <boolean>`
> +> `= <boolean> | compat | fixup`
>  
>  > Default: `true`
>  
> -Flag to enable Supervisor Mode Access Prevention
> +Handling of Supervisor Mode Access Prevention.
> +
> +32bit PV guest kernels qualify as supervisor code, as they execute in ring 1.
> +If Xen uses SMAP protection itself, a PV guest which is not SMAP aware may
> +suffer unexpected pagefaults which it cannot handle. (Experimentally, there
> +are 32bit PV guests which fall foul of SMAP enforcement and spin in an
> +infinite loop taking pagefaults early on boot.)
> +
> +Two further SMAP modes are introduced to work around buggy 32bit PV guests to
> +prevent functional regressions of VMs on newer hardware.  At any point if the
> +guest sets `CR4.SMAP` itself, it is deemed aware, and **compat/fixup** cease
> +to apply.

Guests that is not aware of SMAP or do not support it are not "buggy".

> +
> +A SMAP mode of **compat** causes Xen to disable `CR4.SMAP` in the context of
> +an unaware 32bit PV guest.  This prevents the guest from being subject to SMAP
> +enforcement, but also prevents Xen from benefiting from the added security
> +checks.
> +
> +A SMAP mode of **fixup** causes Xen to set `EFLAGS.AC` when discovering a SMAP
> +pagefault in the context of an unaware 32bit PV guest.  This allows Xen to
> +retain the added security from SMAP checks, but results in a guest-visible
> +state change which it might object to.

What does the PV ABI say about the use of EFLAGS.AC?  Have guests
historically been allowed to use this bit?  If so, does Xen fiddling
with it potentially break some guests?

David

Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[Andrew Cooper]

On 25/06/15 12:18, David Vrabel wrote:
> On 24/06/15 17:31, Andrew Cooper wrote:
>> Experimentally, older Linux guests perform construction of `init` with user
>> pagetable mappings.  This is fine for native systems as such a guest would not
>> set CR4.SMAP itself.
>>
>> However if Xen uses SMAP itself, 32bit PV guests (whose kernels run in ring1)
>> are also affected.  Older Linux guests end up spinning in a loop assuming that
>> the SMAP violation pagefaults are spurious, and make no further progress.
>>
>> One option is to disable SMAP completely, but this is unreasonable.  A better
>> alternative is to disable SMAP only in the context of 32bit PV guests, but
>> reduces the effectiveness SMAP security.  A 3rd option is for Xen to fix up
>> behind a 32bit guest if it were SMAP-aware.  It is a heuristic, and does
>> result in a guest-visible state change, but allows Xen to keep CR4.SMAP
>> unconditionally enabled.
> [...]
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
>>  Flag to enable Supervisor Mode Execution Protection
>>  
>>  ### smap
>> -> `= <boolean>`
>> +> `= <boolean> | compat | fixup`
>>  
>>  > Default: `true`
>>  
>> -Flag to enable Supervisor Mode Access Prevention
>> +Handling of Supervisor Mode Access Prevention.
>> +
>> +32bit PV guest kernels qualify as supervisor code, as they execute in ring 1.
>> +If Xen uses SMAP protection itself, a PV guest which is not SMAP aware may
>> +suffer unexpected pagefaults which it cannot handle. (Experimentally, there
>> +are 32bit PV guests which fall foul of SMAP enforcement and spin in an
>> +infinite loop taking pagefaults early on boot.)
>> +
>> +Two further SMAP modes are introduced to work around buggy 32bit PV guests to
>> +prevent functional regressions of VMs on newer hardware.  At any point if the
>> +guest sets `CR4.SMAP` itself, it is deemed aware, and **compat/fixup** cease
>> +to apply.
> Guests that is not aware of SMAP or do not support it are not "buggy".

Taking and not understanding a SMAP #PF is understandable.  The way it
spins in an infinite loop is unquestionably buggy.

>
>> +
>> +A SMAP mode of **compat** causes Xen to disable `CR4.SMAP` in the context of
>> +an unaware 32bit PV guest.  This prevents the guest from being subject to SMAP
>> +enforcement, but also prevents Xen from benefiting from the added security
>> +checks.
>> +
>> +A SMAP mode of **fixup** causes Xen to set `EFLAGS.AC` when discovering a SMAP
>> +pagefault in the context of an unaware 32bit PV guest.  This allows Xen to
>> +retain the added security from SMAP checks, but results in a guest-visible
>> +state change which it might object to.
> What does the PV ABI say about the use of EFLAGS.AC?  Have guests
> historically been allowed to use this bit?  If so, does Xen fiddling
> with it potentially break some guests?

If there were an ABI written down anywhere, I might be able to answer
that question.

32bit PV guest kernels cannot make use of AC themselves; alignment
checking is only available in cpl3.  AC is however able to be changed by
a popf instruction even in cpl3 (which make it very curious as to why
stac/clac are strictly cpl0 instructions).

Fundamentally, smap=fixup might indeed break a PV guest, but testing
shows that RHEL/CentOS 5/6, SLES 11/12 and Debian 6/7 PV guests are all
fine with it.

~Andrew

Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[Jan Beulich]

>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
>  Flag to enable Supervisor Mode Execution Protection
>  
>  ### smap
> -> `= <boolean>`
> +> `= <boolean> | compat | fixup`
>  
>  > Default: `true`

Knowing that it breaks certain guests, I think the default can't be
true, but instead needs to become compat. People wanting more
security at the expense of breaking guests can then pick either
of true or fixup.

> @@ -730,6 +731,12 @@ static int __init init_pv_cr4_masks(void)
>      if ( cpu_has_fsgsbase )
>          pv_cr4_mask &= ~X86_CR4_FSGSBASE;
>  
> +    /*
> +     * 32bit PV guests may attempt to modify SMAP.
> +     */
> +    if ( cpu_has_smap )
> +        compat_pv_cr4_mask &= ~X86_CR4_SMAP;

Shouldn't this then be accompanied by exposing the CPUID flag to
32-bit guests?

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1437,21 +1437,38 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
>          return 0;
>      }
>  
> -    if ( guest_kernel_mode(v, regs) &&
> -         !(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
> -         (regs->error_code & PFEC_write_access) )
> -    {
> -        if ( VM_ASSIST(d, writable_pagetables) &&
> -             /* Do not check if access-protection fault since the page may
> -                legitimately be not present in shadow page tables */
> -             (paging_mode_enabled(d) ||
> -              (regs->error_code & PFEC_page_present)) &&
> -             ptwr_do_page_fault(v, addr, regs) )
> -            return EXCRET_fault_fixed;
> +    if ( guest_kernel_mode(v, regs) )
> +    {
> +        if (!(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
> +            (regs->error_code & PFEC_write_access) )
> +        {
> +            if ( VM_ASSIST(d, writable_pagetables) &&
> +                 /* Do not check if access-protection fault since the page may
> +                    legitimately be not present in shadow page tables */
> +                 (paging_mode_enabled(d) ||
> +                  (regs->error_code & PFEC_page_present)) &&
> +                 ptwr_do_page_fault(v, addr, regs) )
> +                return EXCRET_fault_fixed;
>  
> -        if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
> -             mmio_ro_do_page_fault(v, addr, regs) )
> +            if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
> +                 mmio_ro_do_page_fault(v, addr, regs) )
> +                return EXCRET_fault_fixed;
> +        }
> +
> +        /*
> +         * SMAP violation behind an unaware 32bit PV guest kernel? Set
> +         * EFLAGS.AC behind its back and try again.
> +         */
> +        if ( (smap_mode == smap_mode_fixup) && is_pv_32bit_domain(d) &&
> +             ((regs->error_code &
> +               (PFEC_insn_fetch | PFEC_reserved_bit |
> +                PFEC_user_mode | PFEC_page_present)) == PFEC_page_present) &&
> +             ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) &&
> +             ((regs->eflags & X86_EFLAGS_AC) == 0) )

At least for single bit checks like these, could I talk you into using
!(x & mask), as is being done elsewhere in the code you modify
above?

Also here as well as in the code enforcing compat mode, I'm not
sure it's correct to tie this to the current guest setting of CR4.SMAP.
Instead I think you should latch any attempt by the guest to set
the bit into a per-domain flag. After all it may itself have reasons to
play strange things with the bit.

Or alternatively the terminology in the comment and command line
option documentation should be changed from "unaware" to "not
currently having enabled".

Jan

Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[Andrew Cooper]

On 25/06/15 16:12, Jan Beulich wrote:
>>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1261,11 +1261,32 @@ Set the serial transmit buffer size.
>>  Flag to enable Supervisor Mode Execution Protection
>>  
>>  ### smap
>> -> `= <boolean>`
>> +> `= <boolean> | compat | fixup`
>>  
>>  > Default: `true`
> Knowing that it breaks certain guests, I think the default can't be
> true, but instead needs to become compat. People wanting more
> security at the expense of breaking guests can then pick either
> of true or fixup.

Ok.

> Jan
>


>
>> @@ -730,6 +731,12 @@ static int __init init_pv_cr4_masks(void)
>>      if ( cpu_has_fsgsbase )
>>          pv_cr4_mask &= ~X86_CR4_FSGSBASE;
>>  
>> +    /*
>> +     * 32bit PV guests may attempt to modify SMAP.
>> +     */
>> +    if ( cpu_has_smap )
>> +        compat_pv_cr4_mask &= ~X86_CR4_SMAP;
> Shouldn't this then be accompanied by exposing the CPUID flag to
> 32-bit guests?

I have become undecided on whether actually exposing SMAP is sensible or
not.  Not exposing it does make these fixes more simple.

>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1437,21 +1437,38 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
>>          return 0;
>>      }
>>  
>> -    if ( guest_kernel_mode(v, regs) &&
>> -         !(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
>> -         (regs->error_code & PFEC_write_access) )
>> -    {
>> -        if ( VM_ASSIST(d, writable_pagetables) &&
>> -             /* Do not check if access-protection fault since the page may
>> -                legitimately be not present in shadow page tables */
>> -             (paging_mode_enabled(d) ||
>> -              (regs->error_code & PFEC_page_present)) &&
>> -             ptwr_do_page_fault(v, addr, regs) )
>> -            return EXCRET_fault_fixed;
>> +    if ( guest_kernel_mode(v, regs) )
>> +    {
>> +        if (!(regs->error_code & (PFEC_reserved_bit | PFEC_insn_fetch)) &&
>> +            (regs->error_code & PFEC_write_access) )
>> +        {
>> +            if ( VM_ASSIST(d, writable_pagetables) &&
>> +                 /* Do not check if access-protection fault since the page may
>> +                    legitimately be not present in shadow page tables */
>> +                 (paging_mode_enabled(d) ||
>> +                  (regs->error_code & PFEC_page_present)) &&
>> +                 ptwr_do_page_fault(v, addr, regs) )
>> +                return EXCRET_fault_fixed;
>>  
>> -        if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
>> -             mmio_ro_do_page_fault(v, addr, regs) )
>> +            if ( is_hardware_domain(d) && (regs->error_code & PFEC_page_present) &&
>> +                 mmio_ro_do_page_fault(v, addr, regs) )
>> +                return EXCRET_fault_fixed;
>> +        }
>> +
>> +        /*
>> +         * SMAP violation behind an unaware 32bit PV guest kernel? Set
>> +         * EFLAGS.AC behind its back and try again.
>> +         */
>> +        if ( (smap_mode == smap_mode_fixup) && is_pv_32bit_domain(d) &&
>> +             ((regs->error_code &
>> +               (PFEC_insn_fetch | PFEC_reserved_bit |
>> +                PFEC_user_mode | PFEC_page_present)) == PFEC_page_present) &&
>> +             ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) &&
>> +             ((regs->eflags & X86_EFLAGS_AC) == 0) )
> At least for single bit checks like these, could I talk you into using
> !(x & mask), as is being done elsewhere in the code you modify
> above?

Ok.

>
> Also here as well as in the code enforcing compat mode, I'm not
> sure it's correct to tie this to the current guest setting of CR4.SMAP.
> Instead I think you should latch any attempt by the guest to set
> the bit into a per-domain flag. After all it may itself have reasons to
> play strange things with the bit.

In both cases, if the guest sets CR4.SMAP itself, Xen needs to stop any
fixup behind the guests back.  If the guest subsequently clears SMAP for
a bit, it will expect to stop getting violations, but Xen still wants to
fix things up properly.

>
> Or alternatively the terminology in the comment and command line
> option documentation should be changed from "unaware" to "not
> currently having enabled".

This is better.  I will update the description.

~Andrew

Re: [PATCH 8/8] xen/x86: Additional SMAP modes to work around buggy 32bit PV guests

[Jan Beulich]

>>> On 24.06.15 at 18:31, <andrew.cooper3@citrix.com> wrote:
> @@ -784,6 +791,11 @@ unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v)
>      if ( v->domain->arch.vtsc )
>          cr4 |= X86_CR4_TSD;
>  
> +    /* Disable SMAP behind unaware 32bit PV guests. */
> +    if ( (smap_mode == smap_mode_compat) && is_pv_32bit_vcpu(v) &&
> +         ((v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_SMAP) == 0) )
> +        cr4 &= ~X86_CR4_SMAP;

There's actually another problem here: The function you modify
is used by paravirt_ctxt_switch_to(). Obviously you will want to
make sure to re-enable SMAP when switching away from the
guest, and not only at the point you finally switch to a suitable
other non-idle vCPU.

And then there's the question whether the above fixup wouldn't
better be deferred until compat_restore_all_guest, minimizing
the "damage" to Xen?

Jan