Re: [Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

2016-08-12 Thread Jan Beulich 
>>> On 12.08.16 at 12:03,  wrote:
> On Thu, Aug 11, 2016 at 07:14:06AM -0600, Jan Beulich wrote:
>> >>> On 11.08.16 at 11:17,  wrote:
>> > @@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long 
> mbi_p)
>> >  if ( !opt_smep )
>> >  setup_clear_cpu_cap(X86_FEATURE_SMEP);
>> >  if ( cpu_has_smep )
>> > +{
>> >  set_in_cr4(X86_CR4_SMEP);
>> > +if ( smep_hvm_only )
>> > +write_cr4(read_cr4() & ~X86_CR4_SMEP);
>> > +}
>> 
>> So that'll clear CR4.SMEP right here, but won't help with CR4 loads
>> from mmu_cr4_features (as e.g. happens indirectly during VM exits,
>> since the HOST_CR4 field gets set from this variable).
>> 
>> Did you in fact test your change, including validation of the features
>> correctly remaining off over the lifetime of the system?
>> 
>> Further, considering that you don't clear the two flags from Xen's
>> internal feature bitmap, and taken together with the internal feature
>> bitmap driving alternative instruction patching, I'd assume pointless
>> (and performance wise perhaps harmful) patching to now take place.
>> 
> Let me see whether I understand this correctly...
> 
> Regarding alternative instruction patching, if enabling SMAP for HVM but
> disabling it for Xen, SMAP bit must be set in x86_capability feature
> bitmap and cleared in mmu_cr4_features, which means instruction patching
> would take place and a #UD may occur (since SMAP is disable in Xen, but
> STAC/CLAC are patched and called).

No #UD would occur (hence I only said "performance wise perhaps
harmful"), as per the SDM.

> A little dirty solution I can think of now is to temperarily clear SMAP
> bit in x86_capability before patching instruction and then set it back
> when instruction patching finish, like:
> 
> ```
> if ( opt_smap == SMAP_HVM_ONLY )
> setup_clear_cpu_cap(X86_FEATURE_SMAP);
> 
> alternative_instructions();
> 
> if ( opt_smap == SMAP_HVM_ONLY )
> __set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
> ```

Except that this would need further adjustment (e.g. using just
__clear_bit() instead of setup_clear_cpu_cap(), or else you'd
break CPU hotplug).

I'm not against "a little dirty" solutions, as long as it doesn't end
in plain hackery, and as long as the end result indeed meets all
requirements.

> Appreciate it if you have a better solution.

Well, if I had a reasonably good solution, I could have gone and
simply implemented it. It is the need to consider all resulting cases
properly which makes this involved, and hence we've turned to
you (Intel) for assistance.

Jan


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

Re: [Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

2016-08-12 Thread He Chen 
On Thu, Aug 11, 2016 at 07:14:06AM -0600, Jan Beulich wrote:
> >>> On 11.08.16 at 11:17,  wrote:
> > @@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long 
> > mbi_p)
> >  if ( !opt_smep )
> >  setup_clear_cpu_cap(X86_FEATURE_SMEP);
> >  if ( cpu_has_smep )
> > +{
> >  set_in_cr4(X86_CR4_SMEP);
> > +if ( smep_hvm_only )
> > +write_cr4(read_cr4() & ~X86_CR4_SMEP);
> > +}
> 
> So that'll clear CR4.SMEP right here, but won't help with CR4 loads
> from mmu_cr4_features (as e.g. happens indirectly during VM exits,
> since the HOST_CR4 field gets set from this variable).
> 
> Did you in fact test your change, including validation of the features
> correctly remaining off over the lifetime of the system?
> 
> Further, considering that you don't clear the two flags from Xen's
> internal feature bitmap, and taken together with the internal feature
> bitmap driving alternative instruction patching, I'd assume pointless
> (and performance wise perhaps harmful) patching to now take place.
> 
Let me see whether I understand this correctly...

Regarding alternative instruction patching, if enabling SMAP for HVM but
disabling it for Xen, SMAP bit must be set in x86_capability feature
bitmap and cleared in mmu_cr4_features, which means instruction patching
would take place and a #UD may occur (since SMAP is disable in Xen, but
STAC/CLAC are patched and called).

A little dirty solution I can think of now is to temperarily clear SMAP
bit in x86_capability before patching instruction and then set it back
when instruction patching finish, like:

```
if ( opt_smap == SMAP_HVM_ONLY )
setup_clear_cpu_cap(X86_FEATURE_SMAP);

alternative_instructions();

if ( opt_smap == SMAP_HVM_ONLY )
__set_bit(X86_FEATURE_SMAP, boot_cpu_data.x86_capability);
```

Appreciate it if you have a better solution.

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

Re: [Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

2016-08-11 Thread Jan Beulich 
>>> On 11.08.16 at 11:17,  wrote:
> Enhance "skaj...@intel.com>mep" and "smap" command line options to support 

I guess that was meant to be "smep".

> enabling SMEP
> or SMAP for HVM only with allowing "hvm" as a value.

A primary complaint of mine on v1 was not addressed: The description
continues to not explain why what is being done is sufficient.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1427,19 +1427,21 @@ enabling more sockets and cores to go into deeper 
> sleep states.
>  
>  Set the serial transmit buffer size.
>  
> -### smep
> -> `= `
> +### smap
> +> `=  | hvm`
>  
>  > Default: `true`
>  
> -Flag to enable Supervisor Mode Execution Protection
> +Flag to enable Supervisor Mode Access Prevention
> +Using `smap=hvm` to enable SMAP for HVM guests only.

Use ...

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -4,6 +4,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>  const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
> @@ -118,6 +119,10 @@ static void __init calculate_pv_featureset(void)
>  __set_bit(X86_FEATURE_HTT, pv_featureset);
>  __set_bit(X86_FEATURE_X2APIC, pv_featureset);
>  __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
> +if ( smep_hvm_only )
> +__clear_bit(X86_FEATURE_SMEP, pv_featureset);
> +if ( smap_hvm_only )
> +__clear_bit(X86_FEATURE_SMAP, pv_featureset);

These flags already can't be set in pv_featureset, so the change
is pointless.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -61,13 +61,19 @@ boolean_param("nosmp", opt_nosmp);
>  static unsigned int __initdata max_cpus;
>  integer_param("maxcpus", max_cpus);
>  
> -/* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
> -static bool_t __initdata opt_smep = 1;
> -boolean_param("smep", opt_smep);
> -
> -/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
> -static bool_t __initdata opt_smap = 1;
> -boolean_param("smap", opt_smap);
> +/* Supervisor Mode Execution Protection (default on). */
> +/* "smep=on": Enable SMEP for Xen and guests. */
> +/* "smep=hvm": Enable SMEP for HVM only.  */
> +/* "smep=off": Disable SMEP for Xen and guests.   */
> +static void parse_smep_param(char *s);
> +custom_param("smep", parse_smep_param);
> +
> +/* Supervisor Mode Access Prevention (default on). */
> +/* "smep=on": Enable SMAP for Xen and guests.  */
> +/* "smep=hvm": Enable SMAP for HVM only.   */
> +/* "smep=off": Disable SMAP for Xen and guests.*/
> +static void parse_smap_param(char *s);
> +custom_param("smap", parse_smap_param);

The commentary goes too far imo; what's wrong with retaining the
earlier comments? Also please avoid forward declarations of static
functions when you easily can.

> @@ -111,6 +117,34 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 
> 0, 0, -1 };
>  
>  unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
>  
> +static bool_t __initdata opt_smep = 1;
> +bool_t __initdata smep_hvm_only = 0;

Why two variables when one (of non-boolean type) will do?

> +static void __init parse_smep_param(char *s)
> +{
> +if ( !parse_bool(s) )
> +{
> +opt_smep = 0;
> +}
> +else if ( !strcmp(s, "hvm") )
> +{
> +smep_hvm_only = 1;
> +}
> +}

A command line like "smep=hvm smep=on" would no longer work afaict.

Also - stray braces.

> @@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  if ( !opt_smep )
>  setup_clear_cpu_cap(X86_FEATURE_SMEP);
>  if ( cpu_has_smep )
> +{
>  set_in_cr4(X86_CR4_SMEP);
> +if ( smep_hvm_only )
> +write_cr4(read_cr4() & ~X86_CR4_SMEP);
> +}

So that'll clear CR4.SMEP right here, but won't help with CR4 loads
from mmu_cr4_features (as e.g. happens indirectly during VM exits,
since the HOST_CR4 field gets set from this variable).

Did you in fact test your change, including validation of the features
correctly remaining off over the lifetime of the system?

Further, considering that you don't clear the two flags from Xen's
internal feature bitmap, and taken together with the internal feature
bitmap driving alternative instruction patching, I'd assume pointless
(and performance wise perhaps harmful) patching to now take place.

Speaking of performance: The extremely bad performance with the
32-bit PV fix patches in was pretty unexpected. I'd now like to be
double certain that nothing similarly unexpected happens due to the
back on forth between SMEP/SMAP enabled and disabled during VM
entry and exit. One would think there should be no such effect, but
I'd prefer this to be confirmed.

Jan

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

[Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

2016-08-11 Thread He Chen 
Enhance "skaj...@intel.com>mep" and "smap" command line options to support 
enabling SMEP
or SMAP for HVM only with allowing "hvm" as a value.

Signed-off-by: He Chen 

---
Changes in V2:
* Allow "hvm" as a value to "smep" and "smap" command line options.
* Clear SMEP/SMAP CPUID bits for pv guests if they are set to hvm only.
* Refine docs.
* Rewrite commit message.
---
 docs/misc/xen-command-line.markdown | 14 +
 xen/arch/x86/cpuid.c|  5 
 xen/arch/x86/setup.c| 58 -
 xen/include/asm-x86/setup.h |  3 ++
 4 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 3a250cb..0e49358 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1427,19 +1427,21 @@ enabling more sockets and cores to go into deeper sleep 
states.
 
 Set the serial transmit buffer size.
 
-### smep
-> `= `
+### smap
+> `=  | hvm`
 
 > Default: `true`
 
-Flag to enable Supervisor Mode Execution Protection
+Flag to enable Supervisor Mode Access Prevention
+Using `smap=hvm` to enable SMAP for HVM guests only.
 
-### smap
-> `= `
+### smep
+> `=  | hvm`
 
 > Default: `true`
 
-Flag to enable Supervisor Mode Access Prevention
+Flag to enable Supervisor Mode Execution Protection
+Using `smep=hvm` to enable SMEP for HVM guests only.
 
 ### snb\_igd\_quirk
 > `=  | cap | `
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 38e34bd..afa16b8 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -4,6 +4,7 @@
 #include 
 #include 
 #include 
+#include 
 
 const uint32_t known_features[] = INIT_KNOWN_FEATURES;
 const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
@@ -118,6 +119,10 @@ static void __init calculate_pv_featureset(void)
 __set_bit(X86_FEATURE_HTT, pv_featureset);
 __set_bit(X86_FEATURE_X2APIC, pv_featureset);
 __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
+if ( smep_hvm_only )
+__clear_bit(X86_FEATURE_SMEP, pv_featureset);
+if ( smap_hvm_only )
+__clear_bit(X86_FEATURE_SMAP, pv_featureset);
 
 sanitise_featureset(pv_featureset);
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 217c775..625b9b4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -61,13 +61,19 @@ boolean_param("nosmp", opt_nosmp);
 static unsigned int __initdata max_cpus;
 integer_param("maxcpus", max_cpus);
 
-/* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
-static bool_t __initdata opt_smep = 1;
-boolean_param("smep", opt_smep);
-
-/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata opt_smap = 1;
-boolean_param("smap", opt_smap);
+/* Supervisor Mode Execution Protection (default on). */
+/* "smep=on": Enable SMEP for Xen and guests. */
+/* "smep=hvm": Enable SMEP for HVM only.  */
+/* "smep=off": Disable SMEP for Xen and guests.   */
+static void parse_smep_param(char *s);
+custom_param("smep", parse_smep_param);
+
+/* Supervisor Mode Access Prevention (default on). */
+/* "smep=on": Enable SMAP for Xen and guests.  */
+/* "smep=hvm": Enable SMAP for HVM only.   */
+/* "smep=off": Disable SMAP for Xen and guests.*/
+static void parse_smap_param(char *s);
+custom_param("smap", parse_smap_param);
 
 unsigned long __read_mostly cr4_pv32_mask;
 
@@ -111,6 +117,34 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 
0, 0, -1 };
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
+static bool_t __initdata opt_smep = 1;
+bool_t __initdata smep_hvm_only = 0;
+static void __init parse_smep_param(char *s)
+{
+if ( !parse_bool(s) )
+{
+opt_smep = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+smep_hvm_only = 1;
+}
+}
+
+static bool_t __initdata opt_smap = 1;
+bool_t __initdata smap_hvm_only = 0;
+static void __init parse_smap_param(char *s)
+{
+if ( !parse_bool(s) )
+{
+opt_smap = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+smap_hvm_only = 1;
+}
+}
+
 bool_t __read_mostly acpi_disabled;
 bool_t __initdata acpi_force;
 static char __initdata acpi_param[10] = "";
@@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 if ( !opt_smep )
 setup_clear_cpu_cap(X86_FEATURE_SMEP);
 if ( cpu_has_smep )
+{
 set_in_cr4(X86_CR4_SMEP);
+if ( smep_hvm_only )
+write_cr4(read_cr4() & ~X86_CR4_SMEP);
+}
 
 if ( !opt_smap )
 setup_clear_cpu_cap(X86_FEATURE_SMAP);
 if ( cpu_has_smap )
+{
 set_in_cr4(X86_CR4_SMAP);
+if ( smap_hvm_only )
+write_cr4(read_cr4() & ~X86_CR4_SMAP);
+}
 
 cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
 
@@ -1570,7 +1612,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)

[Xen-devel] [PATCH V2] xen: support enabling SMEP/SMAP for HVM only

2016-08-10 Thread He Chen 
Enhance "smep" and "smap" command line options to support enabling SMEP
or SMAP for HVM only with allowing "hvm" as a value.

Signed-off-by: He Chen 
---
Changes in V2:
* Allow "hvm" as a value to "smep" and "smap" command line options.
* Clear SMEP/SMAP CPUID bits for pv guests if they are set to hvm only.
* Refine docs.
* Rewrite commit message.
---
 docs/misc/xen-command-line.markdown | 14 +
 xen/arch/x86/cpuid.c|  5 
 xen/arch/x86/setup.c| 58 -
 xen/include/asm-x86/setup.h |  3 ++
 4 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 3a250cb..0e49358 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1427,19 +1427,21 @@ enabling more sockets and cores to go into deeper sleep 
states.
 
 Set the serial transmit buffer size.
 
-### smep
-> `= `
+### smap
+> `=  | hvm`
 
 > Default: `true`
 
-Flag to enable Supervisor Mode Execution Protection
+Flag to enable Supervisor Mode Access Prevention
+Using `smap=hvm` to enable SMAP for HVM guests only.
 
-### smap
-> `= `
+### smep
+> `=  | hvm`
 
 > Default: `true`
 
-Flag to enable Supervisor Mode Access Prevention
+Flag to enable Supervisor Mode Execution Protection
+Using `smep=hvm` to enable SMEP for HVM guests only.
 
 ### snb\_igd\_quirk
 > `=  | cap | `
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 38e34bd..afa16b8 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -4,6 +4,7 @@
 #include 
 #include 
 #include 
+#include 
 
 const uint32_t known_features[] = INIT_KNOWN_FEATURES;
 const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
@@ -118,6 +119,10 @@ static void __init calculate_pv_featureset(void)
 __set_bit(X86_FEATURE_HTT, pv_featureset);
 __set_bit(X86_FEATURE_X2APIC, pv_featureset);
 __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
+if ( smep_hvm_only )
+__clear_bit(X86_FEATURE_SMEP, pv_featureset);
+if ( smap_hvm_only )
+__clear_bit(X86_FEATURE_SMAP, pv_featureset);
 
 sanitise_featureset(pv_featureset);
 }
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 217c775..625b9b4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -61,13 +61,19 @@ boolean_param("nosmp", opt_nosmp);
 static unsigned int __initdata max_cpus;
 integer_param("maxcpus", max_cpus);
 
-/* smep: Enable/disable Supervisor Mode Execution Protection (default on). */
-static bool_t __initdata opt_smep = 1;
-boolean_param("smep", opt_smep);
-
-/* smap: Enable/disable Supervisor Mode Access Prevention (default on). */
-static bool_t __initdata opt_smap = 1;
-boolean_param("smap", opt_smap);
+/* Supervisor Mode Execution Protection (default on). */
+/* "smep=on": Enable SMEP for Xen and guests. */
+/* "smep=hvm": Enable SMEP for HVM only.  */
+/* "smep=off": Disable SMEP for Xen and guests.   */
+static void parse_smep_param(char *s);
+custom_param("smep", parse_smep_param);
+
+/* Supervisor Mode Access Prevention (default on). */
+/* "smep=on": Enable SMAP for Xen and guests.  */
+/* "smep=hvm": Enable SMAP for HVM only.   */
+/* "smep=off": Disable SMAP for Xen and guests.*/
+static void parse_smap_param(char *s);
+custom_param("smap", parse_smap_param);
 
 unsigned long __read_mostly cr4_pv32_mask;
 
@@ -111,6 +117,34 @@ struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 
0, 0, -1 };
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
+static bool_t __initdata opt_smep = 1;
+bool_t __initdata smep_hvm_only = 0;
+static void __init parse_smep_param(char *s)
+{
+if ( !parse_bool(s) )
+{
+opt_smep = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+smep_hvm_only = 1;
+}
+}
+
+static bool_t __initdata opt_smap = 1;
+bool_t __initdata smap_hvm_only = 0;
+static void __init parse_smap_param(char *s)
+{
+if ( !parse_bool(s) )
+{
+opt_smap = 0;
+}
+else if ( !strcmp(s, "hvm") )
+{
+smap_hvm_only = 1;
+}
+}
+
 bool_t __read_mostly acpi_disabled;
 bool_t __initdata acpi_force;
 static char __initdata acpi_param[10] = "";
@@ -1404,12 +1438,20 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 if ( !opt_smep )
 setup_clear_cpu_cap(X86_FEATURE_SMEP);
 if ( cpu_has_smep )
+{
 set_in_cr4(X86_CR4_SMEP);
+if ( smep_hvm_only )
+write_cr4(read_cr4() & ~X86_CR4_SMEP);
+}
 
 if ( !opt_smap )
 setup_clear_cpu_cap(X86_FEATURE_SMAP);
 if ( cpu_has_smap )
+{
 set_in_cr4(X86_CR4_SMAP);
+if ( smap_hvm_only )
+write_cr4(read_cr4() & ~X86_CR4_SMAP);
+}
 
 cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
 
@@ -1570,7 +1612,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)