[PATCH 0/3] x86/pv: Start to trim 32bit support

[PATCH 0/3] x86/pv: Start to trim 32bit support

[Andrew Cooper]

Downstreams may want this for either security or performance reasons.  Offer
some options, and take advantage of some of the very low hanging fruit it
offers.

There is plenty more incremental cleanup which can be done at a later point.

Andrew Cooper (3):
  x86/pv: Options to disable and/or compile out 32bit PV support
  x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds
  x86/pv: Compile out compat_gdt in !CONFIG_PV builds

 docs/misc/xen-command-line.pandoc | 12 +++++++++++-
 xen/arch/x86/Kconfig              | 16 +++++++++++++++
 xen/arch/x86/cpu/common.c         |  5 +++--
 xen/arch/x86/desc.c               |  2 ++
 xen/arch/x86/domctl.c             |  4 ++--
 xen/arch/x86/pv/domain.c          | 41 ++++++++++++++++++++++++++++++++++++---
 xen/arch/x86/pv/hypercall.c       |  2 ++
 xen/arch/x86/setup.c              |  9 +++++++--
 xen/arch/x86/smpboot.c            |  5 ++++-
 xen/arch/x86/traps.c              | 10 +++++++---
 xen/arch/x86/x86_64/asm-offsets.c |  4 +++-
 xen/include/asm-x86/domain.h      |  4 ++--
 xen/include/asm-x86/pv/domain.h   |  6 ++++++
 xen/include/xen/sched.h           | 15 ++++++++++++--
 14 files changed, 116 insertions(+), 19 deletions(-)

-- 
2.11.0

[PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

This is the start of some performance and security-hardening improvements,
based on the fact that 32bit PV guests are few and far between these days.

Ring1 is full or architectural corner cases, such as counting as supervisor
from a paging point of view.  This accounts for a substantial performance hit
on processors from the last 8 years (adjusting SMEP/SMAP on every privilege
transition), and the gap is only going to get bigger with new hardware
features.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

There is a series I can't quite post yet which wants to conditionally turn
opt_pv32 off, which is why I've put it straight in in an int8_t form rather
than a straight boolean form.
---
 docs/misc/xen-command-line.pandoc | 12 +++++++++++-
 xen/arch/x86/Kconfig              | 16 ++++++++++++++++
 xen/arch/x86/pv/domain.c          | 35 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/setup.c              |  9 +++++++--
 xen/include/asm-x86/pv/domain.h   |  6 ++++++
 5 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index acd0b3d994..ee12b0f53f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1694,7 +1694,17 @@ The following resources are available:
     CDP, one COS will corespond two CBMs other than one with CAT, due to the
     sum of CBMs is fixed, that means actual `cos_max` in use will automatically
     reduce to half when CDP is enabled.
-	
+
+### pv
+    = List of [ 32=<bool> ]
+
+    Applicability: x86
+
+Controls for aspects of PV guest support.
+
+*   The `32` boolean controls whether 32bit PV guests can be created.  It
+    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
+
 ### pv-linear-pt (x86)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 8149362bde..4c52197de3 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -49,6 +49,22 @@ config PV
 
 	  If unsure, say Y.
 
+config PV32
+	bool "Support for 32bit PV guests"
+	depends on PV
+	default y
+	---help---
+	  The 32bit PV ABI uses Ring1, an area of the x86 architecture which
+	  was deprecated and mostly removed in the AMD64 spec.  As a result,
+	  it occasionally conflicts with newer x86 hardware features, causing
+	  overheads for Xen to maintain backwards compatibility.
+
+	  People may wish to disable 32bit PV guests for attack surface
+	  reduction, or performance reasons.  Backwards compatibility can be
+	  provided via the PV Shim mechanism.
+
+	  If unsure, say Y.
+
 config PV_LINEAR_PT
        bool "Support for PV linear pagetables"
        depends on PV
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 70fae43965..47a0db082f 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -16,6 +16,39 @@
 #include <asm/pv/domain.h>
 #include <asm/shadow.h>
 
+#ifdef CONFIG_PV32
+int8_t __read_mostly opt_pv32 = -1;
+#endif
+
+static int parse_pv(const char *s)
+{
+    const char *ss;
+    int val, rc = 0;
+
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( (val = parse_boolean("32", s, ss)) >= 0 )
+        {
+#ifdef CONFIG_PV32
+            opt_pv32 = val;
+#else
+            printk(XENLOG_INFO
+                   "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
+#endif
+        }
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
+
+    return rc;
+}
+custom_param("pv", parse_pv);
+
 static __read_mostly enum {
     PCID_OFF,
     PCID_ALL,
@@ -174,6 +207,8 @@ int switch_compat(struct domain *d)
 
     BUILD_BUG_ON(offsetof(struct shared_info, vcpu_info) != 0);
 
+    if ( !opt_pv32 )
+        return -EOPNOTSUPP;
     if ( is_hvm_domain(d) || domain_tot_pages(d) != 0 )
         return -EACCES;
     if ( is_pv_32bit_domain(d) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 885919d5c3..c50aefb2de 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -53,6 +53,7 @@
 #include <asm/spec_ctrl.h>
 #include <asm/guest.h>
 #include <asm/microcode.h>
+#include <asm/pv/domain.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
@@ -1875,8 +1876,12 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
     {
         snprintf(s, sizeof(s), "xen-%d.%d-x86_64 ", major, minor);
         safe_strcat(*info, s);
-        snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
-        safe_strcat(*info, s);
+
+        if ( opt_pv32 )
+        {
+            snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
+            safe_strcat(*info, s);
+        }
     }
     if ( hvm_enabled )
     {
diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
index 7a69bfb303..df9716ff26 100644
--- a/xen/include/asm-x86/pv/domain.h
+++ b/xen/include/asm-x86/pv/domain.h
@@ -23,6 +23,12 @@
 
 #include <xen/sched.h>
 
+#ifdef CONFIG_PV32
+extern int8_t opt_pv32;
+#else
+# define opt_pv32 false
+#endif
+
 /*
  * PCID values for the address spaces of 64-bit pv domains:
  *
-- 
2.11.0

[PATCH 2/3] x86/pv: Short-circuit is_pv_{32, 64}bit_domain() in !CONFIG_PV32 builds

[Andrew Cooper]

... and move arch.is_32bit_pv into the pv union while at it.

Bloat-o-meter reports the following net savings with some notable differences
highlighted:

  add/remove: 4/6 grow/shrink: 5/76 up/down: 1955/-18792 (-16837)
  Function                                     old     new   delta
  ...
  pv_vcpu_initialise                           411     158    -253
  guest_cpuid                                 1837    1584    -253
  pv_hypercall                                 579     297    -282
  check_descriptor                             427     130    -297
  _get_page_type                              5915    5202    -713
  arch_get_info_guest                         2225    1195   -1030
  context_switch                              3831    2635   -1196
  dom0_construct_pv                          10284    8939   -1345
  arch_set_info_guest                         5564    3267   -2297
  Total: Before=3079563, After=3062726, chg -0.55%

In principle, DOMAIN_is_32bit_pv should be based on CONFIG_PV32, but the
assembly code is going to need further untangling before that becomes easy to
do.  For now, use CONFIG_PV as missed accidentally by c/s ec651bd2460 "x86:
make entry point code build when !CONFIG_PV".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domctl.c             |  4 ++--
 xen/arch/x86/pv/domain.c          |  6 +++---
 xen/arch/x86/pv/hypercall.c       |  2 ++
 xen/arch/x86/x86_64/asm-offsets.c |  4 +++-
 xen/include/asm-x86/domain.h      |  4 ++--
 xen/include/xen/sched.h           | 15 +++++++++++++--
 6 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index add70126b9..3822dd7fd1 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -576,8 +576,8 @@ long arch_do_domctl(
             ret = -EOPNOTSUPP;
         else if ( is_pv_domain(d) )
         {
-            if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
-                 ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
+            if ( ((domctl->u.address_size.size == 64) && !d->arch.pv.is_32bit) ||
+                 ((domctl->u.address_size.size == 32) && d->arch.pv.is_32bit) )
                 ret = 0;
             else if ( domctl->u.address_size.size == 32 )
                 ret = switch_compat(d);
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 47a0db082f..e0977bfbd7 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
         return 0;
 
     d->arch.has_32bit_shinfo = 1;
-    d->arch.is_32bit_pv = 1;
+    d->arch.pv.is_32bit = 1;
 
     for_each_vcpu( d, v )
     {
@@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
     return 0;
 
  undo_and_fail:
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
     for_each_vcpu( d, v )
     {
         free_compat_arg_xlat(v);
@@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
     d->arch.ctxt_switch = &pv_csw;
 
     /* 64-bit PV guest by default. */
-    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
+    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
 
     d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
 
diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 17ddf9ea1f..32d90a543f 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -302,6 +302,7 @@ void pv_ring3_init_hypercall_page(void *p)
     }
 }
 
+#ifdef CONFIG_PV32
 void pv_ring1_init_hypercall_page(void *p)
 {
     unsigned int i;
@@ -329,6 +330,7 @@ void pv_ring1_init_hypercall_page(void *p)
         *(u8  *)(p+ 7) = 0xc3;    /* ret */
     }
 }
+#endif
 
 /*
  * Local variables:
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index 500df7a3e7..9f66a69be7 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -98,8 +98,10 @@ void __dummy__(void)
     OFFSET(VCPU_nsvm_hap_enabled, struct vcpu, arch.hvm.nvcpu.u.nsvm.ns_hap_enabled);
     BLANK();
 
-    OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.is_32bit_pv);
+#ifdef CONFIG_PV
+    OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
     BLANK();
+#endif
 
     OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
     OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4192c636b1..ae155d6522 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -254,6 +254,8 @@ struct pv_domain
 
     atomic_t nr_l4_pages;
 
+    /* Is a 32-bit PV guest? */
+    bool is_32bit;
     /* XPTI active? */
     bool xpti;
     /* Use PCID feature? */
@@ -333,8 +335,6 @@ struct arch_domain
     /* NB. protected by d->event_lock and by irq_desc[irq].lock */
     struct radix_tree_root irq_pirq;
 
-    /* Is a 32-bit PV (non-HVM) guest? */
-    bool_t is_32bit_pv;
     /* Is shared-info page in 32-bit format? */
     bool_t has_32bit_shinfo;
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 195e7ee583..6101761d25 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -985,7 +985,11 @@ static always_inline bool is_pv_vcpu(const struct vcpu *v)
 #ifdef CONFIG_COMPAT
 static always_inline bool is_pv_32bit_domain(const struct domain *d)
 {
-    return is_pv_domain(d) && d->arch.is_32bit_pv;
+#ifdef CONFIG_PV32
+    return is_pv_domain(d) && d->arch.pv.is_32bit;
+#else
+    return false;
+#endif
 }
 
 static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
@@ -995,7 +999,14 @@ static always_inline bool is_pv_32bit_vcpu(const struct vcpu *v)
 
 static always_inline bool is_pv_64bit_domain(const struct domain *d)
 {
-    return is_pv_domain(d) && !d->arch.is_32bit_pv;
+    if ( !is_pv_domain(d) )
+        return false;
+
+#ifdef CONFIG_PV32
+    return !d->arch.pv.is_32bit;
+#else
+    return true;
+#endif
 }
 
 static always_inline bool is_pv_64bit_vcpu(const struct vcpu *v)
-- 
2.11.0

[PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Andrew Cooper]

There is no need for the Compat GDT if there are no 32bit PV guests.  This
saves 4k per online CPU

Bloat-o-meter reports the following savings in Xen itself:

  add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
  Function                                     old     new   delta
  cpu_smpboot_free                            1249    1256      +7
  per_cpu__compat_gdt_l1e                        8       -      -8
  per_cpu__compat_gdt                            8       -      -8
  init_idt_traps                               442     420     -22
  load_system_tables                           414     364     -50
  trap_init                                    444     280    -164
  cpu_smpboot_callback                        1255     991    -264
  boot_compat_gdt                             4096       -   -4096
  Total: Before=3062726, After=3058121, chg -0.15%

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

The increase in cpu_smpboot_free() appears to be a consequence of a totally
different layout of basic blocks.
---
 xen/arch/x86/cpu/common.c |  5 +++--
 xen/arch/x86/desc.c       |  2 ++
 xen/arch/x86/smpboot.c    |  5 ++++-
 xen/arch/x86/traps.c      | 10 +++++++---
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 1b33f1ed71..7b093cb421 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -752,8 +752,9 @@ void load_system_tables(void)
 
 	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
 			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
-	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
-			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
+	if ( IS_ENABLED(CONFIG_PV32) )
+		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
+				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
 
 	per_cpu(full_gdt_loaded, cpu) = false;
 	lgdt(&gdtr);
diff --git a/xen/arch/x86/desc.c b/xen/arch/x86/desc.c
index dfeb1beaa8..39080ca672 100644
--- a/xen/arch/x86/desc.c
+++ b/xen/arch/x86/desc.c
@@ -55,6 +55,7 @@ seg_desc_t boot_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
     [SEL2GDT(PER_CPU_SELECTOR)] =     { 0x0000910000000000 },
 };
 
+#ifdef CONFIG_PV32
 __section(".data.page_aligned") __aligned(PAGE_SIZE)
 seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
 {
@@ -83,6 +84,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
     /* 0xe060 - per-CPU entry (limit == cpu) */
     [SEL2GDT(PER_CPU_SELECTOR)] =     { 0x0000910000000000 },
 };
+#endif
 
 /*
  * Used by each CPU as it starts up, to enter C with a suitable %cs.
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 09264b02d1..f9f63e496f 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -959,7 +959,8 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
             free_domheap_page(mfn_to_page(mfn));
     }
 
-    FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
+    if ( IS_ENABLED(CONFIG_PV32) )
+        FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
 
     if ( remove )
     {
@@ -1001,6 +1002,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     BUILD_BUG_ON(NR_CPUS > 0x10000);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
+#ifdef CONFIG_PV32
     per_cpu(compat_gdt, cpu) = gdt = alloc_xenheap_pages(0, memflags);
     if ( gdt == NULL )
         goto out;
@@ -1008,6 +1010,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
         l1e_from_pfn(virt_to_mfn(gdt), __PAGE_HYPERVISOR_RW);
     memcpy(gdt, boot_compat_gdt, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
+#endif
 
     if ( idt_tables[cpu] == NULL )
         idt_tables[cpu] = alloc_xenheap_pages(0, memflags);
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e838846c6b..0bcf554e93 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -100,8 +100,10 @@ static DEFINE_PER_CPU(unsigned long, last_extable_addr);
 
 DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, gdt);
 DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, gdt_l1e);
+#ifdef CONFIG_PV32
 DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, compat_gdt);
 DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, compat_gdt_l1e);
+#endif
 
 /* Master table, used by CPU0. */
 idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
@@ -1999,7 +2001,8 @@ void __init init_idt_traps(void)
     idt_tables[0] = idt_table;
 
     this_cpu(gdt) = boot_gdt;
-    this_cpu(compat_gdt) = boot_compat_gdt;
+    if ( IS_ENABLED(CONFIG_PV32) )
+        this_cpu(compat_gdt) = boot_compat_gdt;
 }
 
 extern void (*const autogen_entrypoints[X86_NR_VECTORS])(void);
@@ -2030,8 +2033,9 @@ void __init trap_init(void)
     /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
     this_cpu(gdt_l1e) =
         l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
-    this_cpu(compat_gdt_l1e) =
-        l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
+    if ( IS_ENABLED(CONFIG_PV32) )
+        this_cpu(compat_gdt_l1e) =
+            l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
 
     percpu_traps_init();
 
-- 
2.11.0

Re: [PATCH 0/3] x86/pv: Start to trim 32bit support

[Wei Liu]

On Fri, Apr 17, 2020 at 04:50:01PM +0100, Andrew Cooper wrote:
> Downstreams may want this for either security or performance reasons.  Offer
> some options, and take advantage of some of the very low hanging fruit it
> offers.
> 
> There is plenty more incremental cleanup which can be done at a later point.
> 
> Andrew Cooper (3):
>   x86/pv: Options to disable and/or compile out 32bit PV support
>   x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds
>   x86/pv: Compile out compat_gdt in !CONFIG_PV builds

Reviewed-by: Wei Liu <wl@xen.org>

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Roger Pau Monné]

On Fri, Apr 17, 2020 at 04:50:02PM +0100, Andrew Cooper wrote:
> This is the start of some performance and security-hardening improvements,
> based on the fact that 32bit PV guests are few and far between these days.
> 
> Ring1 is full or architectural corner cases, such as counting as supervisor
                ^ of
> from a paging point of view.  This accounts for a substantial performance hit
> on processors from the last 8 years (adjusting SMEP/SMAP on every privilege
> transition), and the gap is only going to get bigger with new hardware
> features.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> There is a series I can't quite post yet which wants to conditionally turn
> opt_pv32 off, which is why I've put it straight in in an int8_t form rather

s/in in/in/

> than a straight boolean form.
> ---
>  docs/misc/xen-command-line.pandoc | 12 +++++++++++-
>  xen/arch/x86/Kconfig              | 16 ++++++++++++++++
>  xen/arch/x86/pv/domain.c          | 35 +++++++++++++++++++++++++++++++++++
>  xen/arch/x86/setup.c              |  9 +++++++--
>  xen/include/asm-x86/pv/domain.h   |  6 ++++++
>  5 files changed, 75 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
> index acd0b3d994..ee12b0f53f 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1694,7 +1694,17 @@ The following resources are available:
>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>      sum of CBMs is fixed, that means actual `cos_max` in use will automatically
>      reduce to half when CDP is enabled.
> -	
> +
> +### pv
> +    = List of [ 32=<bool> ]
> +
> +    Applicability: x86
> +
> +Controls for aspects of PV guest support.
> +
> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
> +
>  ### pv-linear-pt (x86)
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 8149362bde..4c52197de3 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -49,6 +49,22 @@ config PV
>  
>  	  If unsure, say Y.
>  
> +config PV32
> +	bool "Support for 32bit PV guests"
> +	depends on PV
> +	default y
> +	---help---
> +	  The 32bit PV ABI uses Ring1, an area of the x86 architecture which
> +	  was deprecated and mostly removed in the AMD64 spec.  As a result,
> +	  it occasionally conflicts with newer x86 hardware features, causing
> +	  overheads for Xen to maintain backwards compatibility.
> +
> +	  People may wish to disable 32bit PV guests for attack surface
> +	  reduction, or performance reasons.  Backwards compatibility can be
> +	  provided via the PV Shim mechanism.
> +
> +	  If unsure, say Y.
> +
>  config PV_LINEAR_PT
>         bool "Support for PV linear pagetables"
>         depends on PV
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index 70fae43965..47a0db082f 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -16,6 +16,39 @@
>  #include <asm/pv/domain.h>
>  #include <asm/shadow.h>
>  
> +#ifdef CONFIG_PV32
> +int8_t __read_mostly opt_pv32 = -1;
> +#endif
> +
> +static int parse_pv(const char *s)

__init

With that:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jan Beulich]

On 17.04.2020 17:50, Andrew Cooper wrote:
> This is the start of some performance and security-hardening improvements,
> based on the fact that 32bit PV guests are few and far between these days.
> 
> Ring1 is full or architectural corner cases, such as counting as supervisor

... full of ...

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1694,7 +1694,17 @@ The following resources are available:
>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>      sum of CBMs is fixed, that means actual `cos_max` in use will automatically
>      reduce to half when CDP is enabled.
> -	
> +
> +### pv
> +    = List of [ 32=<bool> ]
> +
> +    Applicability: x86
> +
> +Controls for aspects of PV guest support.
> +
> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.

Rather than ignoring in the way you do, how about ...

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -16,6 +16,39 @@
>  #include <asm/pv/domain.h>
>  #include <asm/shadow.h>
>  
> +#ifdef CONFIG_PV32
> +int8_t __read_mostly opt_pv32 = -1;
> +#endif
> +
> +static int parse_pv(const char *s)
> +{
> +    const char *ss;
> +    int val, rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( (val = parse_boolean("32", s, ss)) >= 0 )
> +        {
> +#ifdef CONFIG_PV32
> +            opt_pv32 = val;
> +#else
> +            printk(XENLOG_INFO
> +                   "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
> +#endif
> +        }

... moving the #ifdef ahead of the if(), and the #endif here (may
want converting to "else if" with a placeholder if(0) for this
purpose), with the separate printk() dropped? I'm in particular
concerned that we may gain a large number of such printk()s over
time, if we added them in such cases. See parse_iommu_param() for
how I'd prefer things to look like in the long run.

Jan

Re: [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds

[Jan Beulich]

On 17.04.2020 17:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>          return 0;
>  
>      d->arch.has_32bit_shinfo = 1;
> -    d->arch.is_32bit_pv = 1;
> +    d->arch.pv.is_32bit = 1;
>  
>      for_each_vcpu( d, v )
>      {
> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>      return 0;
>  
>   undo_and_fail:
> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>      for_each_vcpu( d, v )
>      {
>          free_compat_arg_xlat(v);
> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>      d->arch.ctxt_switch = &pv_csw;
>  
>      /* 64-bit PV guest by default. */
> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;

Switch to true/false while you're touching these?

Jan

Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Jan Beulich]

On 17.04.2020 17:50, Andrew Cooper wrote:
> There is no need for the Compat GDT if there are no 32bit PV guests.  This
> saves 4k per online CPU
> 
> Bloat-o-meter reports the following savings in Xen itself:
> 
>   add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
>   Function                                     old     new   delta
>   cpu_smpboot_free                            1249    1256      +7
>   per_cpu__compat_gdt_l1e                        8       -      -8
>   per_cpu__compat_gdt                            8       -      -8
>   init_idt_traps                               442     420     -22
>   load_system_tables                           414     364     -50
>   trap_init                                    444     280    -164
>   cpu_smpboot_callback                        1255     991    -264
>   boot_compat_gdt                             4096       -   -4096
>   Total: Before=3062726, After=3058121, chg -0.15%
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The increase in cpu_smpboot_free() appears to be a consequence of a totally
> different layout of basic blocks.
> ---
>  xen/arch/x86/cpu/common.c |  5 +++--
>  xen/arch/x86/desc.c       |  2 ++
>  xen/arch/x86/smpboot.c    |  5 ++++-
>  xen/arch/x86/traps.c      | 10 +++++++---
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 1b33f1ed71..7b093cb421 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -752,8 +752,9 @@ void load_system_tables(void)
>  
>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
> -	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
> -			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
> +	if ( IS_ENABLED(CONFIG_PV32) )
> +		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
> +				 sizeof(*tss) - 1, SYS_DESC_tss_busy);

Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then.

Jan

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jan Beulich]

On 17.04.2020 17:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -49,6 +49,22 @@ config PV
>  
>  	  If unsure, say Y.
>  
> +config PV32
> +	bool "Support for 32bit PV guests"
> +	depends on PV
> +	default y

I guess I can see why you don't want an EXPERT dependency here, but
I guess we really need to settle on our (as a community) position
on the growth of varying configs people can build and expect to be
supported.

Jan

Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Andrew Cooper]

On 20/04/2020 15:12, Jan Beulich wrote:
> On 17.04.2020 17:50, Andrew Cooper wrote:
>> There is no need for the Compat GDT if there are no 32bit PV guests.  This
>> saves 4k per online CPU
>>
>> Bloat-o-meter reports the following savings in Xen itself:
>>
>>   add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
>>   Function                                     old     new   delta
>>   cpu_smpboot_free                            1249    1256      +7
>>   per_cpu__compat_gdt_l1e                        8       -      -8
>>   per_cpu__compat_gdt                            8       -      -8
>>   init_idt_traps                               442     420     -22
>>   load_system_tables                           414     364     -50
>>   trap_init                                    444     280    -164
>>   cpu_smpboot_callback                        1255     991    -264
>>   boot_compat_gdt                             4096       -   -4096
>>   Total: Before=3062726, After=3058121, chg -0.15%
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> The increase in cpu_smpboot_free() appears to be a consequence of a totally
>> different layout of basic blocks.
>> ---
>>  xen/arch/x86/cpu/common.c |  5 +++--
>>  xen/arch/x86/desc.c       |  2 ++
>>  xen/arch/x86/smpboot.c    |  5 ++++-
>>  xen/arch/x86/traps.c      | 10 +++++++---
>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>> index 1b33f1ed71..7b093cb421 100644
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -752,8 +752,9 @@ void load_system_tables(void)
>>  
>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>> -	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>> -			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>> +	if ( IS_ENABLED(CONFIG_PV32) )
>> +		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>> +				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
> Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then.

Doing it like this specifically ensures that there is never a case where
things are half configured.

I don't think it is wise to suggest that making opt_pv32 runtime
configurable might work.

~Andrew

Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Jan Beulich]

On 20.04.2020 16:39, Andrew Cooper wrote:
> On 20/04/2020 15:12, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> There is no need for the Compat GDT if there are no 32bit PV guests.  This
>>> saves 4k per online CPU
>>>
>>> Bloat-o-meter reports the following savings in Xen itself:
>>>
>>>   add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
>>>   Function                                     old     new   delta
>>>   cpu_smpboot_free                            1249    1256      +7
>>>   per_cpu__compat_gdt_l1e                        8       -      -8
>>>   per_cpu__compat_gdt                            8       -      -8
>>>   init_idt_traps                               442     420     -22
>>>   load_system_tables                           414     364     -50
>>>   trap_init                                    444     280    -164
>>>   cpu_smpboot_callback                        1255     991    -264
>>>   boot_compat_gdt                             4096       -   -4096
>>>   Total: Before=3062726, After=3058121, chg -0.15%
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> The increase in cpu_smpboot_free() appears to be a consequence of a totally
>>> different layout of basic blocks.
>>> ---
>>>  xen/arch/x86/cpu/common.c |  5 +++--
>>>  xen/arch/x86/desc.c       |  2 ++
>>>  xen/arch/x86/smpboot.c    |  5 ++++-
>>>  xen/arch/x86/traps.c      | 10 +++++++---
>>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>>> index 1b33f1ed71..7b093cb421 100644
>>> --- a/xen/arch/x86/cpu/common.c
>>> +++ b/xen/arch/x86/cpu/common.c
>>> @@ -752,8 +752,9 @@ void load_system_tables(void)
>>>  
>>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>>> -	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>> -			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>> +	if ( IS_ENABLED(CONFIG_PV32) )
>>> +		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>> +				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>> Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then.
> 
> Doing it like this specifically ensures that there is never a case where
> things are half configured.

But this way you set up something in the GDT that's never going
to be used when "pv=no-32". Why leave a TSS accessible that we
don't need?

> I don't think it is wise to suggest that making opt_pv32 runtime
> configurable might work.

I didn't suggest (nor even consider) runtime changing of this
setting. If we wanted, _that_ would be what might require using
code as you have it right now (if we wanted to avoid setting
this up at the time the setting gets flipped from false to true).

Jan

Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Andrew Cooper]

On 20/04/2020 16:47, Jan Beulich wrote:
> On 20.04.2020 16:39, Andrew Cooper wrote:
>> On 20/04/2020 15:12, Jan Beulich wrote:
>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>> There is no need for the Compat GDT if there are no 32bit PV guests.  This
>>>> saves 4k per online CPU
>>>>
>>>> Bloat-o-meter reports the following savings in Xen itself:
>>>>
>>>>   add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
>>>>   Function                                     old     new   delta
>>>>   cpu_smpboot_free                            1249    1256      +7
>>>>   per_cpu__compat_gdt_l1e                        8       -      -8
>>>>   per_cpu__compat_gdt                            8       -      -8
>>>>   init_idt_traps                               442     420     -22
>>>>   load_system_tables                           414     364     -50
>>>>   trap_init                                    444     280    -164
>>>>   cpu_smpboot_callback                        1255     991    -264
>>>>   boot_compat_gdt                             4096       -   -4096
>>>>   Total: Before=3062726, After=3058121, chg -0.15%
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> The increase in cpu_smpboot_free() appears to be a consequence of a totally
>>>> different layout of basic blocks.
>>>> ---
>>>>  xen/arch/x86/cpu/common.c |  5 +++--
>>>>  xen/arch/x86/desc.c       |  2 ++
>>>>  xen/arch/x86/smpboot.c    |  5 ++++-
>>>>  xen/arch/x86/traps.c      | 10 +++++++---
>>>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>>>> index 1b33f1ed71..7b093cb421 100644
>>>> --- a/xen/arch/x86/cpu/common.c
>>>> +++ b/xen/arch/x86/cpu/common.c
>>>> @@ -752,8 +752,9 @@ void load_system_tables(void)
>>>>  
>>>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>>>> -	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>>> -			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>>> +	if ( IS_ENABLED(CONFIG_PV32) )
>>>> +		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>>> +				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>> Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then.
>> Doing it like this specifically ensures that there is never a case where
>> things are half configured.
> But this way you set up something in the GDT that's never going
> to be used when "pv=no-32". Why leave a TSS accessible that we
> don't need?

Defence in depth.

Having it only partially set up is more likely to fail in a security
relevant way, than having it fully set up.

This particular example is poor.  There is no need to have the TSS in
either GDT after the `ltr` instruction at boot for 64bit, because we
don't task switch, but ISTR you requesting that this stayed as-was for
consistency.  (For 32bit Xen, it was strictly necessary for the #DF task
switch to work.)

However, the other logic, particularly the cached l1e pointing at the
percpu compat_gdt is more liable to go wrong in interesting ways.

~Andrew

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

On 20/04/2020 14:47, Roger Pau Monné wrote:
> On Fri, Apr 17, 2020 at 04:50:02PM +0100, Andrew Cooper wrote:
>> This is the start of some performance and security-hardening improvements,
>> based on the fact that 32bit PV guests are few and far between these days.
>>
>> Ring1 is full or architectural corner cases, such as counting as supervisor
>                 ^ of

Already fixed (I spotted it 30s after posting).

>> from a paging point of view.  This accounts for a substantial performance hit
>> on processors from the last 8 years (adjusting SMEP/SMAP on every privilege
>> transition), and the gap is only going to get bigger with new hardware
>> features.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> There is a series I can't quite post yet which wants to conditionally turn
>> opt_pv32 off, which is why I've put it straight in in an int8_t form rather
> s/in in/in/

"in in" is legitimate in some cases, despite it looking awkard.   In
this case, the structure is "straight in", separate from "in an int8_t
form".

If this sentence were for inclusion in the commit message, I'd have
probably spent more effort trying to phrase it differently.

>
>> than a straight boolean form.
>> ---
>>  docs/misc/xen-command-line.pandoc | 12 +++++++++++-
>>  xen/arch/x86/Kconfig              | 16 ++++++++++++++++
>>  xen/arch/x86/pv/domain.c          | 35 +++++++++++++++++++++++++++++++++++
>>  xen/arch/x86/setup.c              |  9 +++++++--
>>  xen/include/asm-x86/pv/domain.h   |  6 ++++++
>>  5 files changed, 75 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
>> index acd0b3d994..ee12b0f53f 100644
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1694,7 +1694,17 @@ The following resources are available:
>>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>>      sum of CBMs is fixed, that means actual `cos_max` in use will automatically
>>      reduce to half when CDP is enabled.
>> -	
>> +
>> +### pv
>> +    = List of [ 32=<bool> ]
>> +
>> +    Applicability: x86
>> +
>> +Controls for aspects of PV guest support.
>> +
>> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
>> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
>> +
>>  ### pv-linear-pt (x86)
>>  > `= <boolean>`
>>  
>> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>> index 8149362bde..4c52197de3 100644
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -49,6 +49,22 @@ config PV
>>  
>>  	  If unsure, say Y.
>>  
>> +config PV32
>> +	bool "Support for 32bit PV guests"
>> +	depends on PV
>> +	default y
>> +	---help---
>> +	  The 32bit PV ABI uses Ring1, an area of the x86 architecture which
>> +	  was deprecated and mostly removed in the AMD64 spec.  As a result,
>> +	  it occasionally conflicts with newer x86 hardware features, causing
>> +	  overheads for Xen to maintain backwards compatibility.
>> +
>> +	  People may wish to disable 32bit PV guests for attack surface
>> +	  reduction, or performance reasons.  Backwards compatibility can be
>> +	  provided via the PV Shim mechanism.
>> +
>> +	  If unsure, say Y.
>> +
>>  config PV_LINEAR_PT
>>         bool "Support for PV linear pagetables"
>>         depends on PV
>> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
>> index 70fae43965..47a0db082f 100644
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -16,6 +16,39 @@
>>  #include <asm/pv/domain.h>
>>  #include <asm/shadow.h>
>>  
>> +#ifdef CONFIG_PV32
>> +int8_t __read_mostly opt_pv32 = -1;
>> +#endif
>> +
>> +static int parse_pv(const char *s)
> __init
>
> With that:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,

~Andrew

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

On 20/04/2020 15:05, Jan Beulich wrote:
> On 17.04.2020 17:50, Andrew Cooper wrote:
>> This is the start of some performance and security-hardening improvements,
>> based on the fact that 32bit PV guests are few and far between these days.
>>
>> Ring1 is full or architectural corner cases, such as counting as supervisor
> ... full of ...
>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1694,7 +1694,17 @@ The following resources are available:
>>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>>      sum of CBMs is fixed, that means actual `cos_max` in use will automatically
>>      reduce to half when CDP is enabled.
>> -	
>> +
>> +### pv
>> +    = List of [ 32=<bool> ]
>> +
>> +    Applicability: x86
>> +
>> +Controls for aspects of PV guest support.
>> +
>> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
>> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
> Rather than ignoring in the way you do, how about ...
>
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -16,6 +16,39 @@
>>  #include <asm/pv/domain.h>
>>  #include <asm/shadow.h>
>>  
>> +#ifdef CONFIG_PV32
>> +int8_t __read_mostly opt_pv32 = -1;
>> +#endif
>> +
>> +static int parse_pv(const char *s)
>> +{
>> +    const char *ss;
>> +    int val, rc = 0;
>> +
>> +    do {
>> +        ss = strchr(s, ',');
>> +        if ( !ss )
>> +            ss = strchr(s, '\0');
>> +
>> +        if ( (val = parse_boolean("32", s, ss)) >= 0 )
>> +        {
>> +#ifdef CONFIG_PV32
>> +            opt_pv32 = val;
>> +#else
>> +            printk(XENLOG_INFO
>> +                   "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
>> +#endif
>> +        }
> ... moving the #ifdef ahead of the if(), and the #endif here (may
> want converting to "else if" with a placeholder if(0) for this
> purpose), with the separate printk() dropped?

In this specific case, it would be even more awkward as there is no use
of val outside of the ifdef.

> I'm in particular
> concerned that we may gain a large number of such printk()s over
> time, if we added them in such cases.

The printk() was a bit of an afterthought, but deliberately avoiding the
-EINVAL path was specifically not.

In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
they should see something other than

(XEN) parameter "pv=no-32" unknown!

I don't think overloading the return value is a clever move, because
then every parse function has to take care of ensuring that -EOPNOTSUPP
(or ENODEV?) never clobbers -EINVAL.

We could have a generic helper which looks like:

static inline void ignored_param(const char *cfg, const char *name,
const char *s, const char *ss)
{
    printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
cfg, name, s, (int)(ss - s));
}

which at least would keep all the users consistent.

> See parse_iommu_param() for
> how I'd prefer things to look like in the long run.

I'm aware of that, just as you are aware of my specific dislike for the
ifndefs, which make the logic opaque and hard to follow.

~Andrew

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jan Beulich]

On 20.04.2020 20:05, Andrew Cooper wrote:
> On 20/04/2020 15:05, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> This is the start of some performance and security-hardening improvements,
>>> based on the fact that 32bit PV guests are few and far between these days.
>>>
>>> Ring1 is full or architectural corner cases, such as counting as supervisor
>> ... full of ...
>>
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1694,7 +1694,17 @@ The following resources are available:
>>>      CDP, one COS will corespond two CBMs other than one with CAT, due to the
>>>      sum of CBMs is fixed, that means actual `cos_max` in use will automatically
>>>      reduce to half when CDP is enabled.
>>> -	
>>> +
>>> +### pv
>>> +    = List of [ 32=<bool> ]
>>> +
>>> +    Applicability: x86
>>> +
>>> +Controls for aspects of PV guest support.
>>> +
>>> +*   The `32` boolean controls whether 32bit PV guests can be created.  It
>>> +    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
>> Rather than ignoring in the way you do, how about ...
>>
>>> --- a/xen/arch/x86/pv/domain.c
>>> +++ b/xen/arch/x86/pv/domain.c
>>> @@ -16,6 +16,39 @@
>>>  #include <asm/pv/domain.h>
>>>  #include <asm/shadow.h>
>>>  
>>> +#ifdef CONFIG_PV32
>>> +int8_t __read_mostly opt_pv32 = -1;
>>> +#endif
>>> +
>>> +static int parse_pv(const char *s)
>>> +{
>>> +    const char *ss;
>>> +    int val, rc = 0;
>>> +
>>> +    do {
>>> +        ss = strchr(s, ',');
>>> +        if ( !ss )
>>> +            ss = strchr(s, '\0');
>>> +
>>> +        if ( (val = parse_boolean("32", s, ss)) >= 0 )
>>> +        {
>>> +#ifdef CONFIG_PV32
>>> +            opt_pv32 = val;
>>> +#else
>>> +            printk(XENLOG_INFO
>>> +                   "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
>>> +#endif
>>> +        }
>> ... moving the #ifdef ahead of the if(), and the #endif here (may
>> want converting to "else if" with a placeholder if(0) for this
>> purpose), with the separate printk() dropped?
> 
> In this specific case, it would be even more awkward as there is no use
> of val outside of the ifdef.

True, but can be dealt with in the body of the suggested placeholder
if(0).

>> I'm in particular
>> concerned that we may gain a large number of such printk()s over
>> time, if we added them in such cases.
> 
> The printk() was a bit of an afterthought, but deliberately avoiding the
> -EINVAL path was specifically not.
> 
> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
> they should see something other than
> 
> (XEN) parameter "pv=no-32" unknown!

Why - to this specific build of Xen the parameter is unknown.

> I don't think overloading the return value is a clever move, because
> then every parse function has to take care of ensuring that -EOPNOTSUPP
> (or ENODEV?) never clobbers -EINVAL.

I didn't suggest overloading the return value. Instead I
specifically want this to go the -EINVAL path.

> We could have a generic helper which looks like:
> 
> static inline void ignored_param(const char *cfg, const char *name,
> const char *s, const char *ss)
> {
>     printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
> cfg, name, s, (int)(ss - s));
> }
> 
> which at least would keep all the users consistent.

Further bloating the binary with (almost) useless string literals.
I'd specifically like to avoid this.

>> See parse_iommu_param() for
>> how I'd prefer things to look like in the long run.
> 
> I'm aware of that, just as you are aware of my specific dislike for the
> ifndefs, which make the logic opaque and hard to follow.

A matter of taste and/or perception, I guess, but yes, I'm aware.
I don't recall a suggestion (from you or anyone else) as to a good
alternative, though. What I'd like to achieve is that command line
options valid only in certain configurations get similar treatment
no matter by whom they were added. It doesn't need to be my way of
handling, but I'm pretty convinced that consistency especially in
such "user interface" aspects is very desirable goal.

Jan

Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Jan Beulich]

On 20.04.2020 19:08, Andrew Cooper wrote:
> On 20/04/2020 16:47, Jan Beulich wrote:
>> On 20.04.2020 16:39, Andrew Cooper wrote:
>>> On 20/04/2020 15:12, Jan Beulich wrote:
>>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>>> There is no need for the Compat GDT if there are no 32bit PV guests.  This
>>>>> saves 4k per online CPU
>>>>>
>>>>> Bloat-o-meter reports the following savings in Xen itself:
>>>>>
>>>>>   add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605)
>>>>>   Function                                     old     new   delta
>>>>>   cpu_smpboot_free                            1249    1256      +7
>>>>>   per_cpu__compat_gdt_l1e                        8       -      -8
>>>>>   per_cpu__compat_gdt                            8       -      -8
>>>>>   init_idt_traps                               442     420     -22
>>>>>   load_system_tables                           414     364     -50
>>>>>   trap_init                                    444     280    -164
>>>>>   cpu_smpboot_callback                        1255     991    -264
>>>>>   boot_compat_gdt                             4096       -   -4096
>>>>>   Total: Before=3062726, After=3058121, chg -0.15%
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: Wei Liu <wl@xen.org>
>>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>>
>>>>> The increase in cpu_smpboot_free() appears to be a consequence of a totally
>>>>> different layout of basic blocks.
>>>>> ---
>>>>>  xen/arch/x86/cpu/common.c |  5 +++--
>>>>>  xen/arch/x86/desc.c       |  2 ++
>>>>>  xen/arch/x86/smpboot.c    |  5 ++++-
>>>>>  xen/arch/x86/traps.c      | 10 +++++++---
>>>>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
>>>>> index 1b33f1ed71..7b093cb421 100644
>>>>> --- a/xen/arch/x86/cpu/common.c
>>>>> +++ b/xen/arch/x86/cpu/common.c
>>>>> @@ -752,8 +752,9 @@ void load_system_tables(void)
>>>>>  
>>>>>  	_set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
>>>>>  			 sizeof(*tss) - 1, SYS_DESC_tss_avail);
>>>>> -	_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>>>> -			 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>>>> +	if ( IS_ENABLED(CONFIG_PV32) )
>>>>> +		_set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss,
>>>>> +				 sizeof(*tss) - 1, SYS_DESC_tss_busy);
>>>> Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then.
>>> Doing it like this specifically ensures that there is never a case where
>>> things are half configured.
>> But this way you set up something in the GDT that's never going
>> to be used when "pv=no-32". Why leave a TSS accessible that we
>> don't need?
> 
> Defence in depth.
> 
> Having it only partially set up is more likely to fail in a security
> relevant way, than having it fully set up.

Well, I'm not convinced, but anyway
Acked-by: Jan Beulich <jbeulich@suse.com>

> This particular example is poor.  There is no need to have the TSS in
> either GDT after the `ltr` instruction at boot for 64bit, because we
> don't task switch, but ISTR you requesting that this stayed as-was for
> consistency.  (For 32bit Xen, it was strictly necessary for the #DF task
> switch to work.)

Well, I'm simply of the opinion that what the TR holds should point
to something valid in the currently active GDT. (As an alternative
we could decide to zap TSS descriptors from _both_ GDTs once we're
past the LTR. This wouldn't even conflict with resume from S3, as
we re-write the TSS descriptors immediately ahead of the LTR.)

> However, the other logic, particularly the cached l1e pointing at the
> percpu compat_gdt is more liable to go wrong in interesting ways.

Possibly, yes.

Jan

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

On 21/04/2020 07:02, Jan Beulich wrote:
> On 20.04.2020 20:05, Andrew Cooper wrote:
>> On 20/04/2020 15:05, Jan Beulich wrote:
>>> I'm in particular
>>> concerned that we may gain a large number of such printk()s over
>>> time, if we added them in such cases.
>> The printk() was a bit of an afterthought, but deliberately avoiding the
>> -EINVAL path was specifically not.
>>
>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>> they should see something other than
>>
>> (XEN) parameter "pv=no-32" unknown!
> Why - to this specific build of Xen the parameter is unknown.

Because it is unnecessarily problematic and borderline obnoxious to
users, as well as occasional Xen developers.

"you've not got the correct CONFIG_$X for that to be meaningful" is
specifically useful to separate from "I've got no idea".

>> I don't think overloading the return value is a clever move, because
>> then every parse function has to take care of ensuring that -EOPNOTSUPP
>> (or ENODEV?) never clobbers -EINVAL.
> I didn't suggest overloading the return value. Instead I
> specifically want this to go the -EINVAL path.
>
>> We could have a generic helper which looks like:
>>
>> static inline void ignored_param(const char *cfg, const char *name,
>> const char *s, const char *ss)
>> {
>>     printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>> cfg, name, s, (int)(ss - s));
>> }
>>
>> which at least would keep all the users consistent.
> Further bloating the binary with (almost) useless string literals.
> I'd specifically like to avoid this.

I don't accept that as a valid argument.

We're talking about literally tens of bytes (which will merge anyway, so
0 in practice), and a resulting UI which helps people get out of
problems rather than penalises them for having gotten into a problem to
begin with.

I will absolutely prioritise a more helpful UI over a handful of bytes.

~Andrew

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jürgen Groß]

On 23.04.20 19:35, Andrew Cooper wrote:
> On 21/04/2020 07:02, Jan Beulich wrote:
>> On 20.04.2020 20:05, Andrew Cooper wrote:
>>> On 20/04/2020 15:05, Jan Beulich wrote:
>>>> I'm in particular
>>>> concerned that we may gain a large number of such printk()s over
>>>> time, if we added them in such cases.
>>> The printk() was a bit of an afterthought, but deliberately avoiding the
>>> -EINVAL path was specifically not.
>>>
>>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>>> they should see something other than
>>>
>>> (XEN) parameter "pv=no-32" unknown!
>> Why - to this specific build of Xen the parameter is unknown.
> 
> Because it is unnecessarily problematic and borderline obnoxious to
> users, as well as occasional Xen developers.
> 
> "you've not got the correct CONFIG_$X for that to be meaningful" is
> specifically useful to separate from "I've got no idea".
> 
>>> I don't think overloading the return value is a clever move, because
>>> then every parse function has to take care of ensuring that -EOPNOTSUPP
>>> (or ENODEV?) never clobbers -EINVAL.
>> I didn't suggest overloading the return value. Instead I
>> specifically want this to go the -EINVAL path.
>>
>>> We could have a generic helper which looks like:
>>>
>>> static inline void ignored_param(const char *cfg, const char *name,
>>> const char *s, const char *ss)
>>> {
>>>      printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>>> cfg, name, s, (int)(ss - s));
>>> }
>>>
>>> which at least would keep all the users consistent.
>> Further bloating the binary with (almost) useless string literals.
>> I'd specifically like to avoid this.
> 
> I don't accept that as a valid argument.
> 
> We're talking about literally tens of bytes (which will merge anyway, so
> 0 in practice), and a resulting UI which helps people get out of
> problems rather than penalises them for having gotten into a problem to
> begin with.
> 
> I will absolutely prioritise a more helpful UI over a handful of bytes.

What about a kconfig option (defaulting to "yes") enabling this feature?

That way an embedded variant can be made smaller while a server one is
more user friendly.


Juergen

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jan Beulich]

On 23.04.2020 19:35, Andrew Cooper wrote:
> On 21/04/2020 07:02, Jan Beulich wrote:
>> On 20.04.2020 20:05, Andrew Cooper wrote:
>>> On 20/04/2020 15:05, Jan Beulich wrote:
>>>> I'm in particular
>>>> concerned that we may gain a large number of such printk()s over
>>>> time, if we added them in such cases.
>>> The printk() was a bit of an afterthought, but deliberately avoiding the
>>> -EINVAL path was specifically not.
>>>
>>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>>> they should see something other than
>>>
>>> (XEN) parameter "pv=no-32" unknown!
>> Why - to this specific build of Xen the parameter is unknown.
> 
> Because it is unnecessarily problematic and borderline obnoxious to
> users, as well as occasional Xen developers.
> 
> "you've not got the correct CONFIG_$X for that to be meaningful" is
> specifically useful to separate from "I've got no idea".
> 
>>> I don't think overloading the return value is a clever move, because
>>> then every parse function has to take care of ensuring that -EOPNOTSUPP
>>> (or ENODEV?) never clobbers -EINVAL.
>> I didn't suggest overloading the return value. Instead I
>> specifically want this to go the -EINVAL path.
>>
>>> We could have a generic helper which looks like:
>>>
>>> static inline void ignored_param(const char *cfg, const char *name,
>>> const char *s, const char *ss)
>>> {
>>>     printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>>> cfg, name, s, (int)(ss - s));
>>> }
>>>
>>> which at least would keep all the users consistent.
>> Further bloating the binary with (almost) useless string literals.
>> I'd specifically like to avoid this.
> 
> I don't accept that as a valid argument.
> 
> We're talking about literally tens of bytes (which will merge anyway, so
> 0 in practice), and a resulting UI which helps people get out of
> problems rather than penalises them for having gotten into a problem to
> begin with.

How will they merge? The different instances of the format string
above may/should, yes, but the different "CONFIG_xyz" literals the
callers pass won't, for example.

> I will absolutely prioritise a more helpful UI over a handful of bytes.

This "a handful of bytes doesn't matter" attitude has lead to
imo unacceptable growth of various software packages over the
years.

Anyway - I don't want to block this change over this argument,
so I'm willing to ack a patch with the helper introduced (and
preferably the "CONFIG_" part of the cfg arguments moved into
the helper's format string), as long as we plan to then make
consistent use of the helper everywhere. That said, I don't
immediately see what would be passed for "cfg" in some of
parse_iommu_param()'s cases.

Jan

Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

On 24/04/2020 06:28, Jürgen Groß wrote:
> On 23.04.20 19:35, Andrew Cooper wrote:
>> On 21/04/2020 07:02, Jan Beulich wrote:
>>> On 20.04.2020 20:05, Andrew Cooper wrote:
>>>> On 20/04/2020 15:05, Jan Beulich wrote:
>>>>> I'm in particular
>>>>> concerned that we may gain a large number of such printk()s over
>>>>> time, if we added them in such cases.
>>>> The printk() was a bit of an afterthought, but deliberately
>>>> avoiding the
>>>> -EINVAL path was specifically not.
>>>>
>>>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>>>> they should see something other than
>>>>
>>>> (XEN) parameter "pv=no-32" unknown!
>>> Why - to this specific build of Xen the parameter is unknown.
>>
>> Because it is unnecessarily problematic and borderline obnoxious to
>> users, as well as occasional Xen developers.
>>
>> "you've not got the correct CONFIG_$X for that to be meaningful" is
>> specifically useful to separate from "I've got no idea".
>>
>>>> I don't think overloading the return value is a clever move, because
>>>> then every parse function has to take care of ensuring that
>>>> -EOPNOTSUPP
>>>> (or ENODEV?) never clobbers -EINVAL.
>>> I didn't suggest overloading the return value. Instead I
>>> specifically want this to go the -EINVAL path.
>>>
>>>> We could have a generic helper which looks like:
>>>>
>>>> static inline void ignored_param(const char *cfg, const char *name,
>>>> const char *s, const char *ss)
>>>> {
>>>>      printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>>>> cfg, name, s, (int)(ss - s));
>>>> }
>>>>
>>>> which at least would keep all the users consistent.
>>> Further bloating the binary with (almost) useless string literals.
>>> I'd specifically like to avoid this.
>>
>> I don't accept that as a valid argument.
>>
>> We're talking about literally tens of bytes (which will merge anyway, so
>> 0 in practice), and a resulting UI which helps people get out of
>> problems rather than penalises them for having gotten into a problem to
>> begin with.
>>
>> I will absolutely prioritise a more helpful UI over a handful of bytes.
>
> What about a kconfig option (defaulting to "yes") enabling this feature?

IMO, that's literally not worth the bytes taken to implement.

> That way an embedded variant can be made smaller while a server one is
> more user friendly.

There is far lower hanging fruit for an embedded usecase, and its not at
all a foregone conclusion that such a usecase would want the less user
friendly version.  (Embedded very definitely doesn't mean that it won't
have users interacting with the command line).

I would certainly recommend against attempting to speculatively fix
something which isn't a problem, based on guesswork about what a
hypothetical group of people might want while totally ignoring the far
larger savings to be gained by e.g. making CONFIG_INTEL/AMD work.

~Andrew

[PATCH v2 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Andrew Cooper]

This is the start of some performance and security-hardening improvements,
based on the fact that 32bit PV guests are few and far between these days.

Ring1 is full of architectural corner cases, such as counting as supervisor
from a paging point of view.  This accounts for a substantial performance hit
on processors from the last 8 years (adjusting SMEP/SMAP on every privilege
transition), and the gap is only going to get bigger with new hardware
features.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wl@xen.org>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Fix typos, __init
 * Introduce no_config_param() wrapper
---
 docs/misc/xen-command-line.pandoc | 12 +++++++++++-
 xen/arch/x86/Kconfig              | 16 ++++++++++++++++
 xen/arch/x86/pv/domain.c          | 34 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/setup.c              |  9 +++++++--
 xen/include/asm-x86/pv/domain.h   |  6 ++++++
 xen/include/xen/param.h           |  9 +++++++++
 6 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index acd0b3d994..ee12b0f53f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1694,7 +1694,17 @@ The following resources are available:
     CDP, one COS will corespond two CBMs other than one with CAT, due to the
     sum of CBMs is fixed, that means actual `cos_max` in use will automatically
     reduce to half when CDP is enabled.
-	
+
+### pv
+    = List of [ 32=<bool> ]
+
+    Applicability: x86
+
+Controls for aspects of PV guest support.
+
+*   The `32` boolean controls whether 32bit PV guests can be created.  It
+    defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
+
 ### pv-linear-pt (x86)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index a69be983d6..96432f1f69 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -49,6 +49,22 @@ config PV
 
 	  If unsure, say Y.
 
+config PV32
+	bool "Support for 32bit PV guests"
+	depends on PV
+	default y
+	---help---
+	  The 32bit PV ABI uses Ring1, an area of the x86 architecture which
+	  was deprecated and mostly removed in the AMD64 spec.  As a result,
+	  it occasionally conflicts with newer x86 hardware features, causing
+	  overheads for Xen to maintain backwards compatibility.
+
+	  People may wish to disable 32bit PV guests for attack surface
+	  reduction, or performance reasons.  Backwards compatibility can be
+	  provided via the PV Shim mechanism.
+
+	  If unsure, say Y.
+
 config PV_LINEAR_PT
        bool "Support for PV linear pagetables"
        depends on PV
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 43da5c179f..3579dc063e 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -16,6 +16,38 @@
 #include <asm/pv/domain.h>
 #include <asm/shadow.h>
 
+#ifdef CONFIG_PV32
+int8_t __read_mostly opt_pv32 = -1;
+#endif
+
+static __init int parse_pv(const char *s)
+{
+    const char *ss;
+    int val, rc = 0;
+
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( (val = parse_boolean("32", s, ss)) >= 0 )
+        {
+#ifdef CONFIG_PV32
+            opt_pv32 = val;
+#else
+            no_config_param("PV32", "pv", s, ss);
+#endif
+        }
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
+
+    return rc;
+}
+custom_param("pv", parse_pv);
+
 static __read_mostly enum {
     PCID_OFF,
     PCID_ALL,
@@ -174,6 +206,8 @@ int switch_compat(struct domain *d)
 
     BUILD_BUG_ON(offsetof(struct shared_info, vcpu_info) != 0);
 
+    if ( !opt_pv32 )
+        return -EOPNOTSUPP;
     if ( is_hvm_domain(d) || domain_tot_pages(d) != 0 )
         return -EACCES;
     if ( is_pv_32bit_domain(d) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index eb56d78c2f..9e9576344c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -53,6 +53,7 @@
 #include <asm/spec_ctrl.h>
 #include <asm/guest.h>
 #include <asm/microcode.h>
+#include <asm/pv/domain.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
@@ -1870,8 +1871,12 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
     {
         snprintf(s, sizeof(s), "xen-%d.%d-x86_64 ", major, minor);
         safe_strcat(*info, s);
-        snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
-        safe_strcat(*info, s);
+
+        if ( opt_pv32 )
+        {
+            snprintf(s, sizeof(s), "xen-%d.%d-x86_32p ", major, minor);
+            safe_strcat(*info, s);
+        }
     }
     if ( hvm_enabled )
     {
diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
index 7a69bfb303..df9716ff26 100644
--- a/xen/include/asm-x86/pv/domain.h
+++ b/xen/include/asm-x86/pv/domain.h
@@ -23,6 +23,12 @@
 
 #include <xen/sched.h>
 
+#ifdef CONFIG_PV32
+extern int8_t opt_pv32;
+#else
+# define opt_pv32 false
+#endif
+
 /*
  * PCID values for the address spaces of 64-bit pv domains:
  *
diff --git a/xen/include/xen/param.h b/xen/include/xen/param.h
index d4578cd27f..a1dc3ba8f0 100644
--- a/xen/include/xen/param.h
+++ b/xen/include/xen/param.h
@@ -127,4 +127,13 @@ extern const struct kernel_param __param_start[], __param_end[];
     string_param(_name, _var); \
     string_runtime_only_param(_name, _var)
 
+static inline void no_config_param(const char *cfg, const char *param,
+                                   const char *s, const char *e)
+{
+    int len = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
+
+    printk(XENLOG_INFO "CONFIG_%s disabled - ignoring '%s=%*s' setting\n",
+           cfg, param, len, s);
+}
+
 #endif /* _XEN_PARAM_H */
-- 
2.11.0

Re: [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds

[Andrew Cooper]

On 20/04/2020 15:09, Jan Beulich wrote:
> On 17.04.2020 17:50, Andrew Cooper wrote:
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>          return 0;
>>  
>>      d->arch.has_32bit_shinfo = 1;
>> -    d->arch.is_32bit_pv = 1;
>> +    d->arch.pv.is_32bit = 1;
>>  
>>      for_each_vcpu( d, v )
>>      {
>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>      return 0;
>>  
>>   undo_and_fail:
>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>      for_each_vcpu( d, v )
>>      {
>>          free_compat_arg_xlat(v);
>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>      d->arch.ctxt_switch = &pv_csw;
>>  
>>      /* 64-bit PV guest by default. */
>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
> Switch to true/false while you're touching these?

Yes, but I'm tempted to delete these lines in the final hunk.  Its
writing zeros into a zeroed structures.

~Andrew

Re: [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds

[Jan Beulich]

On 29.04.2020 15:13, Andrew Cooper wrote:
> On 20/04/2020 15:09, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/pv/domain.c
>>> +++ b/xen/arch/x86/pv/domain.c
>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>          return 0;
>>>  
>>>      d->arch.has_32bit_shinfo = 1;
>>> -    d->arch.is_32bit_pv = 1;
>>> +    d->arch.pv.is_32bit = 1;
>>>  
>>>      for_each_vcpu( d, v )
>>>      {
>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>      return 0;
>>>  
>>>   undo_and_fail:
>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>      for_each_vcpu( d, v )
>>>      {
>>>          free_compat_arg_xlat(v);
>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>      d->arch.ctxt_switch = &pv_csw;
>>>  
>>>      /* 64-bit PV guest by default. */
>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>> Switch to true/false while you're touching these?
> 
> Yes, but I'm tempted to delete these lines in the final hunk.  Its
> writing zeros into a zeroed structures.

Oh, yes, agreed.

Jan

Re: [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds

[Andrew Cooper]

On 29/04/2020 14:29, Jan Beulich wrote:
> On 29.04.2020 15:13, Andrew Cooper wrote:
>> On 20/04/2020 15:09, Jan Beulich wrote:
>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/pv/domain.c
>>>> +++ b/xen/arch/x86/pv/domain.c
>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>>          return 0;
>>>>  
>>>>      d->arch.has_32bit_shinfo = 1;
>>>> -    d->arch.is_32bit_pv = 1;
>>>> +    d->arch.pv.is_32bit = 1;
>>>>  
>>>>      for_each_vcpu( d, v )
>>>>      {
>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>>      return 0;
>>>>  
>>>>   undo_and_fail:
>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>>      for_each_vcpu( d, v )
>>>>      {
>>>>          free_compat_arg_xlat(v);
>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>>      d->arch.ctxt_switch = &pv_csw;
>>>>  
>>>>      /* 64-bit PV guest by default. */
>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>> Switch to true/false while you're touching these?
>> Yes, but I'm tempted to delete these lines in the final hunk.  Its
>> writing zeros into a zeroed structures.
> Oh, yes, agreed.

Can I take this as an ack then?

~Andrew

Re: [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() in !CONFIG_PV32 builds

[Jan Beulich]

On 29.04.2020 15:30, Andrew Cooper wrote:
> On 29/04/2020 14:29, Jan Beulich wrote:
>> On 29.04.2020 15:13, Andrew Cooper wrote:
>>> On 20/04/2020 15:09, Jan Beulich wrote:
>>>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/pv/domain.c
>>>>> +++ b/xen/arch/x86/pv/domain.c
>>>>> @@ -215,7 +215,7 @@ int switch_compat(struct domain *d)
>>>>>          return 0;
>>>>>  
>>>>>      d->arch.has_32bit_shinfo = 1;
>>>>> -    d->arch.is_32bit_pv = 1;
>>>>> +    d->arch.pv.is_32bit = 1;
>>>>>  
>>>>>      for_each_vcpu( d, v )
>>>>>      {
>>>>> @@ -235,7 +235,7 @@ int switch_compat(struct domain *d)
>>>>>      return 0;
>>>>>  
>>>>>   undo_and_fail:
>>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>>>      for_each_vcpu( d, v )
>>>>>      {
>>>>>          free_compat_arg_xlat(v);
>>>>> @@ -358,7 +358,7 @@ int pv_domain_initialise(struct domain *d)
>>>>>      d->arch.ctxt_switch = &pv_csw;
>>>>>  
>>>>>      /* 64-bit PV guest by default. */
>>>>> -    d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>>>> +    d->arch.pv.is_32bit = d->arch.has_32bit_shinfo = 0;
>>>> Switch to true/false while you're touching these?
>>> Yes, but I'm tempted to delete these lines in the final hunk.  Its
>>> writing zeros into a zeroed structures.
>> Oh, yes, agreed.
> 
> Can I take this as an ack then?

Sorry, didn't realize I didn't give one yet with the adjustments
made:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 1/3] x86/pv: Options to disable and/or compile out 32bit PV support

[Jan Beulich]

On 29.04.2020 15:06, Andrew Cooper wrote:
> This is the start of some performance and security-hardening improvements,
> based on the fact that 32bit PV guests are few and far between these days.
> 
> Ring1 is full of architectural corner cases, such as counting as supervisor
> from a paging point of view.  This accounts for a substantial performance hit
> on processors from the last 8 years (adjusting SMEP/SMAP on every privilege
> transition), and the gap is only going to get bigger with new hardware
> features.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Wei Liu <wl@xen.org>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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