[PATCH 0/5] x86: Misc improvements from KAISER-prep work

[PATCH 0/5] x86: Misc improvements from KAISER-prep work

[Andrew Cooper]

This is a misc set of improvements I made when doing the KAISER-prep work, but
stand on their own merit irrespective of that series.

Andrew Cooper (5):
  x86/idt: Factor out enabling and disabling of ISTs
  x86/pv: Rename invalidate_shadow_ldt() to pv_destroy_ldt()
  x86/pv: Break handle_ldt_mapping_fault() out of handle_gdt_ldt_mapping_fault()
  x86/pv: Drop support for paging out the LDT
  x86/monitor: Capture Xen's intent to use monitor at boot time

 xen/arch/x86/acpi/lib.c             | 16 +------
 xen/arch/x86/cpu/common.c           | 11 +++--
 xen/arch/x86/domain.c               |  7 +---
 xen/arch/x86/hvm/svm/svm.c          |  8 +---
 xen/arch/x86/mm.c                   | 66 ++---------------------------
 xen/arch/x86/pv/descriptor-tables.c | 34 ++++++++++++++-
 xen/arch/x86/pv/domain.c            |  2 -
 xen/arch/x86/pv/mm.c                |  3 --
 xen/arch/x86/smpboot.c              |  4 +-
 xen/arch/x86/traps.c                | 83 +++++++++++++++++++++----------------
 xen/include/asm-x86/cpufeature.h    |  5 +--
 xen/include/asm-x86/cpufeatures.h   |  1 +
 xen/include/asm-x86/domain.h        |  4 --
 xen/include/asm-x86/mwait.h         |  3 ++
 xen/include/asm-x86/processor.h     | 14 +++++++
 xen/include/asm-x86/pv/mm.h         |  3 ++
 16 files changed, 119 insertions(+), 145 deletions(-)

-- 
2.1.4


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

[PATCH 1/5] x86/idt: Factor out enabling and disabling of ISTs

[Andrew Cooper]

All alteration of IST settings (other than the crash path) happen in an
identical triple.  Introduce helpers to keep the triple in sync, and reduce
the risk of opencoded mistakes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpu/common.c       |  4 +---
 xen/arch/x86/hvm/svm/svm.c      |  8 ++------
 xen/arch/x86/smpboot.c          |  4 +---
 xen/arch/x86/traps.c            |  4 +---
 xen/include/asm-x86/processor.h | 14 ++++++++++++++
 5 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 06e0eab..0a5c6ad 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -703,9 +703,7 @@ void load_system_tables(void)
 	ltr(TSS_ENTRY << 3);
 	lldt(0);
 
-	set_ist(&idt_tables[cpu][TRAP_double_fault],  IST_DF);
-	set_ist(&idt_tables[cpu][TRAP_nmi],	      IST_NMI);
-	set_ist(&idt_tables[cpu][TRAP_machine_check], IST_MCE);
+	enable_each_ist(idt_tables[cpu]);
 
 	/*
 	 * Bottom-of-stack must be 16-byte aligned!
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfa..9243852 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1038,9 +1038,7 @@ static void svm_ctxt_switch_from(struct vcpu *v)
     svm_vmload_pa(per_cpu(host_vmcb, cpu));
 
     /* Resume use of ISTs now that the host TR is reinstated. */
-    set_ist(&idt_tables[cpu][TRAP_double_fault],  IST_DF);
-    set_ist(&idt_tables[cpu][TRAP_nmi],           IST_NMI);
-    set_ist(&idt_tables[cpu][TRAP_machine_check], IST_MCE);
+    enable_each_ist(idt_tables[cpu]);
 }
 
 static void svm_ctxt_switch_to(struct vcpu *v)
@@ -1059,9 +1057,7 @@ static void svm_ctxt_switch_to(struct vcpu *v)
      * Cannot use ISTs for NMI/#MC/#DF while we are running with the guest TR.
      * But this doesn't matter: the IST is only req'd to handle SYSCALL/SYSRET.
      */
-    set_ist(&idt_tables[cpu][TRAP_double_fault],  IST_NONE);
-    set_ist(&idt_tables[cpu][TRAP_nmi],           IST_NONE);
-    set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
+    disable_each_ist(idt_tables[cpu]);
 
     svm_restore_dr(v);
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7b97ff8..e7fa159 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -723,9 +723,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     if ( idt_tables[cpu] == NULL )
         goto out;
     memcpy(idt_tables[cpu], idt_table, IDT_ENTRIES * sizeof(idt_entry_t));
-    set_ist(&idt_tables[cpu][TRAP_double_fault],  IST_NONE);
-    set_ist(&idt_tables[cpu][TRAP_nmi],           IST_NONE);
-    set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
+    disable_each_ist(idt_tables[cpu]);
 
     for ( stub_page = 0, i = cpu & ~(STUBS_PER_PAGE - 1);
           i < nr_cpu_ids && i <= (cpu | (STUBS_PER_PAGE - 1)); ++i )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2509ea7..b5849c3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1884,9 +1884,7 @@ void __init init_idt_traps(void)
     set_intr_gate(TRAP_simd_error,&simd_coprocessor_error);
 
     /* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
-    set_ist(&idt_table[TRAP_double_fault],  IST_DF);
-    set_ist(&idt_table[TRAP_nmi],           IST_NMI);
-    set_ist(&idt_table[TRAP_machine_check], IST_MCE);
+    enable_each_ist(idt_table);
 
     /* CPU0 uses the master IDT. */
     idt_tables[0] = idt_table;
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 41a8d8c..a0c524b 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -457,6 +457,20 @@ static always_inline void set_ist(idt_entry_t *idt, unsigned long ist)
     _write_gate_lower(idt, &new);
 }
 
+static inline void enable_each_ist(idt_entry_t *idt)
+{
+    set_ist(&idt[TRAP_double_fault],  IST_DF);
+    set_ist(&idt[TRAP_nmi],           IST_NMI);
+    set_ist(&idt[TRAP_machine_check], IST_MCE);
+}
+
+static inline void disable_each_ist(idt_entry_t *idt)
+{
+    set_ist(&idt[TRAP_double_fault],  IST_NONE);
+    set_ist(&idt[TRAP_nmi],           IST_NONE);
+    set_ist(&idt[TRAP_machine_check], IST_NONE);
+}
+
 #define IDT_ENTRIES 256
 extern idt_entry_t idt_table[];
 extern idt_entry_t *idt_tables[];
-- 
2.1.4


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

[PATCH 2/5] x86/pv: Rename invalidate_shadow_ldt() to pv_destroy_ldt()

[Andrew Cooper]

and move it into pv/descriptor-tables.c beside its GDT counterpart.  Reduce
the !in_irq() check from a BUG_ON() to ASSERT().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/mm.c                   | 51 ++++---------------------------------
 xen/arch/x86/pv/descriptor-tables.c | 42 ++++++++++++++++++++++++++++--
 xen/include/asm-x86/pv/mm.h         |  3 +++
 3 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index a56f875..14cfa93 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -125,6 +125,7 @@
 
 #include <asm/hvm/grant_table.h>
 #include <asm/pv/grant_table.h>
+#include <asm/pv/mm.h>
 
 #include "pv/mm.h"
 
@@ -544,48 +545,6 @@ static inline void set_tlbflush_timestamp(struct page_info *page)
 const char __section(".bss.page_aligned.const") __aligned(PAGE_SIZE)
     zero_page[PAGE_SIZE];
 
-/*
- * Flush the LDT, dropping any typerefs.  Returns a boolean indicating whether
- * mappings have been removed (i.e. a TLB flush is needed).
- */
-static bool invalidate_shadow_ldt(struct vcpu *v)
-{
-    l1_pgentry_t *pl1e;
-    unsigned int i, mappings_dropped = 0;
-    struct page_info *page;
-
-    BUG_ON(unlikely(in_irq()));
-
-    spin_lock(&v->arch.pv_vcpu.shadow_ldt_lock);
-
-    if ( v->arch.pv_vcpu.shadow_ldt_mapcnt == 0 )
-        goto out;
-
-    pl1e = pv_ldt_ptes(v);
-
-    for ( i = 0; i < 16; i++ )
-    {
-        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
-            continue;
-
-        page = l1e_get_page(pl1e[i]);
-        l1e_write(&pl1e[i], l1e_empty());
-        mappings_dropped++;
-
-        ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
-        ASSERT_PAGE_IS_DOMAIN(page, v->domain);
-        put_page_and_type(page);
-    }
-
-    ASSERT(v->arch.pv_vcpu.shadow_ldt_mapcnt == mappings_dropped);
-    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
-
- out:
-    spin_unlock(&v->arch.pv_vcpu.shadow_ldt_lock);
-
-    return mappings_dropped;
-}
-
 
 static int alloc_segdesc_page(struct page_info *page)
 {
@@ -1242,7 +1201,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
         {
             for_each_vcpu ( pg_owner, v )
             {
-                if ( invalidate_shadow_ldt(v) )
+                if ( pv_destroy_ldt(v) )
                     flush_tlb_mask(v->vcpu_dirty_cpumask);
             }
         }
@@ -2825,7 +2784,7 @@ int new_guest_cr3(mfn_t mfn)
             return rc;
         }
 
-        invalidate_shadow_ldt(curr); /* Unconditional TLB flush later. */
+        pv_destroy_ldt(curr); /* Unconditional TLB flush later. */
         write_ptbase(curr);
 
         return 0;
@@ -2861,7 +2820,7 @@ int new_guest_cr3(mfn_t mfn)
         return rc;
     }
 
-    invalidate_shadow_ldt(curr); /* Unconditional TLB flush later. */
+    pv_destroy_ldt(curr); /* Unconditional TLB flush later. */
 
     if ( !VM_ASSIST(d, m2p_strict) && !paging_mode_refcounts(d) )
         fill_ro_mpt(mfn);
@@ -3368,7 +3327,7 @@ long do_mmuext_op(
             else if ( (curr->arch.pv_vcpu.ldt_ents != ents) ||
                       (curr->arch.pv_vcpu.ldt_base != ptr) )
             {
-                if ( invalidate_shadow_ldt(curr) )
+                if ( pv_destroy_ldt(curr) )
                     flush_tlb_local();
 
                 curr->arch.pv_vcpu.ldt_base = ptr;
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index d1c4296..b418bbb 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -31,9 +31,47 @@
 #undef page_to_mfn
 #define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
 
-/*******************
- * Descriptor Tables
+/*
+ * Flush the LDT, dropping any typerefs.  Returns a boolean indicating whether
+ * mappings have been removed (i.e. a TLB flush is needed).
  */
+bool pv_destroy_ldt(struct vcpu *v)
+{
+    l1_pgentry_t *pl1e;
+    unsigned int i, mappings_dropped = 0;
+    struct page_info *page;
+
+    ASSERT(!in_irq());
+
+    spin_lock(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+    if ( v->arch.pv_vcpu.shadow_ldt_mapcnt == 0 )
+        goto out;
+
+    pl1e = pv_ldt_ptes(v);
+
+    for ( i = 0; i < 16; i++ )
+    {
+        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
+            continue;
+
+        page = l1e_get_page(pl1e[i]);
+        l1e_write(&pl1e[i], l1e_empty());
+        mappings_dropped++;
+
+        ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
+        ASSERT_PAGE_IS_DOMAIN(page, v->domain);
+        put_page_and_type(page);
+    }
+
+    ASSERT(v->arch.pv_vcpu.shadow_ldt_mapcnt == mappings_dropped);
+    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
+
+ out:
+    spin_unlock(&v->arch.pv_vcpu.shadow_ldt_lock);
+
+    return mappings_dropped;
+}
 
 void pv_destroy_gdt(struct vcpu *v)
 {
diff --git a/xen/include/asm-x86/pv/mm.h b/xen/include/asm-x86/pv/mm.h
index 5d2fe4c..246b990 100644
--- a/xen/include/asm-x86/pv/mm.h
+++ b/xen/include/asm-x86/pv/mm.h
@@ -29,6 +29,7 @@ long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries);
 void pv_destroy_gdt(struct vcpu *v);
 
 bool pv_map_ldt_shadow_page(unsigned int off);
+bool pv_destroy_ldt(struct vcpu *v);
 
 #else
 
@@ -48,6 +49,8 @@ static inline long pv_set_gdt(struct vcpu *v, unsigned long *frames,
 static inline void pv_destroy_gdt(struct vcpu *v) { ASSERT_UNREACHABLE(); }
 
 static inline bool pv_map_ldt_shadow_page(unsigned int off) { return false; }
+static inline bool pv_destroy_ldt(struct vcpu *v)
+{ ASSERT_UNREACHABLE(); return false; }
 
 #endif
 
-- 
2.1.4


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

[PATCH 3/5] x86/pv: Break handle_ldt_mapping_fault() out of handle_gdt_ldt_mapping_fault()

[Andrew Cooper]

Adjust handle_ldt_mapping_fault() exclude the use of this fixup path for
non-PV guests.  Well-formed code shouldn't reference the LDT while in HVM vcpu
context, but currently on a context switch from PV to HVM context, there may
be a stale LDT selector loaded, over an unmapped region.

By explicitly excluding HVM context at this point, we avoid erroneous
hypervisor execution resulting in a cascade failure, by falling into
pv_map_ldt_shadow_page().

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

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b5849c3..3d6a4d5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1093,6 +1093,48 @@ static void reserved_bit_page_fault(unsigned long addr,
     show_execution_state(regs);
 }
 
+static int handle_ldt_mapping_fault(unsigned int offset,
+                                    struct cpu_user_regs *regs)
+{
+    struct vcpu *curr = current;
+
+    /*
+     * Not in PV context?  Something is very broken.  Leave it to the #PF
+     * handler, which will probably result in a panic().
+     */
+    if ( !is_pv_vcpu(curr) )
+        return 0;
+
+    /* Try to copy a mapping from the guest's LDT, if it is valid. */
+    if ( likely(pv_map_ldt_shadow_page(offset)) )
+    {
+        if ( guest_mode(regs) )
+            trace_trap_two_addr(TRC_PV_GDT_LDT_MAPPING_FAULT,
+                                regs->rip, offset);
+    }
+    else
+    {
+        /* In hypervisor mode? Leave it to the #PF handler to fix up. */
+        if ( !guest_mode(regs) )
+            return 0;
+
+        /* Access would have become non-canonical? Pass #GP[sel] back. */
+        if ( unlikely(!is_canonical_address(
+                          curr->arch.pv_vcpu.ldt_base + offset)) )
+        {
+            uint16_t ec = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) | X86_XEC_TI;
+
+            pv_inject_hw_exception(TRAP_gp_fault, ec);
+        }
+        else
+            /* else pass the #PF back, with adjusted %cr2. */
+            pv_inject_page_fault(regs->error_code,
+                                 curr->arch.pv_vcpu.ldt_base + offset);
+    }
+
+    return EXCRET_fault_fixed;
+}
+
 static int handle_gdt_ldt_mapping_fault(unsigned long offset,
                                         struct cpu_user_regs *regs)
 {
@@ -1114,40 +1156,11 @@ static int handle_gdt_ldt_mapping_fault(unsigned long offset,
     offset &= (1UL << (GDT_LDT_VCPU_VA_SHIFT-1)) - 1UL;
 
     if ( likely(is_ldt_area) )
-    {
-        /* LDT fault: Copy a mapping from the guest's LDT, if it is valid. */
-        if ( likely(pv_map_ldt_shadow_page(offset)) )
-        {
-            if ( guest_mode(regs) )
-                trace_trap_two_addr(TRC_PV_GDT_LDT_MAPPING_FAULT,
-                                    regs->rip, offset);
-        }
-        else
-        {
-            /* In hypervisor mode? Leave it to the #PF handler to fix up. */
-            if ( !guest_mode(regs) )
-                return 0;
+        return handle_ldt_mapping_fault(offset, regs);
 
-            /* Access would have become non-canonical? Pass #GP[sel] back. */
-            if ( unlikely(!is_canonical_address(
-                              curr->arch.pv_vcpu.ldt_base + offset)) )
-            {
-                uint16_t ec = (offset & ~(X86_XEC_EXT | X86_XEC_IDT)) | X86_XEC_TI;
-
-                pv_inject_hw_exception(TRAP_gp_fault, ec);
-            }
-            else
-                /* else pass the #PF back, with adjusted %cr2. */
-                pv_inject_page_fault(regs->error_code,
-                                     curr->arch.pv_vcpu.ldt_base + offset);
-        }
-    }
-    else
-    {
-        /* GDT fault: handle the fault as #GP(selector). */
-        regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
-        (void)do_general_protection(regs);
-    }
+    /* GDT fault: handle the fault as #GP(selector). */
+    regs->error_code = offset & ~(X86_XEC_EXT | X86_XEC_IDT | X86_XEC_TI);
+    do_general_protection(regs);
 
     return EXCRET_fault_fixed;
 }
-- 
2.1.4


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

[PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Andrew Cooper]

Windows is the only OS which pages out kernel datastructures, so chances are
good that this is a vestigial remnant of the PV Windows XP experiment.
Furthermore the implementation is incomplete; it only functions for a present
=> not-present transition, rather than a present => read/write transition.

The for_each_vcpu() is one scalability limitation for PV guests, which can't
reasonably be altered to be continuable.

One side effects of dropping paging out support is that now, the LDT (like the
GDT) is only ever modified in current context, allowing us to drop
shadow_ldt_mapcnt and shadow_ldt_lock from struct vcpu.

Another side effect is that the LDT no longer automatically cleans itself up
on domain destruction.  Cover this by explicitly releasing the LDT frames at
the same time as the GDT frames.

Finally, leave some asserts around to confirm the expected behaviour of all
the functions playing with PGT_seg_desc_page references.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domain.c               |  7 ++-----
 xen/arch/x86/mm.c                   | 17 -----------------
 xen/arch/x86/pv/descriptor-tables.c | 20 ++++++--------------
 xen/arch/x86/pv/domain.c            |  2 --
 xen/arch/x86/pv/mm.c                |  3 ---
 xen/include/asm-x86/domain.h        |  4 ----
 6 files changed, 8 insertions(+), 45 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index da1bf1a..2b7bc5b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
         {
             for_each_vcpu ( d, v )
             {
-                /*
-                 * Relinquish GDT mappings. No need for explicit unmapping of
-                 * the LDT as it automatically gets squashed with the guest
-                 * mappings.
-                 */
+                /* Relinquish GDT/LDT mappings. */
+                pv_destroy_ldt(v);
                 pv_destroy_gdt(v);
             }
         }
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 14cfa93..15a9334 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1152,7 +1152,6 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
     unsigned long     pfn = l1e_get_pfn(l1e);
     struct page_info *page;
     struct domain    *pg_owner;
-    struct vcpu      *v;
 
     if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || is_iomem_page(_mfn(pfn)) )
         return;
@@ -1188,25 +1187,9 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
      */
     if ( (l1e_get_flags(l1e) & _PAGE_RW) &&
          ((l1e_owner == pg_owner) || !paging_mode_external(pg_owner)) )
-    {
         put_page_and_type(page);
-    }
     else
-    {
-        /* We expect this is rare so we blow the entire shadow LDT. */
-        if ( unlikely(((page->u.inuse.type_info & PGT_type_mask) ==
-                       PGT_seg_desc_page)) &&
-             unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
-             (l1e_owner == pg_owner) )
-        {
-            for_each_vcpu ( pg_owner, v )
-            {
-                if ( pv_destroy_ldt(v) )
-                    flush_tlb_mask(v->vcpu_dirty_cpumask);
-            }
-        }
         put_page(page);
-    }
 }
 
 
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index b418bbb..77f9851 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -37,18 +37,12 @@
  */
 bool pv_destroy_ldt(struct vcpu *v)
 {
-    l1_pgentry_t *pl1e;
+    l1_pgentry_t *pl1e = pv_ldt_ptes(v);
     unsigned int i, mappings_dropped = 0;
     struct page_info *page;
 
     ASSERT(!in_irq());
-
-    spin_lock(&v->arch.pv_vcpu.shadow_ldt_lock);
-
-    if ( v->arch.pv_vcpu.shadow_ldt_mapcnt == 0 )
-        goto out;
-
-    pl1e = pv_ldt_ptes(v);
+    ASSERT(v == current || cpumask_empty(v->vcpu_dirty_cpumask));
 
     for ( i = 0; i < 16; i++ )
     {
@@ -64,12 +58,6 @@ bool pv_destroy_ldt(struct vcpu *v)
         put_page_and_type(page);
     }
 
-    ASSERT(v->arch.pv_vcpu.shadow_ldt_mapcnt == mappings_dropped);
-    v->arch.pv_vcpu.shadow_ldt_mapcnt = 0;
-
- out:
-    spin_unlock(&v->arch.pv_vcpu.shadow_ldt_lock);
-
     return mappings_dropped;
 }
 
@@ -80,6 +68,8 @@ void pv_destroy_gdt(struct vcpu *v)
     l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
     unsigned int i;
 
+    ASSERT(v == current || cpumask_empty(v->vcpu_dirty_cpumask));
+
     v->arch.pv_vcpu.gdt_ents = 0;
     for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
     {
@@ -100,6 +90,8 @@ long pv_set_gdt(struct vcpu *v, unsigned long *frames, unsigned int entries)
     l1_pgentry_t *pl1e;
     unsigned int i, nr_frames = DIV_ROUND_UP(entries, 512);
 
+    ASSERT(v == current || cpumask_empty(v->vcpu_dirty_cpumask));
+
     if ( entries > FIRST_RESERVED_GDT_ENTRY )
         return -EINVAL;
 
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 74e9e66..fbf8941 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -128,8 +128,6 @@ int pv_vcpu_initialise(struct vcpu *v)
 
     ASSERT(!is_idle_domain(d));
 
-    spin_lock_init(&v->arch.pv_vcpu.shadow_ldt_lock);
-
     rc = pv_create_gdt_ldt_l1tab(v);
     if ( rc )
         return rc;
diff --git a/xen/arch/x86/pv/mm.c b/xen/arch/x86/pv/mm.c
index 8d7a4fd..d293724 100644
--- a/xen/arch/x86/pv/mm.c
+++ b/xen/arch/x86/pv/mm.c
@@ -125,10 +125,7 @@ bool pv_map_ldt_shadow_page(unsigned int offset)
     pl1e = &pv_ldt_ptes(curr)[offset >> PAGE_SHIFT];
     l1e_add_flags(gl1e, _PAGE_RW);
 
-    spin_lock(&curr->arch.pv_vcpu.shadow_ldt_lock);
     l1e_write(pl1e, gl1e);
-    curr->arch.pv_vcpu.shadow_ldt_mapcnt++;
-    spin_unlock(&curr->arch.pv_vcpu.shadow_ldt_lock);
 
     return true;
 }
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4679d54..758e030 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -491,10 +491,6 @@ struct pv_vcpu
     unsigned int iopl;        /* Current IOPL for this VCPU, shifted left by
                                * 12 to match the eflags register. */
 
-    /* Current LDT details. */
-    unsigned long shadow_ldt_mapcnt;
-    spinlock_t shadow_ldt_lock;
-
     /* data breakpoint extension MSRs */
     uint32_t dr_mask[4];
 
-- 
2.1.4


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

[PATCH 5/5] x86/monitor: Capture Xen's intent to use monitor at boot time

[Andrew Cooper]

The ACPI idle driver uses an IPI to retrieve cpuid_ecx(5).  This is wasteful.

Introduce X86_FEATURE_XEN_MONITOR as a synthetic feature bit meaning MONITOR
&& EXTENSIONS && INTERRUPT_BREAK, and calculate it when a cpu comes up rather
than repeatedly at runtime.

Drop the duplicate defines for MWAIT cpuid information, and use the
definitions from mwait.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/acpi/lib.c           | 16 +---------------
 xen/arch/x86/cpu/common.c         |  7 +++++++
 xen/include/asm-x86/cpufeature.h  |  5 +----
 xen/include/asm-x86/cpufeatures.h |  1 +
 xen/include/asm-x86/mwait.h       |  3 +++
 5 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index 7d7c718..1d64e74 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -80,16 +80,10 @@ unsigned int acpi_get_processor_id(unsigned int cpu)
 	return INVALID_ACPIID;
 }
 
-static void get_mwait_ecx(void *info)
-{
-	*(u32 *)info = cpuid_ecx(CPUID_MWAIT_LEAF);
-}
-
 int arch_acpi_set_pdc_bits(u32 acpi_id, u32 *pdc, u32 mask)
 {
 	unsigned int cpu = get_cpu_id(acpi_id);
 	struct cpuinfo_x86 *c;
-	u32 ecx;
 
 	if (!(acpi_id + 1))
 		c = &boot_cpu_data;
@@ -110,15 +104,7 @@ int arch_acpi_set_pdc_bits(u32 acpi_id, u32 *pdc, u32 mask)
 	 * If mwait/monitor or its break-on-interrupt extension are
 	 * unsupported, Cx_FFH will be disabled.
 	 */
-	if (!cpu_has(c, X86_FEATURE_MONITOR) ||
-	    c->cpuid_level < CPUID_MWAIT_LEAF)
-		ecx = 0;
-	else if (c == &boot_cpu_data || cpu == smp_processor_id())
-		ecx = cpuid_ecx(CPUID_MWAIT_LEAF);
-	else
-		on_selected_cpus(cpumask_of(cpu), get_mwait_ecx, &ecx, 1);
-	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED) ||
-	    !(ecx & CPUID5_ECX_INTERRUPT_BREAK))
+	if ( !cpu_has_xen_monitor )
 		pdc[2] &= ~(ACPI_PDC_C_C1_FFH | ACPI_PDC_C_C2C3_FFH);
 
 	return 0;
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 0a5c6ad..ffc5230 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -12,6 +12,7 @@
 #include <mach_apic.h>
 #include <asm/setup.h>
 #include <public/sysctl.h> /* for XEN_INVALID_{SOCKET,CORE}_ID */
+#include <asm/mwait.h>
 
 #include "cpu.h"
 
@@ -312,6 +313,12 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	if ( cpu_has(c, X86_FEATURE_CLFLUSH) )
 		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
 
+	/* Xen only uses MONITOR if INTERRUPT_BREAK is available. */
+	if ( cpu_has(c, X86_FEATURE_MONITOR) &&
+	     ((cpuid_ecx(CPUID_MWAIT_LEAF) & CPUID_MWAIT_MIN_FEATURES) ==
+	      CPUID_MWAIT_MIN_FEATURES) )
+		set_bit(X86_FEATURE_XEN_MONITOR, c->x86_capability);
+
 	if ( (c->cpuid_level >= CPUID_PM_LEAF) &&
 	     (cpuid_ecx(CPUID_PM_LEAF) & CPUID6_ECX_APERFMPERF_CAPABILITY) )
 		set_bit(X86_FEATURE_APERFMPERF, c->x86_capability);
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 84cc51d..8b24e0e 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -22,10 +22,6 @@
 #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
 #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
 
-#define CPUID_MWAIT_LEAF                5
-#define CPUID5_ECX_EXTENSIONS_SUPPORTED 0x1
-#define CPUID5_ECX_INTERRUPT_BREAK      0x2
-
 #define CPUID_PM_LEAF                    6
 #define CPUID6_ECX_APERFMPERF_CAPABILITY 0x1
 
@@ -104,6 +100,7 @@
 #define cpu_has_arch_perfmon    boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
 #define cpu_has_cpuid_faulting  boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
 #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
+#define cpu_has_xen_monitor     boot_cpu_has(X86_FEATURE_XEN_MONITOR)
 
 enum _cache_type {
     CACHE_TYPE_NULL = 0,
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index bc98227..98637d0 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -22,3 +22,4 @@ XEN_CPUFEATURE(APERFMPERF,      (FSCAPINTS+0)*32+ 8) /* APERFMPERF */
 XEN_CPUFEATURE(MFENCE_RDTSC,    (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTSC */
 XEN_CPUFEATURE(XEN_SMEP,        (FSCAPINTS+0)*32+10) /* SMEP gets used by Xen itself */
 XEN_CPUFEATURE(XEN_SMAP,        (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen itself */
+XEN_CPUFEATURE(XEN_MONITOR,     (FSCAPINTS+0)*32+12) /* Xen uses MONITOR */
diff --git a/xen/include/asm-x86/mwait.h b/xen/include/asm-x86/mwait.h
index ba9c0ea..a1bfeb1 100644
--- a/xen/include/asm-x86/mwait.h
+++ b/xen/include/asm-x86/mwait.h
@@ -9,6 +9,9 @@
 #define CPUID5_ECX_EXTENSIONS_SUPPORTED 0x1
 #define CPUID5_ECX_INTERRUPT_BREAK	0x2
 
+#define CPUID_MWAIT_MIN_FEATURES \
+    (CPUID5_ECX_EXTENSIONS_SUPPORTED | CPUID5_ECX_INTERRUPT_BREAK)
+
 #define MWAIT_ECX_INTERRUPT_BREAK	0x1
 
 void mwait_idle_with_hints(unsigned int eax, unsigned int ecx);
-- 
2.1.4


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

Re: [PATCH 1/5] x86/idt: Factor out enabling and disabling of ISTs

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 372 bytes --]

On 1/12/18 12:37 PM, Andrew Cooper wrote:
> All alteration of IST settings (other than the crash path) happen in an
> identical triple.  Introduce helpers to keep the triple in sync, and reduce
> the risk of opencoded mistakes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 2/5] x86/pv: Rename invalidate_shadow_ldt() to pv_destroy_ldt()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 314 bytes --]

On 1/12/18 12:37 PM, Andrew Cooper wrote:
> and move it into pv/descriptor-tables.c beside its GDT counterpart.  Reduce
> the !in_irq() check from a BUG_ON() to ASSERT().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 3/5] x86/pv: Break handle_ldt_mapping_fault() out of handle_gdt_ldt_mapping_fault()

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 792 bytes --]

On 1/12/18 12:37 PM, Andrew Cooper wrote:
> Adjust handle_ldt_mapping_fault() exclude the use of this fixup path for
> non-PV guests.  Well-formed code shouldn't reference the LDT while in HVM vcpu
> context, but currently on a context switch from PV to HVM context, there may
> be a stale LDT selector loaded, over an unmapped region.
> 
> By explicitly excluding HVM context at this point, we avoid erroneous
> hypervisor execution resulting in a cascade failure, by falling into
> pv_map_ldt_shadow_page().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Obviously a behavior change here but the rationale behind it seems clear
to me and well worth doing for the net positive result:

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Doug Goldstein]

[-- Attachment #1.1.1: Type: text/plain, Size: 1228 bytes --]

On 1/12/18 12:37 PM, Andrew Cooper wrote:
> Windows is the only OS which pages out kernel datastructures, so chances are
> good that this is a vestigial remnant of the PV Windows XP experiment.
> Furthermore the implementation is incomplete; it only functions for a present
> => not-present transition, rather than a present => read/write transition.
> 
> The for_each_vcpu() is one scalability limitation for PV guests, which can't
> reasonably be altered to be continuable.
> 
> One side effects of dropping paging out support is that now, the LDT (like the
> GDT) is only ever modified in current context, allowing us to drop
> shadow_ldt_mapcnt and shadow_ldt_lock from struct vcpu.
> 
> Another side effect is that the LDT no longer automatically cleans itself up
> on domain destruction.  Cover this by explicitly releasing the LDT frames at
> the same time as the GDT frames.
> 
> Finally, leave some asserts around to confirm the expected behaviour of all
> the functions playing with PGT_seg_desc_page references.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Makes sense to me and the code looks good.

Reviewed-by: Doug Goldstein <cardoe@cardoe.com>

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [PATCH 1/5] x86/idt: Factor out enabling and disabling of ISTs

[Wei Liu]

On Fri, Jan 12, 2018 at 06:37:20PM +0000, Andrew Cooper wrote:
> All alteration of IST settings (other than the crash path) happen in an
> identical triple.  Introduce helpers to keep the triple in sync, and reduce
> the risk of opencoded mistakes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 2/5] x86/pv: Rename invalidate_shadow_ldt() to pv_destroy_ldt()

[Wei Liu]

On Fri, Jan 12, 2018 at 06:37:21PM +0000, Andrew Cooper wrote:
> and move it into pv/descriptor-tables.c beside its GDT counterpart.  Reduce
> the !in_irq() check from a BUG_ON() to ASSERT().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 1/5] x86/idt: Factor out enabling and disabling of ISTs

[Jan Beulich]

>>> On 17.01.18 at 15:43, <wei.liu2@citrix.com> wrote:
> On Fri, Jan 12, 2018 at 06:37:20PM +0000, Andrew Cooper wrote:
>> All alteration of IST settings (other than the crash path) happen in an
>> identical triple.  Introduce helpers to keep the triple in sync, and reduce
>> the risk of opencoded mistakes.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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



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

Re: [PATCH 2/5] x86/pv: Rename invalidate_shadow_ldt() to pv_destroy_ldt()

[Jan Beulich]

>>> On 17.01.18 at 15:43, <wei.liu2@citrix.com> wrote:
> On Fri, Jan 12, 2018 at 06:37:21PM +0000, Andrew Cooper wrote:
>> and move it into pv/descriptor-tables.c beside its GDT counterpart.  Reduce
>> the !in_irq() check from a BUG_ON() to ASSERT().
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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



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

Re: [PATCH 3/5] x86/pv: Break handle_ldt_mapping_fault() out of handle_gdt_ldt_mapping_fault()

[Jan Beulich]

>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
> Adjust handle_ldt_mapping_fault() exclude the use of this fixup path for
> non-PV guests.  Well-formed code shouldn't reference the LDT while in HVM vcpu
> context, but currently on a context switch from PV to HVM context, there may
> be a stale LDT selector loaded, over an unmapped region.
> 
> By explicitly excluding HVM context at this point, we avoid erroneous
> hypervisor execution resulting in a cascade failure, by falling into
> pv_map_ldt_shadow_page().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Jan Beulich]

>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
> Windows is the only OS which pages out kernel datastructures, so chances are
> good that this is a vestigial remnant of the PV Windows XP experiment.

This is based on what? How do you know there are no other OSes
doing so, including perhaps ones which none of us has ever heard
of? The diffstat of the change here is certainly nice, but as always
with something being removed from the ABI I'm rather hesitant.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>          {
>              for_each_vcpu ( d, v )
>              {
> -                /*
> -                 * Relinquish GDT mappings. No need for explicit unmapping of
> -                 * the LDT as it automatically gets squashed with the guest
> -                 * mappings.
> -                 */
> +                /* Relinquish GDT/LDT mappings. */
> +                pv_destroy_ldt(v);
>                  pv_destroy_gdt(v);

The domain is dead at this point, so the order doesn't matter much,
but strictly speaking you should destroy the GDT before destroying
the LDT (just like LDT _loads_ always need to come _after_ GDT
adjustments).

Everything else here looks fine, but the initial comment may need
further discussion. For example we may want to consider a
two-stage phasing out of the feature, with a couple of years in
between: Make the functionality dependent upon a default-off
command line option for the time being, and issue a bright warning
when someone actually enables it (telling them to tell us).

Jan


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

Re: [PATCH 5/5] x86/monitor: Capture Xen's intent to use monitor at boot time

[Jan Beulich]

>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
> @@ -312,6 +313,12 @@ static void generic_identify(struct cpuinfo_x86 *c)
>  	if ( cpu_has(c, X86_FEATURE_CLFLUSH) )
>  		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
>  
> +	/* Xen only uses MONITOR if INTERRUPT_BREAK is available. */
> +	if ( cpu_has(c, X86_FEATURE_MONITOR) &&
> +	     ((cpuid_ecx(CPUID_MWAIT_LEAF) & CPUID_MWAIT_MIN_FEATURES) ==
> +	      CPUID_MWAIT_MIN_FEATURES) )
> +		set_bit(X86_FEATURE_XEN_MONITOR, c->x86_capability);

Leaving aside that this could/should be __set_bit(), I think we
shouldn't do anything like this for synthetic feature bits and at
the same time ...

> @@ -104,6 +100,7 @@
>  #define cpu_has_arch_perfmon    boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
>  #define cpu_has_cpuid_faulting  boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
>  #define cpu_has_aperfmperf      boot_cpu_has(X86_FEATURE_APERFMPERF)
> +#define cpu_has_xen_monitor     boot_cpu_has(X86_FEATURE_XEN_MONITOR)

... test for them like this. Either we're certain all CPUs are identical,
in which case the above should be setup_force_cpu_cap(), or each
CPU should really inspect its own feature flag before using the
feature. I'd got the former route for now, and with that change
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[George Dunlap]

On Thu, Jan 18, 2018 at 7:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
>> Windows is the only OS which pages out kernel datastructures, so chances are
>> good that this is a vestigial remnant of the PV Windows XP experiment.
>
> This is based on what? How do you know there are no other OSes
> doing so, including perhaps ones which none of us has ever heard
> of? The diffstat of the change here is certainly nice, but as always
> with something being removed from the ABI I'm rather hesitant.
>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>>          {
>>              for_each_vcpu ( d, v )
>>              {
>> -                /*
>> -                 * Relinquish GDT mappings. No need for explicit unmapping of
>> -                 * the LDT as it automatically gets squashed with the guest
>> -                 * mappings.
>> -                 */
>> +                /* Relinquish GDT/LDT mappings. */
>> +                pv_destroy_ldt(v);
>>                  pv_destroy_gdt(v);
>
> The domain is dead at this point, so the order doesn't matter much,
> but strictly speaking you should destroy the GDT before destroying
> the LDT (just like LDT _loads_ always need to come _after_ GDT
> adjustments).
>
> Everything else here looks fine, but the initial comment may need
> further discussion. For example we may want to consider a
> two-stage phasing out of the feature, with a couple of years in
> between: Make the functionality dependent upon a default-off
> command line option for the time being, and issue a bright warning
> when someone actually enables it (telling them to tell us).

One of the problems we have is that people seem to wait for 2-3 years
after a release has been made to start updating to it.  So we'd have
to leave such a warning for probably 5 years minimum.

 -George

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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Jan Beulich]

>>> On 18.01.18 at 11:38, <dunlapg@umich.edu> wrote:
> On Thu, Jan 18, 2018 at 7:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>>>          {
>>>              for_each_vcpu ( d, v )
>>>              {
>>> -                /*
>>> -                 * Relinquish GDT mappings. No need for explicit unmapping of
>>> -                 * the LDT as it automatically gets squashed with the guest
>>> -                 * mappings.
>>> -                 */
>>> +                /* Relinquish GDT/LDT mappings. */
>>> +                pv_destroy_ldt(v);
>>>                  pv_destroy_gdt(v);
>>
>> The domain is dead at this point, so the order doesn't matter much,
>> but strictly speaking you should destroy the GDT before destroying
>> the LDT (just like LDT _loads_ always need to come _after_ GDT
>> adjustments).
>>
>> Everything else here looks fine, but the initial comment may need
>> further discussion. For example we may want to consider a
>> two-stage phasing out of the feature, with a couple of years in
>> between: Make the functionality dependent upon a default-off
>> command line option for the time being, and issue a bright warning
>> when someone actually enables it (telling them to tell us).
> 
> One of the problems we have is that people seem to wait for 2-3 years
> after a release has been made to start updating to it.  So we'd have
> to leave such a warning for probably 5 years minimum.

That's a reasonable time frame imo for phasing out something that's
a de-facto part of an ABI.

Jan


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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Andrew Cooper]

On 18/01/18 10:38, George Dunlap wrote:
> On Thu, Jan 18, 2018 at 7:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
>>> Windows is the only OS which pages out kernel datastructures, so chances are
>>> good that this is a vestigial remnant of the PV Windows XP experiment.
>> This is based on what? How do you know there are no other OSes
>> doing so, including perhaps ones which none of us has ever heard
>> of?

The chances of there being a production OS ported to PV that we've never
head of 0.

>>  The diffstat of the change here is certainly nice, but as always
>> with something being removed from the ABI I'm rather hesitant.
>>
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>>>          {
>>>              for_each_vcpu ( d, v )
>>>              {
>>> -                /*
>>> -                 * Relinquish GDT mappings. No need for explicit unmapping of
>>> -                 * the LDT as it automatically gets squashed with the guest
>>> -                 * mappings.
>>> -                 */
>>> +                /* Relinquish GDT/LDT mappings. */
>>> +                pv_destroy_ldt(v);
>>>                  pv_destroy_gdt(v);
>> The domain is dead at this point, so the order doesn't matter much,
>> but strictly speaking you should destroy the GDT before destroying
>> the LDT (just like LDT _loads_ always need to come _after_ GDT
>> adjustments).

By that logic, I've got it the correct way round (which is they way I
intended).  Allocation and freeing of the LDT is nested within the scope
of the GDT.

>>
>> Everything else here looks fine, but the initial comment may need
>> further discussion. For example we may want to consider a
>> two-stage phasing out of the feature, with a couple of years in
>> between: Make the functionality dependent upon a default-off
>> command line option for the time being, and issue a bright warning
>> when someone actually enables it (telling them to tell us).
> One of the problems we have is that people seem to wait for 2-3 years
> after a release has been made to start updating to it.  So we'd have
> to leave such a warning for probably 5 years minimum.

In almost any other case, I'd agree, and would be the first to suggest this.

However, This patch is strictly necessary for a more complete XPTI,
which is how I stumbled onto the issue in my KAISER series to begin
with.  It is the only codepath where we ever poke at a remote critical
data structure, which is why

Also as noted in the commit message, it is broken even in the case you
wanted to sensibly page the LDT, which further backs up the exception
that it was only for Windows XP, and has never ever encountered a
production system.

~Andrew

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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Andrew Cooper]

On 18/01/18 10:57, Andrew Cooper wrote:
> On 18/01/18 10:38, George Dunlap wrote:
>> On Thu, Jan 18, 2018 at 7:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
>>>> Windows is the only OS which pages out kernel datastructures, so chances are
>>>> good that this is a vestigial remnant of the PV Windows XP experiment.
>>> This is based on what? How do you know there are no other OSes
>>> doing so, including perhaps ones which none of us has ever heard
>>> of?
> The chances of there being a production OS ported to PV that we've never
> head of 0.
>
>>>  The diffstat of the change here is certainly nice, but as always
>>> with something being removed from the ABI I'm rather hesitant.
>>>
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>>>>          {
>>>>              for_each_vcpu ( d, v )
>>>>              {
>>>> -                /*
>>>> -                 * Relinquish GDT mappings. No need for explicit unmapping of
>>>> -                 * the LDT as it automatically gets squashed with the guest
>>>> -                 * mappings.
>>>> -                 */
>>>> +                /* Relinquish GDT/LDT mappings. */
>>>> +                pv_destroy_ldt(v);
>>>>                  pv_destroy_gdt(v);
>>> The domain is dead at this point, so the order doesn't matter much,
>>> but strictly speaking you should destroy the GDT before destroying
>>> the LDT (just like LDT _loads_ always need to come _after_ GDT
>>> adjustments).
> By that logic, I've got it the correct way round (which is they way I
> intended).  Allocation and freeing of the LDT is nested within the scope
> of the GDT.
>
>>> Everything else here looks fine, but the initial comment may need
>>> further discussion. For example we may want to consider a
>>> two-stage phasing out of the feature, with a couple of years in
>>> between: Make the functionality dependent upon a default-off
>>> command line option for the time being, and issue a bright warning
>>> when someone actually enables it (telling them to tell us).
>> One of the problems we have is that people seem to wait for 2-3 years
>> after a release has been made to start updating to it.  So we'd have
>> to leave such a warning for probably 5 years minimum.
> In almost any other case, I'd agree, and would be the first to suggest this.
>
> However, This patch is strictly necessary for a more complete XPTI,
> which is how I stumbled onto the issue in my KAISER series to begin
> with.  It is the only codepath where we ever poke at a remote critical
> data structure, which is why

Sorry - sent too early.

... which is why isolating the LDT in a per-cpu doesn't work in
combination with this algorithm.

~Andrew

>
> Also as noted in the commit message, it is broken even in the case you
> wanted to sensibly page the LDT, which further backs up the exception
> that it was only for Windows XP, and has never ever encountered a
> production system.
>
> ~Andrew


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

Re: [PATCH 4/5] x86/pv: Drop support for paging out the LDT

[Jan Beulich]

>>> On 18.01.18 at 12:00, <andrew.cooper3@citrix.com> wrote:
> On 18/01/18 10:57, Andrew Cooper wrote:
>> On 18/01/18 10:38, George Dunlap wrote:
>>> On Thu, Jan 18, 2018 at 7:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 12.01.18 at 19:37, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -1942,11 +1942,8 @@ int domain_relinquish_resources(struct domain *d)
>>>>>          {
>>>>>              for_each_vcpu ( d, v )
>>>>>              {
>>>>> -                /*
>>>>> -                 * Relinquish GDT mappings. No need for explicit unmapping of
>>>>> -                 * the LDT as it automatically gets squashed with the guest
>>>>> -                 * mappings.
>>>>> -                 */
>>>>> +                /* Relinquish GDT/LDT mappings. */
>>>>> +                pv_destroy_ldt(v);
>>>>>                  pv_destroy_gdt(v);
>>>> The domain is dead at this point, so the order doesn't matter much,
>>>> but strictly speaking you should destroy the GDT before destroying
>>>> the LDT (just like LDT _loads_ always need to come _after_ GDT
>>>> adjustments).
>> By that logic, I've got it the correct way round (which is they way I
>> intended).  Allocation and freeing of the LDT is nested within the scope
>> of the GDT.

Ah, I see how I didn't properly express what I mean. The idea
behind the remark was that the GDT might still have a reference
to the LDT, which would become stale with the LDT gone. The
better thing to compare with would be construction of an LDT
versus insertion of the respective descriptor into the GDT.

>>>> Everything else here looks fine, but the initial comment may need
>>>> further discussion. For example we may want to consider a
>>>> two-stage phasing out of the feature, with a couple of years in
>>>> between: Make the functionality dependent upon a default-off
>>>> command line option for the time being, and issue a bright warning
>>>> when someone actually enables it (telling them to tell us).
>>> One of the problems we have is that people seem to wait for 2-3 years
>>> after a release has been made to start updating to it.  So we'd have
>>> to leave such a warning for probably 5 years minimum.
>> In almost any other case, I'd agree, and would be the first to suggest this.
>>
>> However, This patch is strictly necessary for a more complete XPTI,
>> which is how I stumbled onto the issue in my KAISER series to begin
>> with.  It is the only codepath where we ever poke at a remote critical
>> data structure, which is why
> 
> Sorry - sent too early.
> 
> ... which is why isolating the LDT in a per-cpu doesn't work in
> combination with this algorithm.

Right, in the context of that series I can see it likely becoming
unavoidable to remove this functionality (aiui it could be kept,
but would become more complicated). Not having heard back
on the incompleteness discussion on stage 1 yet, I can't really
conclude at this point whether we will actually _need_ (not
just want) this full series making things per-CPU (taken together
with there so far not being any promise of it to help recover
performance).

Jan


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