Patchwork x86: introduce and use setup_force_cpu_cap()

login
register
mail settings
Submitter Jan Beulich
Date Sept. 5, 2017, 1:22 p.m.
Message ID <59AEC14A0200007800177861@prv-mh.provo.novell.com>
Download mbox | patch
Permalink /patch/332265/
State New
Headers show

Comments

Jan Beulich - Sept. 5, 2017, 1:22 p.m.
For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd
need to clone the respective hack used for CPUID_FAULTING. Introduce an
inverse of setup_clear_cpu_cap() instead, but let clearing of features
overrule forced setting of them.

XEN_SMAP being wrong post-boot is a problem specifically for live
patching, as a live patch may need alternative instruction patching
keyed off of that feature flag.

Reported-by: Sarah Newman <security@prgmr.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Andrew Cooper - Sept. 5, 2017, 3:58 p.m.
On 05/09/17 14:22, Jan Beulich wrote:
> For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd
> need to clone the respective hack used for CPUID_FAULTING. Introduce an
> inverse of setup_clear_cpu_cap() instead, but let clearing of features
> overrule forced setting of them.
>
> XEN_SMAP being wrong post-boot is a problem specifically for live
> patching, as a live patch may need alternative instruction patching
> keyed off of that feature flag.
>
> Reported-by: Sarah Newman <security@prgmr.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Sarah Newman - Sept. 7, 2017, 1:55 a.m.
On 09/05/2017 06:22 AM, Jan Beulich wrote:
> For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd
> need to clone the respective hack used for CPUID_FAULTING. Introduce an
> inverse of setup_clear_cpu_cap() instead, but let clearing of features
> overrule forced setting of them.
> 
> XEN_SMAP being wrong post-boot is a problem specifically for live
> patching, as a live patch may need alternative instruction patching
> keyed off of that feature flag.
> 
> Reported-by: Sarah Newman <security@prgmr.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reported-by/Tested-by: Sarah Newman <srn@prgmr.com>

Some questions:

It looks like setup_clear_cpu_cap has a search for dependent capabilities that also must be cleared. Does the same need to happen for
setup_force_cpu_cap even if it doesn't matter for the current cpu features?

Does it make sense to add a comment where forced_caps is declared that cleared_caps overrides forced_caps instead of just in the commit message?
Jan Beulich - Sept. 7, 2017, 6 a.m.
>>> Sarah Newman <srn@prgmr.com> 09/07/17 3:55 AM >>>
>On 09/05/2017 06:22 AM, Jan Beulich wrote:
>> For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd
>> need to clone the respective hack used for CPUID_FAULTING. Introduce an
>> inverse of setup_clear_cpu_cap() instead, but let clearing of features
>> overrule forced setting of them.
>> 
>> XEN_SMAP being wrong post-boot is a problem specifically for live
>> patching, as a live patch may need alternative instruction patching
>> keyed off of that feature flag.
>> 
>> Reported-by: Sarah Newman <security@prgmr.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>Reported-by/Tested-by: Sarah Newman <srn@prgmr.com>

Thanks.

>Some questions:
>
>It looks like setup_clear_cpu_cap has a search for dependent capabilities
>that also must be cleared. Does the same need to happen for
>setup_force_cpu_cap even if it doesn't matter for the current cpu features?

We obviously can't force-set dependent capabilities, and forcing a feature
on won't result in the need to clear any others (unless of course it was a
strange inverse "feature", but for those it would probably be better to not
force them on in the first place).

>Does it make sense to add a comment where forced_caps is declared
>that cleared_caps overrides forced_caps instead of just in the commit
>message?

From the code it should be pretty obvious, I would think. But of course
you're free to submit a patch to add comments if you feel strongly about
it.

Jan

Patch

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -853,7 +853,7 @@  static int __init detect_init_APIC (void
         return -1;
     }
 
-    __set_bit(X86_FEATURE_APIC, boot_cpu_data.x86_capability);
+    setup_force_cpu_cap(X86_FEATURE_APIC);
     mp_lapic_addr = APIC_DEFAULT_PHYS_BASE;
 
     /* The BIOS may have set up the APIC at some other address */
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -54,6 +54,7 @@  unsigned int vaddr_bits __read_mostly =
 u64 host_pat = 0x050100070406;
 
 static unsigned int cleared_caps[NCAPINTS];
+static unsigned int forced_caps[NCAPINTS];
 
 void __init setup_clear_cpu_cap(unsigned int cap)
 {
@@ -63,6 +64,10 @@  void __init setup_clear_cpu_cap(unsigned
 	if (__test_and_set_bit(cap, cleared_caps))
 		return;
 
+	if (test_bit(cap, forced_caps))
+		printk("%pS clearing previously forced feature %#x\n",
+		       __builtin_return_address(0), cap);
+
 	__clear_bit(cap, boot_cpu_data.x86_capability);
 	dfs = lookup_deep_deps(cap);
 
@@ -72,7 +77,26 @@  void __init setup_clear_cpu_cap(unsigned
 	for (i = 0; i < FSCAPINTS; ++i) {
 		cleared_caps[i] |= dfs[i];
 		boot_cpu_data.x86_capability[i] &= ~dfs[i];
+		if (!(forced_caps[i] & dfs[i]))
+			continue;
+		printk("%pS implicitly clearing previously forced feature(s) %u:%#x\n",
+		       __builtin_return_address(0),
+		       i, forced_caps[i] & dfs[i]);
+	}
+}
+
+void __init setup_force_cpu_cap(unsigned int cap)
+{
+	if (__test_and_set_bit(cap, forced_caps))
+		return;
+
+	if (test_bit(cap, cleared_caps)) {
+		printk("%pS tries to force previously cleared feature %#x\n",
+		       __builtin_return_address(0), cap);
+		return;
 	}
+
+	__set_bit(cap, boot_cpu_data.x86_capability);
 }
 
 static void default_init(struct cpuinfo_x86 * c)
@@ -375,8 +399,10 @@  void identify_cpu(struct cpuinfo_x86 *c)
 	for (i = 0; i < FSCAPINTS; ++i)
 		c->x86_capability[i] &= known_features[i];
 
-	for (i = 0 ; i < NCAPINTS ; ++i)
+	for (i = 0 ; i < NCAPINTS ; ++i) {
+		c->x86_capability[i] |= forced_caps[i];
 		c->x86_capability[i] &= ~cleared_caps[i];
+	}
 
 	/* If the model name is still unset, do table lookup. */
 	if ( !c->x86_model_id[0] ) {
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -27,7 +27,7 @@  static bool __init probe_intel_cpuid_fau
 
 	expected_levelling_cap |= LCAP_faulting;
 	levelling_caps |=  LCAP_faulting;
-	__set_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability);
+	setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING);
 	return 1;
 }
 
@@ -320,9 +320,6 @@  static void early_init_intel(struct cpui
 	if (c == &boot_cpu_data)
 		intel_init_levelling();
 
-	if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
-		__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
-
 	intel_ctxt_switch_levelling(NULL);
 }
 
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1485,14 +1485,14 @@  void __init noreturn __start_xen(unsigne
     if ( !opt_smep )
         setup_clear_cpu_cap(X86_FEATURE_SMEP);
     if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY )
-        __set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability);
+        setup_force_cpu_cap(X86_FEATURE_XEN_SMEP);
     if ( boot_cpu_has(X86_FEATURE_XEN_SMEP) )
         set_in_cr4(X86_CR4_SMEP);
 
     if ( !opt_smap )
         setup_clear_cpu_cap(X86_FEATURE_SMAP);
     if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY )
-        __set_bit(X86_FEATURE_XEN_SMAP, boot_cpu_data.x86_capability);
+        setup_force_cpu_cap(X86_FEATURE_XEN_SMAP);
     if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
         set_in_cr4(X86_CR4_SMAP);
 
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -169,6 +169,7 @@  extern const struct x86_cpu_id *x86_matc
 
 extern void identify_cpu(struct cpuinfo_x86 *);
 extern void setup_clear_cpu_cap(unsigned int);
+extern void setup_force_cpu_cap(unsigned int);
 extern void print_cpu_info(unsigned int cpu);
 extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);