[PATCH v3 0/7] Make CONFIG_PV work on x86

[PATCH v3 0/7] Make CONFIG_PV work on x86

[Wei Liu]

Hi all

This series makes CONFIG_PV work.

Booting a hypervisor with PVH Dom0 works.

Due to an issue in Xen implementation, XTF tests cause hypervisor to crash (seen
on staging as well). But with a local patch to work around the issue,
all XTF HVM tests passed.

See v1 cover letter for more information.

Wei.

Wei Liu (7):
  x86: make traps.c build with !CONFIG_PV
  x86/domctl: rework XEN_DOMCTL_{set,get}_address_size
  x86: make PV hypercall entry points work with !CONFIG_PV
  x86: rearrange x86_64/entry.S
  x86: make entry point code build when !CONFIG_PV
  x86: expose CONFIG_PV
  x86: update help string for CONFIG_HVM

 xen/arch/x86/Kconfig            |  17 +++--
 xen/arch/x86/domctl.c           |  32 +++++++---
 xen/arch/x86/hvm/vmx/vmcs.c     |   5 +-
 xen/arch/x86/traps.c            |  16 +++++-
 xen/arch/x86/x86_64/Makefile    |   2 +-
 xen/arch/x86/x86_64/entry.S     | 104 +++++++++++++++++++++++----------
 xen/arch/x86/x86_64/traps.c     |  19 +++++-
 xen/include/asm-x86/asm_defns.h |   4 +-
 xen/include/asm-x86/domain.h    |   7 ++-
 9 files changed, 156 insertions(+), 50 deletions(-)

base-commit: 6cb27e417e57c2f4d689fa19971f20f75e9c0708
-- 
git-series 0.9.1

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

[PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV

[Wei Liu]

Provide a stub for pv_inject_event, put code that accesses PV fields
and GDT / LDT fault handling code under CONFIG_PV.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3:
1. don't skip updating dr_masks in activate_debugregs.
2. remove ASSERT_UNREACHABLE in fixup_page_fault.

v2: reduce the amount of ifdefs
---
 xen/arch/x86/traps.c         | 16 ++++++++++++++++
 xen/include/asm-x86/domain.h |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c60c8f5..69449e0 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1115,6 +1115,7 @@ static void reserved_bit_page_fault(unsigned long addr,
     show_execution_state(regs);
 }
 
+#ifdef CONFIG_PV
 static int handle_ldt_mapping_fault(unsigned int offset,
                                     struct cpu_user_regs *regs)
 {
@@ -1185,6 +1186,7 @@ static int handle_gdt_ldt_mapping_fault(unsigned long offset,
 
     return EXCRET_fault_fixed;
 }
+#endif
 
 #define IN_HYPERVISOR_RANGE(va) \
     (((va) >= HYPERVISOR_VIRT_START) && ((va) < HYPERVISOR_VIRT_END))
@@ -1335,10 +1337,12 @@ static int fixup_page_fault(unsigned long addr, struct cpu_user_regs *regs)
 
     if ( unlikely(IN_HYPERVISOR_RANGE(addr)) )
     {
+#ifdef CONFIG_PV
         if ( !(regs->error_code & (PFEC_user_mode | PFEC_reserved_bit)) &&
              (addr >= GDT_LDT_VIRT_START) && (addr < GDT_LDT_VIRT_END) )
             return handle_gdt_ldt_mapping_fault(
                 addr - GDT_LDT_VIRT_START, regs);
+#endif
         return 0;
     }
 
@@ -1494,7 +1498,9 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
 
 void do_general_protection(struct cpu_user_regs *regs)
 {
+#ifdef CONFIG_PV
     struct vcpu *v = current;
+#endif
     unsigned long fixup;
 
     if ( debugger_trap_entry(TRAP_gp_fault, regs) )
@@ -1506,6 +1512,7 @@ void do_general_protection(struct cpu_user_regs *regs)
     if ( !guest_mode(regs) )
         goto gp_in_kernel;
 
+#ifdef CONFIG_PV
     /*
      * Cunning trick to allow arbitrary "INT n" handling.
      *
@@ -1557,6 +1564,7 @@ void do_general_protection(struct cpu_user_regs *regs)
     /* Pass on GPF as is. */
     pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
     return;
+#endif
 
  gp_in_kernel:
 
@@ -1744,7 +1752,9 @@ void unset_nmi_callback(void)
 
 void do_device_not_available(struct cpu_user_regs *regs)
 {
+#ifdef CONFIG_PV
     struct vcpu *curr = current;
+#endif
 
     if ( !guest_mode(regs) )
     {
@@ -1762,6 +1772,7 @@ void do_device_not_available(struct cpu_user_regs *regs)
         return;
     }
 
+#ifdef CONFIG_PV
     vcpu_restore_fpu_lazy(curr);
 
     if ( curr->arch.pv.ctrlreg[0] & X86_CR0_TS )
@@ -1771,6 +1782,9 @@ void do_device_not_available(struct cpu_user_regs *regs)
     }
     else
         TRACE_0D(TRC_PV_MATH_STATE_RESTORE);
+#else
+    ASSERT_UNREACHABLE();
+#endif
 
     return;
 }
@@ -2078,6 +2092,7 @@ void activate_debugregs(const struct vcpu *curr)
     }
 }
 
+#ifdef CONFIG_PV
 /*
  * Used by hypercalls and the emulator.
  *  -ENODEV => #UD
@@ -2193,6 +2208,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 
     return 0;
 }
+#endif  /* CONFIG_PV */
 
 void asm_domain_crash_synchronous(unsigned long addr)
 {
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7214037..643e69a 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -681,7 +681,14 @@ void arch_vcpu_regs_init(struct vcpu *v);
 struct vcpu_hvm_context;
 int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
 
+#ifdef CONFIG_PV
 void pv_inject_event(const struct x86_event *event);
+#else
+static inline void pv_inject_event(const struct x86_event *event)
+{
+    ASSERT_UNREACHABLE();
+}
+#endif
 
 static inline void pv_inject_hw_exception(unsigned int vector, int errcode)
 {
-- 
git-series 0.9.1

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

[PATCH v3 2/7] x86/domctl: rework XEN_DOMCTL_{set, get}_address_size

[Wei Liu]

Going through toolstack code, they are used for PV guests only.

Tighten their access to PV only. Return -EOPNOTSUPP if they are called
on HVM guests. Rewrite the code in a pattern that makes DCE work.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: rewritten
---
 xen/arch/x86/domctl.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index f79827e..33f9a86 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -609,19 +609,33 @@ long arch_do_domctl(
         break;
 
     case XEN_DOMCTL_set_address_size:
-        if ( ((domctl->u.address_size.size == 64) && !d->arch.is_32bit_pv) ||
-             ((domctl->u.address_size.size == 32) && d->arch.is_32bit_pv) )
-            ret = 0;
-        else if ( domctl->u.address_size.size == 32 )
-            ret = switch_compat(d);
+        if ( is_hvm_domain(d) )
+            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) )
+                ret = 0;
+            else if ( domctl->u.address_size.size == 32 )
+                ret = switch_compat(d);
+            else
+                ret = -EINVAL;
+        }
         else
-            ret = -EINVAL;
+            ASSERT_UNREACHABLE();
         break;
 
     case XEN_DOMCTL_get_address_size:
-        domctl->u.address_size.size = is_pv_32bit_domain(d) ? 32 :
-                                                              BITS_PER_LONG;
-        copyback = true;
+        if ( is_hvm_domain(d) )
+            ret = -EOPNOTSUPP;
+        else if ( is_pv_domain(d) )
+        {
+            domctl->u.address_size.size =
+                is_pv_32bit_domain(d) ? 32 : BITS_PER_LONG;
+            copyback = true;
+        }
+        else
+            ASSERT_UNREACHABLE();
         break;
 
     case XEN_DOMCTL_set_machine_address_size:
-- 
git-series 0.9.1

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

[PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

We want Xen to crash if we hit these paths when PV is disabled.

For syscall, we provide stubs for {l,c}star_enter which end up calling
panic.  For sysenter, we initialise CS to 0 so that #GP can be raised.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: rewrite
---
 xen/arch/x86/hvm/vmx/vmcs.c |  5 +++--
 xen/arch/x86/x86_64/traps.c | 19 +++++++++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index d9747b4..dec21d1 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1160,8 +1160,9 @@ static int construct_vmcs(struct vcpu *v)
     __vmwrite(HOST_RIP, (unsigned long)vmx_asm_vmexit_handler);
 
     /* Host SYSENTER CS:RIP. */
-    __vmwrite(HOST_SYSENTER_CS, __HYPERVISOR_CS);
-    __vmwrite(HOST_SYSENTER_EIP, (unsigned long)sysenter_entry);
+    __vmwrite(HOST_SYSENTER_CS, IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0);
+    __vmwrite(HOST_SYSENTER_EIP,
+              IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
 
     /* MSR intercepts. */
     __vmwrite(VM_EXIT_MSR_LOAD_COUNT, 0);
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 27154f2..35a60d4 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
 }
 
 DEFINE_PER_CPU(struct stubs, stubs);
+
+#ifdef CONFIG_PV
 void lstar_enter(void);
 void cstar_enter(void);
+#else
+static inline void lstar_enter(void)
+{
+    panic("%s called", __func__);
+}
+
+static inline void cstar_enter(void)
+{
+    panic("%s called", __func__);
+}
+#endif /* CONFIG_PV */
 
 void subarch_percpu_traps_init(void)
 {
@@ -329,8 +342,10 @@ void subarch_percpu_traps_init(void)
     {
         /* SYSENTER entry. */
         wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
-        wrmsrl(MSR_IA32_SYSENTER_EIP, (unsigned long)sysenter_entry);
-        wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
+        wrmsrl(MSR_IA32_SYSENTER_EIP,
+               IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
+        wrmsr(MSR_IA32_SYSENTER_CS,
+              IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0, 0);
     }
 
     /* Trampoline for SYSCALL entry from compatibility mode. */
-- 
git-series 0.9.1

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

[PATCH v3 4/7] x86: rearrange x86_64/entry.S

[Wei Liu]

Split the file into two halves. The first half pertains to PV guest
code while the second half is mostly used by the hypervisor itself to
handle interrupts and exceptions.

No functional change intended.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: remove self_ipi_restore_all_guest
v2: new, requested by Andrew
---
 xen/arch/x86/x86_64/entry.S | 65 ++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 48cb96c..9b02899 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -121,16 +121,6 @@ process_trap:
         call create_bounce_frame
         jmp  test_all_events
 
-/* No special register assumptions. */
-ENTRY(ret_from_intr)
-        GET_CURRENT(bx)
-        testb $3, UREGS_cs(%rsp)
-        jz    restore_all_xen
-        movq  VCPU_domain(%rbx), %rax
-        cmpb  $0, DOMAIN_is_32bit_pv(%rax)
-        je    test_all_events
-        jmp   compat_test_all_events
-
         .section .text.entry, "ax", @progbits
 
 /* %rbx: struct vcpu, interrupts disabled */
@@ -211,26 +201,6 @@ iret_exit_to_guest:
 .Lft0:  iretq
         _ASM_PRE_EXTABLE(.Lft0, handle_exception)
 
-        ALIGN
-/* No special register assumptions. */
-restore_all_xen:
-        /*
-         * Check whether we need to switch to the per-CPU page tables, in
-         * case we return to late PV exit code (from an NMI or #MC).
-         */
-        GET_STACK_END(bx)
-        cmpb  $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
-UNLIKELY_START(ne, exit_cr3)
-        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
-        mov   %rax, %cr3
-UNLIKELY_END(exit_cr3)
-
-        /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
-        SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
-
-        RESTORE_ALL adj=8
-        iretq
-
 /*
  * When entering SYSCALL from kernel mode:
  *  %rax                            = hypercall vector
@@ -553,8 +523,43 @@ ENTRY(dom_crash_sync_extable)
         jmp   asm_domain_crash_synchronous /* Does not return */
         .popsection
 
+/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
+
+        .text
+
+        ALIGN
+/* No special register assumptions. */
+ENTRY(ret_from_intr)
+        GET_CURRENT(bx)
+        testb $3, UREGS_cs(%rsp)
+        jz    restore_all_xen
+        movq  VCPU_domain(%rbx), %rax
+        cmpb  $0, DOMAIN_is_32bit_pv(%rax)
+        je    test_all_events
+        jmp   compat_test_all_events
+
         .section .text.entry, "ax", @progbits
 
+        ALIGN
+/* No special register assumptions. */
+restore_all_xen:
+        /*
+         * Check whether we need to switch to the per-CPU page tables, in
+         * case we return to late PV exit code (from an NMI or #MC).
+         */
+        GET_STACK_END(bx)
+        cmpb  $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
+UNLIKELY_START(ne, exit_cr3)
+        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
+        mov   %rax, %cr3
+UNLIKELY_END(exit_cr3)
+
+        /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+        SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
+
+        RESTORE_ALL adj=8
+        iretq
+
 ENTRY(common_interrupt)
         SAVE_ALL CLAC
 
-- 
git-series 0.9.1

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

[PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV

[Wei Liu]

Skip building x86_64/compat/entry.S and put CONFIG_PV in
x86_64/entry.S.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3:
1. make CR4_PV32_RESTORE expand to nothing when !PV
2. use unconditional jmp and add assertions

v2: new
---
 xen/arch/x86/x86_64/Makefile    |  2 +-
 xen/arch/x86/x86_64/entry.S     | 39 +++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/asm_defns.h |  4 +++-
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
index f336a6a..4bfa148 100644
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -1,4 +1,4 @@
-subdir-y += compat
+subdir-$(CONFIG_PV) += compat
 
 obj-bin-y += entry.o
 obj-y += traps.o
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 9b02899..975ed6a 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -15,6 +15,18 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+#ifndef NDEBUG
+/* %rsp: struct cpu_user_regs */
+#define ASSERT_INTERRUPTED_XEN_CONTEXT            \
+        testb $3, UREGS_cs(%rsp);                 \
+        jz    1f;                                 \
+        ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
+1:
+#else
+#define ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif
+
+#ifdef CONFIG_PV
 /* %rbx: struct vcpu */
 ENTRY(switch_to_kernel)
         leaq  VCPU_trap_bounce(%rbx),%rdx
@@ -522,6 +534,7 @@ ENTRY(dom_crash_sync_extable)
         xorl  %edi,%edi
         jmp   asm_domain_crash_synchronous /* Does not return */
         .popsection
+#endif /* CONFIG_PV */
 
 /* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
 
@@ -530,6 +543,7 @@ ENTRY(dom_crash_sync_extable)
         ALIGN
 /* No special register assumptions. */
 ENTRY(ret_from_intr)
+#ifdef CONFIG_PV
         GET_CURRENT(bx)
         testb $3, UREGS_cs(%rsp)
         jz    restore_all_xen
@@ -537,6 +551,10 @@ ENTRY(ret_from_intr)
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         je    test_all_events
         jmp   compat_test_all_events
+#else
+        ASSERT_INTERRUPTED_XEN_CONTEXT
+        jmp   restore_all_xen
+#endif
 
         .section .text.entry, "ax", @progbits
 
@@ -619,6 +637,7 @@ handle_exception_saved:
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
 
+#ifdef CONFIG_PV
         ALTERNATIVE_2 "jmp .Lcr4_pv32_done", \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
@@ -658,6 +677,9 @@ handle_exception_saved:
         test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
         jz    compat_test_all_events
 .Lcr4_pv32_done:
+#else
+        ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif /* CONFIG_PV */
         sti
 1:      movq  %rsp,%rdi
         movzbl UREGS_entry_vector(%rsp),%eax
@@ -667,12 +689,17 @@ handle_exception_saved:
         INDIRECT_CALL %rdx
         mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
         mov   %r13b, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
+#ifdef CONFIG_PV
         testb $3,UREGS_cs(%rsp)
         jz    restore_all_xen
         movq  VCPU_domain(%rbx),%rax
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         jne   compat_test_all_events
         jmp   test_all_events
+#else
+        ASSERT_INTERRUPTED_XEN_CONTEXT
+        jmp   restore_all_xen
+#endif
 
 /* No special register assumptions. */
 exception_with_ints_disabled:
@@ -823,6 +850,7 @@ handle_ist_exception:
         mov   %r12, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
 .List_cr3_okay:
 
+#ifdef CONFIG_PV
         CR4_PV32_RESTORE
         testb $3,UREGS_cs(%rsp)
         jz    1f
@@ -837,7 +865,11 @@ handle_ist_exception:
         movl  $UREGS_kernel_sizeof/8,%ecx
         movq  %rdi,%rsp
         rep   movsq
-1:      movq  %rsp,%rdi
+1:
+#else
+        ASSERT_INTERRUPTED_XEN_CONTEXT
+#endif
+        movq  %rsp,%rdi
         movzbl UREGS_entry_vector(%rsp),%eax
         leaq  exception_table(%rip),%rdx
         mov   (%rdx, %rax, 8), %rdx
@@ -848,6 +880,7 @@ handle_ist_exception:
         jne   ret_from_intr
 
         /* We want to get straight to the IRET on the NMI exit path. */
+#ifdef CONFIG_PV
         testb $3,UREGS_cs(%rsp)
         jz    restore_all_xen
         GET_CURRENT(bx)
@@ -863,6 +896,10 @@ handle_ist_exception:
         cmpb  $0,DOMAIN_is_32bit_pv(%rax)
         je    restore_all_guest
         jmp   compat_restore_all_guest
+#else
+        ASSERT_INTERRUPTED_XEN_CONTEXT
+        jmp   restore_all_xen
+#endif
 
 ENTRY(machine_check)
         pushq $0
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index da504ea..e688cf1 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -321,10 +321,14 @@ static always_inline void stac(void)
         subq  $-(UREGS_error_code-UREGS_r15+\adj), %rsp
 .endm
 
+#ifdef CONFIG_PV
 #define CR4_PV32_RESTORE                               \
     ALTERNATIVE_2 "",                                  \
         "call cr4_pv32_restore", X86_FEATURE_XEN_SMEP, \
         "call cr4_pv32_restore", X86_FEATURE_XEN_SMAP
+#else
+#define CR4_PV32_RESTORE
+#endif
 
 #include <asm/spec_ctrl_asm.h>
 
-- 
git-series 0.9.1

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

[PATCH v3 6/7] x86: expose CONFIG_PV

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: update text
v2: guest -> domain
---
 xen/arch/x86/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 548cbf9..37af728 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -37,6 +37,14 @@ source "arch/Kconfig"
 
 config PV
 	def_bool y
+	prompt "PV support"
+	---help---
+	  Interfaces to support PV domains. These require guest kernel support
+	  to run as a PV guest, but don't require any specific hardware support.
+
+	  This option is needed if you want to run PV domains.
+
+	  If unsure, say Y.
 
 config PV_LINEAR_PT
        bool "Support for PV linear pagetables"
-- 
git-series 0.9.1

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

[PATCH v3 7/7] x86: update help string for CONFIG_HVM

[Wei Liu]

Update text. Change "guest" to "domain" where appropriate because
"guest" doesn't include Domain 0.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v3: update text
v2: new
---
 xen/arch/x86/Kconfig |  9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 37af728..5c2d107 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -70,12 +70,11 @@ config HVM
 	def_bool !PV_SHIM_EXCLUSIVE
 	prompt "HVM support"
 	---help---
-	  Interfaces to support HVM guests which require hardware
-	  support like Intel's VT-x or AMD's SVM. Note the hypervisor
-	  doesn't distinguish HVM or PVH guest types. PVH guest type
-	  is only a concept for end users.
+	  Interfaces to support HVM domains.  HVM domains require hardware
+	  virtualisation extensions (e.g. Intel VT-x, AMD SVM), but can boot
+	  guests which have no specific Xen knowledge.
 
-	  This option is needed if you want to run HVM or PVH guests.
+	  This option is needed if you want to run HVM or PVH domains.
 
 	  If unsure, say Y.
 
-- 
git-series 0.9.1

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

Re: [PATCH v3 1/7] x86: make traps.c build with !CONFIG_PV

[Andrew Cooper]

On 02/11/18 15:55, Wei Liu wrote:
> @@ -2078,6 +2092,7 @@ void activate_debugregs(const struct vcpu *curr)
>      }
>  }
>  
> +#ifdef CONFIG_PV
>  /*
>   * Used by hypercalls and the emulator.
>   *  -ENODEV => #UD
> @@ -2193,6 +2208,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
>  
>      return 0;
>  }
> +#endif  /* CONFIG_PV */

set_debugreg() doesn't really belong here.  How about moving it to
pv/misc-hypercalls.c ?

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 2/7] x86/domctl: rework XEN_DOMCTL_{set, get}_address_size

[Andrew Cooper]

On 02/11/18 15:55, Wei Liu wrote:
> Going through toolstack code, they are used for PV guests only.
>
> Tighten their access to PV only. Return -EOPNOTSUPP if they are called
> on HVM guests. Rewrite the code in a pattern that makes DCE work.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Andrew Cooper]

On 02/11/18 15:55, Wei Liu wrote:
> We want Xen to crash if we hit these paths when PV is disabled.
>
> For syscall, we provide stubs for {l,c}star_enter which end up calling
> panic.  For sysenter, we initialise CS to 0 so that #GP can be raised.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3: rewrite
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c |  5 +++--
>  xen/arch/x86/x86_64/traps.c | 19 +++++++++++++++++--
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d9747b4..dec21d1 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1160,8 +1160,9 @@ static int construct_vmcs(struct vcpu *v)
>      __vmwrite(HOST_RIP, (unsigned long)vmx_asm_vmexit_handler);
>  
>      /* Host SYSENTER CS:RIP. */
> -    __vmwrite(HOST_SYSENTER_CS, __HYPERVISOR_CS);
> -    __vmwrite(HOST_SYSENTER_EIP, (unsigned long)sysenter_entry);
> +    __vmwrite(HOST_SYSENTER_CS, IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0);
> +    __vmwrite(HOST_SYSENTER_EIP,
> +              IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);

Jun/Kevin: Given that the VMCS backing page is zeroed, is it safe to
omit these VMWRITE's entirely in the !CONFIG_PV case, rather than
explicitly writing 0?

It would be more efficient, especially when nested, if it is safe to do so.

>  
>      /* MSR intercepts. */
>      __vmwrite(VM_EXIT_MSR_LOAD_COUNT, 0);
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 27154f2..35a60d4 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>  }
>  
>  DEFINE_PER_CPU(struct stubs, stubs);
> +
> +#ifdef CONFIG_PV
>  void lstar_enter(void);
>  void cstar_enter(void);
> +#else
> +static inline void lstar_enter(void)
> +{
> +    panic("%s called", __func__);

\n

With this fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +}
> +
> +static inline void cstar_enter(void)
> +{
> +    panic("%s called", __func__);
> +}
> +#endif /* CONFIG_PV */
>  
>  void subarch_percpu_traps_init(void)
>  {
> @@ -329,8 +342,10 @@ void subarch_percpu_traps_init(void)
>      {
>          /* SYSENTER entry. */
>          wrmsrl(MSR_IA32_SYSENTER_ESP, stack_bottom);
> -        wrmsrl(MSR_IA32_SYSENTER_EIP, (unsigned long)sysenter_entry);
> -        wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
> +        wrmsrl(MSR_IA32_SYSENTER_EIP,
> +               IS_ENABLED(CONFIG_PV) ? (unsigned long)sysenter_entry : 0);
> +        wrmsr(MSR_IA32_SYSENTER_CS,
> +              IS_ENABLED(CONFIG_PV) ? __HYPERVISOR_CS : 0, 0);
>      }
>  
>      /* Trampoline for SYSCALL entry from compatibility mode. */


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

Re: [PATCH v3 4/7] x86: rearrange x86_64/entry.S

[Andrew Cooper]

On 02/11/18 15:55, Wei Liu wrote:
> @@ -553,8 +523,43 @@ ENTRY(dom_crash_sync_extable)
>          jmp   asm_domain_crash_synchronous /* Does not return */
>          .popsection
>  
> +/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
> +
> +        .text
> +
> +        ALIGN

No need for an ALIGN here.  There is one inside ENTRY().

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +/* No special register assumptions. */
> +ENTRY(ret_from_intr)
> +        GET_CURRENT(bx)
> +        testb $3, UREGS_cs(%rsp)
> +        jz    restore_all_xen
> +        movq  VCPU_domain(%rbx), %rax
> +        cmpb  $0, DOMAIN_is_32bit_pv(%rax)
> +        je    test_all_events
> +        jmp   compat_test_all_events
> +
>          .section .text.entry, "ax", @progbits
>  
> +        ALIGN
> +/* No special register assumptions. */
> +restore_all_xen:
> +        /*
> +         * Check whether we need to switch to the per-CPU page tables, in
> +         * case we return to late PV exit code (from an NMI or #MC).
> +         */
> +        GET_STACK_END(bx)
> +        cmpb  $0, STACK_CPUINFO_FIELD(use_pv_cr3)(%rbx)
> +UNLIKELY_START(ne, exit_cr3)
> +        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rbx), %rax
> +        mov   %rax, %cr3
> +UNLIKELY_END(exit_cr3)
> +
> +        /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
> +        SPEC_CTRL_EXIT_TO_XEN_IST /* Req: %rbx=end, Clob: acd */
> +
> +        RESTORE_ALL adj=8
> +        iretq
> +
>  ENTRY(common_interrupt)
>          SAVE_ALL CLAC
>  


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

Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV

[Andrew Cooper]

On 02/11/18 15:55, Wei Liu wrote:
> Skip building x86_64/compat/entry.S and put CONFIG_PV in
> x86_64/entry.S.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3:
> 1. make CR4_PV32_RESTORE expand to nothing when !PV
> 2. use unconditional jmp and add assertions
>
> v2: new
> ---
>  xen/arch/x86/x86_64/Makefile    |  2 +-
>  xen/arch/x86/x86_64/entry.S     | 39 +++++++++++++++++++++++++++++++++-
>  xen/include/asm-x86/asm_defns.h |  4 +++-
>  3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
> index f336a6a..4bfa148 100644
> --- a/xen/arch/x86/x86_64/Makefile
> +++ b/xen/arch/x86/x86_64/Makefile
> @@ -1,4 +1,4 @@
> -subdir-y += compat
> +subdir-$(CONFIG_PV) += compat
>  
>  obj-bin-y += entry.o
>  obj-y += traps.o
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 9b02899..975ed6a 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -15,6 +15,18 @@
>  #include <public/xen.h>
>  #include <irq_vectors.h>
>  
> +#ifndef NDEBUG
> +/* %rsp: struct cpu_user_regs */
> +#define ASSERT_INTERRUPTED_XEN_CONTEXT            \

Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
of the cases below?

> +        testb $3, UREGS_cs(%rsp);                 \
> +        jz    1f;                                 \
> +        ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \

This probably wants to be:

.macro ASSERT_CONTEXT_IS_XEN
#ifndef NDEBUG
    testb $3, UREGS_cs(%rsp);
    UNLIKELY_START(nz, ASSERT_XEN_\@);
    ASSERT_FAILED("CONTEXT IS XEN");
    __UNLIKELY_END(ASSERT_XEN_\@);
#endif
.endm

which will assemble in the fastpath to:

testb $3, UREGS_cs(%rsp);
jne <unlikely>
jmp restore_all_xen

And avoid a conditional jump in the expected path.  (Code completely
untested.)

~Andrew

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Jan Beulich]

>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>  }
>  
>  DEFINE_PER_CPU(struct stubs, stubs);
> +
> +#ifdef CONFIG_PV
>  void lstar_enter(void);
>  void cstar_enter(void);
> +#else
> +static inline void lstar_enter(void)
> +{
> +    panic("%s called", __func__);
> +}
> +
> +static inline void cstar_enter(void)
> +{
> +    panic("%s called", __func__);
> +}
> +#endif /* CONFIG_PV */

Do we really need two separate stubs (and two separate string literals)
here?

Jan



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

Re: [PATCH v3 7/7] x86: update help string for CONFIG_HVM

[Jan Beulich]

>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> Update text. Change "guest" to "domain" where appropriate because
> "guest" doesn't include Domain 0.
> 
> Signed-off-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 v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> > --- a/xen/arch/x86/x86_64/traps.c
> > +++ b/xen/arch/x86/x86_64/traps.c
> > @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >  }
> >  
> >  DEFINE_PER_CPU(struct stubs, stubs);
> > +
> > +#ifdef CONFIG_PV
> >  void lstar_enter(void);
> >  void cstar_enter(void);
> > +#else
> > +static inline void lstar_enter(void)
> > +{
> > +    panic("%s called", __func__);
> > +}
> > +
> > +static inline void cstar_enter(void)
> > +{
> > +    panic("%s called", __func__);
> > +}
> > +#endif /* CONFIG_PV */
> 
> Do we really need two separate stubs (and two separate string literals)
> here?

I think it is clearer if we have two distinct messages. But I'm not too
fussed either way really. If you feel strongly about this, I'm happy to
change it to only one function.

Wei.

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

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Andrew Cooper]

On 05/11/18 15:48, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>> --- a/xen/arch/x86/x86_64/traps.c
>>> +++ b/xen/arch/x86/x86_64/traps.c
>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>  }
>>>  
>>>  DEFINE_PER_CPU(struct stubs, stubs);
>>> +
>>> +#ifdef CONFIG_PV
>>>  void lstar_enter(void);
>>>  void cstar_enter(void);
>>> +#else
>>> +static inline void lstar_enter(void)
>>> +{
>>> +    panic("%s called", __func__);
>>> +}
>>> +
>>> +static inline void cstar_enter(void)
>>> +{
>>> +    panic("%s called", __func__);
>>> +}
>>> +#endif /* CONFIG_PV */
>> Do we really need two separate stubs (and two separate string literals)
>> here?
> I think it is clearer if we have two distinct messages. But I'm not too
> fussed either way really. If you feel strongly about this, I'm happy to
> change it to only one function.

This is the correct way to do it.  __func__ will already be in the
string table, and the format string (being identical) will be merged.

~Andrew

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Mon, Nov 05, 2018 at 03:49:40PM +0000, Andrew Cooper wrote:
> On 05/11/18 15:48, Wei Liu wrote:
> > On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>> --- a/xen/arch/x86/x86_64/traps.c
> >>> +++ b/xen/arch/x86/x86_64/traps.c
> >>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>>  }
> >>>  
> >>>  DEFINE_PER_CPU(struct stubs, stubs);
> >>> +
> >>> +#ifdef CONFIG_PV
> >>>  void lstar_enter(void);
> >>>  void cstar_enter(void);
> >>> +#else
> >>> +static inline void lstar_enter(void)
> >>> +{
> >>> +    panic("%s called", __func__);
> >>> +}
> >>> +
> >>> +static inline void cstar_enter(void)
> >>> +{
> >>> +    panic("%s called", __func__);
> >>> +}
> >>> +#endif /* CONFIG_PV */
> >> Do we really need two separate stubs (and two separate string literals)
> >> here?
> > I think it is clearer if we have two distinct messages. But I'm not too
> > fussed either way really. If you feel strongly about this, I'm happy to
> > change it to only one function.
> 
> This is the correct way to do it.  __func__ will already be in the
> string table, and the format string (being identical) will be merged.

I think Jan was complaining two __func__'s (two string literals).

Wei.

> 
> ~Andrew

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Jan Beulich]

>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> On 05/11/18 15:48, Wei Liu wrote:
>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>>  }
>>>>  
>>>>  DEFINE_PER_CPU(struct stubs, stubs);
>>>> +
>>>> +#ifdef CONFIG_PV
>>>>  void lstar_enter(void);
>>>>  void cstar_enter(void);
>>>> +#else
>>>> +static inline void lstar_enter(void)
>>>> +{
>>>> +    panic("%s called", __func__);
>>>> +}
>>>> +
>>>> +static inline void cstar_enter(void)
>>>> +{
>>>> +    panic("%s called", __func__);
>>>> +}
>>>> +#endif /* CONFIG_PV */
>>> Do we really need two separate stubs (and two separate string literals)
>>> here?
>> I think it is clearer if we have two distinct messages. But I'm not too
>> fussed either way really. If you feel strongly about this, I'm happy to
>> change it to only one function.
> 
> This is the correct way to do it.  __func__ will already be in the
> string table, and the format string (being identical) will be merged.

Why would __func__ be in the string table already, for functions
containing no other references to it?

Jan



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

Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV

[Wei Liu]

On Mon, Nov 05, 2018 at 02:33:23PM +0000, Andrew Cooper wrote:
> On 02/11/18 15:55, Wei Liu wrote:
> > Skip building x86_64/compat/entry.S and put CONFIG_PV in
> > x86_64/entry.S.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > v3:
> > 1. make CR4_PV32_RESTORE expand to nothing when !PV
> > 2. use unconditional jmp and add assertions
> >
> > v2: new
> > ---
> >  xen/arch/x86/x86_64/Makefile    |  2 +-
> >  xen/arch/x86/x86_64/entry.S     | 39 +++++++++++++++++++++++++++++++++-
> >  xen/include/asm-x86/asm_defns.h |  4 +++-
> >  3 files changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
> > index f336a6a..4bfa148 100644
> > --- a/xen/arch/x86/x86_64/Makefile
> > +++ b/xen/arch/x86/x86_64/Makefile
> > @@ -1,4 +1,4 @@
> > -subdir-y += compat
> > +subdir-$(CONFIG_PV) += compat
> >  
> >  obj-bin-y += entry.o
> >  obj-y += traps.o
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index 9b02899..975ed6a 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -15,6 +15,18 @@
> >  #include <public/xen.h>
> >  #include <irq_vectors.h>
> >  
> > +#ifndef NDEBUG
> > +/* %rsp: struct cpu_user_regs */
> > +#define ASSERT_INTERRUPTED_XEN_CONTEXT            \
> 
> Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
> of the cases below?
> 
> > +        testb $3, UREGS_cs(%rsp);                 \
> > +        jz    1f;                                 \
> > +        ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
> 
> This probably wants to be:
> 
> .macro ASSERT_CONTEXT_IS_XEN
> #ifndef NDEBUG
>     testb $3, UREGS_cs(%rsp);
>     UNLIKELY_START(nz, ASSERT_XEN_\@);
>     ASSERT_FAILED("CONTEXT IS XEN");
>     __UNLIKELY_END(ASSERT_XEN_\@);
> #endif
> .endm

This works. I have deleted all semicolons though.

(To avoid posting other patches)

---8<---
From 8f3ce0b0a5b6a215d4309d77d2c63229fb4af900 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Fri, 19 Oct 2018 12:32:12 +0100
Subject: [PATCH] x86: make entry point code build when !CONFIG_PV

Skip building x86_64/compat/entry.S and put CONFIG_PV in
x86_64/entry.S.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v4:
1. Change name to ASSERT_CONTEXT_IS_XEN
2. Use UNLIKELY

v3:
1. make CR4_PV32_RESTORE expand to nothing when !PV
2. use unconditional jmp and add assertions

v2: new
---
 xen/arch/x86/x86_64/Makefile    |  2 +-
 xen/arch/x86/x86_64/entry.S     | 38 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/asm_defns.h |  4 ++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
index f336a6ae65..4bfa1480eb 100644
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -1,4 +1,4 @@
-subdir-y += compat
+subdir-$(CONFIG_PV) += compat
 
 obj-bin-y += entry.o
 obj-y += traps.o
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 8e12decea8..e8eae3b08d 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -15,6 +15,17 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+/* %rsp: struct cpu_user_regs */
+.macro ASSERT_CONTEXT_IS_XEN
+#ifndef NDEBUG
+        testb $3, UREGS_cs(%rsp)
+        UNLIKELY_START(nz, ASSERT_XEN_\@)
+        ASSERT_FAILED("INTERRUPTED XEN CONTEXT")
+        __UNLIKELY_END(ASSERT_XEN_\@)
+#endif
+.endm
+
+#ifdef CONFIG_PV
 /* %rbx: struct vcpu */
 ENTRY(switch_to_kernel)
         leaq  VCPU_trap_bounce(%rbx),%rdx
@@ -522,6 +533,7 @@ ENTRY(dom_crash_sync_extable)
         xorl  %edi,%edi
         jmp   asm_domain_crash_synchronous /* Does not return */
         .popsection
+#endif /* CONFIG_PV */
 
 /* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
 
@@ -529,6 +541,7 @@ ENTRY(dom_crash_sync_extable)
 
 /* No special register assumptions. */
 ENTRY(ret_from_intr)
+#ifdef CONFIG_PV
         GET_CURRENT(bx)
         testb $3, UREGS_cs(%rsp)
         jz    restore_all_xen
@@ -536,6 +549,10 @@ ENTRY(ret_from_intr)
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         je    test_all_events
         jmp   compat_test_all_events
+#else
+        ASSERT_CONTEXT_IS_XEN
+        jmp   restore_all_xen
+#endif
 
         .section .text.entry, "ax", @progbits
 
@@ -618,6 +635,7 @@ handle_exception_saved:
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
 
+#ifdef CONFIG_PV
         ALTERNATIVE_2 "jmp .Lcr4_pv32_done", \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMEP, \
             __stringify(mov VCPU_domain(%rbx), %rax), X86_FEATURE_XEN_SMAP
@@ -657,6 +675,9 @@ handle_exception_saved:
         test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
         jz    compat_test_all_events
 .Lcr4_pv32_done:
+#else
+        ASSERT_CONTEXT_IS_XEN
+#endif /* CONFIG_PV */
         sti
 1:      movq  %rsp,%rdi
         movzbl UREGS_entry_vector(%rsp),%eax
@@ -666,12 +687,17 @@ handle_exception_saved:
         INDIRECT_CALL %rdx
         mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
         mov   %r13b, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
+#ifdef CONFIG_PV
         testb $3,UREGS_cs(%rsp)
         jz    restore_all_xen
         movq  VCPU_domain(%rbx),%rax
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         jne   compat_test_all_events
         jmp   test_all_events
+#else
+        ASSERT_CONTEXT_IS_XEN
+        jmp   restore_all_xen
+#endif
 
 /* No special register assumptions. */
 exception_with_ints_disabled:
@@ -822,6 +848,7 @@ handle_ist_exception:
         mov   %r12, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
 .List_cr3_okay:
 
+#ifdef CONFIG_PV
         CR4_PV32_RESTORE
         testb $3,UREGS_cs(%rsp)
         jz    1f
@@ -836,7 +863,11 @@ handle_ist_exception:
         movl  $UREGS_kernel_sizeof/8,%ecx
         movq  %rdi,%rsp
         rep   movsq
-1:      movq  %rsp,%rdi
+1:
+#else
+        ASSERT_CONTEXT_IS_XEN
+#endif
+        movq  %rsp,%rdi
         movzbl UREGS_entry_vector(%rsp),%eax
         leaq  exception_table(%rip),%rdx
         mov   (%rdx, %rax, 8), %rdx
@@ -847,6 +878,7 @@ handle_ist_exception:
         jne   ret_from_intr
 
         /* We want to get straight to the IRET on the NMI exit path. */
+#ifdef CONFIG_PV
         testb $3,UREGS_cs(%rsp)
         jz    restore_all_xen
         GET_CURRENT(bx)
@@ -862,6 +894,10 @@ handle_ist_exception:
         cmpb  $0,DOMAIN_is_32bit_pv(%rax)
         je    restore_all_guest
         jmp   compat_restore_all_guest
+#else
+        ASSERT_CONTEXT_IS_XEN
+        jmp   restore_all_xen
+#endif
 
 ENTRY(machine_check)
         pushq $0
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index da504eaec2..e688cf1c16 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -321,10 +321,14 @@ static always_inline void stac(void)
         subq  $-(UREGS_error_code-UREGS_r15+\adj), %rsp
 .endm
 
+#ifdef CONFIG_PV
 #define CR4_PV32_RESTORE                               \
     ALTERNATIVE_2 "",                                  \
         "call cr4_pv32_restore", X86_FEATURE_XEN_SMEP, \
         "call cr4_pv32_restore", X86_FEATURE_XEN_SMAP
+#else
+#define CR4_PV32_RESTORE
+#endif
 
 #include <asm/spec_ctrl_asm.h>
 
-- 
2.11.0


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

Re: [PATCH v3 5/7] x86: make entry point code build when !CONFIG_PV

[Andrew Cooper]

On 05/11/18 17:08, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 02:33:23PM +0000, Andrew Cooper wrote:
>> On 02/11/18 15:55, Wei Liu wrote:
>>> Skip building x86_64/compat/entry.S and put CONFIG_PV in
>>> x86_64/entry.S.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>> v3:
>>> 1. make CR4_PV32_RESTORE expand to nothing when !PV
>>> 2. use unconditional jmp and add assertions
>>>
>>> v2: new
>>> ---
>>>  xen/arch/x86/x86_64/Makefile    |  2 +-
>>>  xen/arch/x86/x86_64/entry.S     | 39 +++++++++++++++++++++++++++++++++-
>>>  xen/include/asm-x86/asm_defns.h |  4 +++-
>>>  3 files changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/x86_64/Makefile b/xen/arch/x86/x86_64/Makefile
>>> index f336a6a..4bfa148 100644
>>> --- a/xen/arch/x86/x86_64/Makefile
>>> +++ b/xen/arch/x86/x86_64/Makefile
>>> @@ -1,4 +1,4 @@
>>> -subdir-y += compat
>>> +subdir-$(CONFIG_PV) += compat
>>>  
>>>  obj-bin-y += entry.o
>>>  obj-y += traps.o
>>> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
>>> index 9b02899..975ed6a 100644
>>> --- a/xen/arch/x86/x86_64/entry.S
>>> +++ b/xen/arch/x86/x86_64/entry.S
>>> @@ -15,6 +15,18 @@
>>>  #include <public/xen.h>
>>>  #include <irq_vectors.h>
>>>  
>>> +#ifndef NDEBUG
>>> +/* %rsp: struct cpu_user_regs */
>>> +#define ASSERT_INTERRUPTED_XEN_CONTEXT            \
>> Could I suggest ASSERT_CONTEXT_IS_XEN which I think fits better in some
>> of the cases below?
>>
>>> +        testb $3, UREGS_cs(%rsp);                 \
>>> +        jz    1f;                                 \
>>> +        ASSERT_FAILED("INTERRUPTED XEN CONTEXT"); \
>> This probably wants to be:
>>
>> .macro ASSERT_CONTEXT_IS_XEN
>> #ifndef NDEBUG
>>     testb $3, UREGS_cs(%rsp);
>>     UNLIKELY_START(nz, ASSERT_XEN_\@);
>>     ASSERT_FAILED("CONTEXT IS XEN");
>>     __UNLIKELY_END(ASSERT_XEN_\@);
>> #endif
>> .endm
> This works. I have deleted all semicolons though.

Nice!

>
> (To avoid posting other patches)
>
> ---8<---
> From 8f3ce0b0a5b6a215d4309d77d2c63229fb4af900 Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Fri, 19 Oct 2018 12:32:12 +0100
> Subject: [PATCH] x86: make entry point code build when !CONFIG_PV
>
> Skip building x86_64/compat/entry.S and put CONFIG_PV in
> x86_64/entry.S.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> > On 05/11/18 15:48, Wei Liu wrote:
> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>>> --- a/xen/arch/x86/x86_64/traps.c
> >>>> +++ b/xen/arch/x86/x86_64/traps.c
> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>>>  }
> >>>>  
> >>>>  DEFINE_PER_CPU(struct stubs, stubs);
> >>>> +
> >>>> +#ifdef CONFIG_PV
> >>>>  void lstar_enter(void);
> >>>>  void cstar_enter(void);
> >>>> +#else
> >>>> +static inline void lstar_enter(void)
> >>>> +{
> >>>> +    panic("%s called", __func__);
> >>>> +}
> >>>> +
> >>>> +static inline void cstar_enter(void)
> >>>> +{
> >>>> +    panic("%s called", __func__);
> >>>> +}
> >>>> +#endif /* CONFIG_PV */
> >>> Do we really need two separate stubs (and two separate string literals)
> >>> here?
> >> I think it is clearer if we have two distinct messages. But I'm not too
> >> fussed either way really. If you feel strongly about this, I'm happy to
> >> change it to only one function.
> > 
> > This is the correct way to do it.  __func__ will already be in the
> > string table, and the format string (being identical) will be merged.
> 
> Why would __func__ be in the string table already, for functions
> containing no other references to it?

What is the way forward? Do we really care if there is one more string
literal in the binary?

Wei.

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Andrew Cooper]

On 08/11/18 15:33, Wei Liu wrote:
> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>>> On 05/11/18 15:48, Wei Liu wrote:
>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>>>>  }
>>>>>>  
>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
>>>>>> +
>>>>>> +#ifdef CONFIG_PV
>>>>>>  void lstar_enter(void);
>>>>>>  void cstar_enter(void);
>>>>>> +#else
>>>>>> +static inline void lstar_enter(void)
>>>>>> +{
>>>>>> +    panic("%s called", __func__);
>>>>>> +}
>>>>>> +
>>>>>> +static inline void cstar_enter(void)
>>>>>> +{
>>>>>> +    panic("%s called", __func__);
>>>>>> +}
>>>>>> +#endif /* CONFIG_PV */
>>>>> Do we really need two separate stubs (and two separate string literals)
>>>>> here?
>>>> I think it is clearer if we have two distinct messages. But I'm not too
>>>> fussed either way really. If you feel strongly about this, I'm happy to
>>>> change it to only one function.
>>> This is the correct way to do it.  __func__ will already be in the
>>> string table, and the format string (being identical) will be merged.
>> Why would __func__ be in the string table already, for functions
>> containing no other references to it?
> What is the way forward? Do we really care if there is one more string
> literal in the binary?

No.  One extra string like this is not something which needs caring
about in the slightest.

~Andrew

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Jan Beulich]

>>> On 08.11.18 at 16:33, <wei.liu2@citrix.com> wrote:
> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>> > On 05/11/18 15:48, Wei Liu wrote:
>> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>> >>>> --- a/xen/arch/x86/x86_64/traps.c
>> >>>> +++ b/xen/arch/x86/x86_64/traps.c
>> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>> >>>>  }
>> >>>>  
>> >>>>  DEFINE_PER_CPU(struct stubs, stubs);
>> >>>> +
>> >>>> +#ifdef CONFIG_PV
>> >>>>  void lstar_enter(void);
>> >>>>  void cstar_enter(void);
>> >>>> +#else
>> >>>> +static inline void lstar_enter(void)
>> >>>> +{
>> >>>> +    panic("%s called", __func__);
>> >>>> +}
>> >>>> +
>> >>>> +static inline void cstar_enter(void)
>> >>>> +{
>> >>>> +    panic("%s called", __func__);
>> >>>> +}
>> >>>> +#endif /* CONFIG_PV */
>> >>> Do we really need two separate stubs (and two separate string literals)
>> >>> here?
>> >> I think it is clearer if we have two distinct messages. But I'm not too
>> >> fussed either way really. If you feel strongly about this, I'm happy to
>> >> change it to only one function.
>> > 
>> > This is the correct way to do it.  __func__ will already be in the
>> > string table, and the format string (being identical) will be merged.
>> 
>> Why would __func__ be in the string table already, for functions
>> containing no other references to it?
> 
> What is the way forward? Do we really care if there is one more string
> literal in the binary?

Well, if I understood what Andrew means, I could decide whether to
give up my objection. Yes, a single string literal and a few insn bytes
don't matter all that much. But nowadays everyone thinks this way
everywhere, and one has to load dozens if not hundreds of megabytes
of data just to get a basic OS booted.

Jan



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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Jan Beulich]

>>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> On 08/11/18 15:33, Wei Liu wrote:
>> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>>>> On 05/11/18 15:48, Wei Liu wrote:
>>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>>>>>>>  }
>>>>>>>  
>>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
>>>>>>> +
>>>>>>> +#ifdef CONFIG_PV
>>>>>>>  void lstar_enter(void);
>>>>>>>  void cstar_enter(void);
>>>>>>> +#else
>>>>>>> +static inline void lstar_enter(void)
>>>>>>> +{
>>>>>>> +    panic("%s called", __func__);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static inline void cstar_enter(void)
>>>>>>> +{
>>>>>>> +    panic("%s called", __func__);
>>>>>>> +}
>>>>>>> +#endif /* CONFIG_PV */
>>>>>> Do we really need two separate stubs (and two separate string literals)
>>>>>> here?
>>>>> I think it is clearer if we have two distinct messages. But I'm not too
>>>>> fussed either way really. If you feel strongly about this, I'm happy to
>>>>> change it to only one function.
>>>> This is the correct way to do it.  __func__ will already be in the
>>>> string table, and the format string (being identical) will be merged.
>>> Why would __func__ be in the string table already, for functions
>>> containing no other references to it?
>> What is the way forward? Do we really care if there is one more string
>> literal in the binary?
> 
> No.  One extra string like this is not something which needs caring
> about in the slightest.

So just to clarify - the unnecessary redundancy in the source is of more
relevance to me than the resulting slightly larger binary.

Jan



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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Thu, Nov 08, 2018 at 08:39:36AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 16:33, <wei.liu2@citrix.com> wrote:
> > On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >> >>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >> > On 05/11/18 15:48, Wei Liu wrote:
> >> >> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >> >>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >> >>>> --- a/xen/arch/x86/x86_64/traps.c
> >> >>>> +++ b/xen/arch/x86/x86_64/traps.c
> >> >>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >> >>>>  }
> >> >>>>  
> >> >>>>  DEFINE_PER_CPU(struct stubs, stubs);
> >> >>>> +
> >> >>>> +#ifdef CONFIG_PV
> >> >>>>  void lstar_enter(void);
> >> >>>>  void cstar_enter(void);
> >> >>>> +#else
> >> >>>> +static inline void lstar_enter(void)
> >> >>>> +{
> >> >>>> +    panic("%s called", __func__);
> >> >>>> +}
> >> >>>> +
> >> >>>> +static inline void cstar_enter(void)
> >> >>>> +{
> >> >>>> +    panic("%s called", __func__);
> >> >>>> +}
> >> >>>> +#endif /* CONFIG_PV */
> >> >>> Do we really need two separate stubs (and two separate string literals)
> >> >>> here?
> >> >> I think it is clearer if we have two distinct messages. But I'm not too
> >> >> fussed either way really. If you feel strongly about this, I'm happy to
> >> >> change it to only one function.
> >> > 
> >> > This is the correct way to do it.  __func__ will already be in the
> >> > string table, and the format string (being identical) will be merged.
> >> 
> >> Why would __func__ be in the string table already, for functions
> >> containing no other references to it?
> > 
> > What is the way forward? Do we really care if there is one more string
> > literal in the binary?
> 
> Well, if I understood what Andrew means, I could decide whether to
> give up my objection. Yes, a single string literal and a few insn bytes
> don't matter all that much. But nowadays everyone thinks this way
> everywhere, and one has to load dozens if not hundreds of megabytes
> of data just to get a basic OS booted.

Can we please do this sort of things systematically? Like, have a list
of criteria that state which when and where should we pay attention for
adding string literals. Arguing over this on a case by case basis is not
very productive.

BTW I don't think Xen is too bad in that regard -- it certainly doesn't
need hundreds of megabytes of data to boot.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> > On 08/11/18 15:33, Wei Liu wrote:
> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >>>> On 05/11/18 15:48, Wei Liu wrote:
> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >>>>>>>  }
> >>>>>>>  
> >>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
> >>>>>>> +
> >>>>>>> +#ifdef CONFIG_PV
> >>>>>>>  void lstar_enter(void);
> >>>>>>>  void cstar_enter(void);
> >>>>>>> +#else
> >>>>>>> +static inline void lstar_enter(void)
> >>>>>>> +{
> >>>>>>> +    panic("%s called", __func__);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +static inline void cstar_enter(void)
> >>>>>>> +{
> >>>>>>> +    panic("%s called", __func__);
> >>>>>>> +}
> >>>>>>> +#endif /* CONFIG_PV */
> >>>>>> Do we really need two separate stubs (and two separate string literals)
> >>>>>> here?
> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
> >>>>> change it to only one function.
> >>>> This is the correct way to do it.  __func__ will already be in the
> >>>> string table, and the format string (being identical) will be merged.
> >>> Why would __func__ be in the string table already, for functions
> >>> containing no other references to it?
> >> What is the way forward? Do we really care if there is one more string
> >> literal in the binary?
> > 
> > No.  One extra string like this is not something which needs caring
> > about in the slightest.
> 
> So just to clarify - the unnecessary redundancy in the source is of more
> relevance to me than the resulting slightly larger binary.

So you would rather have

    static inline void lcstar_enter(void)
    {
        panic("lstar/cstar\n");
    }

?

TBH I think this is not as good as using two functions. I'm up for
sending a followup patch to do whatever it is agreed upon, but for now
I'm intending to commit this patch as-is.

Wei.

> 
> Jan
> 
> 

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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Jan Beulich]

>>> On 08.11.18 at 17:09, <wei.liu2@citrix.com> wrote:
> On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
>> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
>> > On 08/11/18 15:33, Wei Liu wrote:
>> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
>> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
>> >>>> On 05/11/18 15:48, Wei Liu wrote:
>> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
>> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
>> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
>> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
>> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
>> >>>>>>>  }
>> >>>>>>>  
>> >>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
>> >>>>>>> +
>> >>>>>>> +#ifdef CONFIG_PV
>> >>>>>>>  void lstar_enter(void);
>> >>>>>>>  void cstar_enter(void);
>> >>>>>>> +#else
>> >>>>>>> +static inline void lstar_enter(void)
>> >>>>>>> +{
>> >>>>>>> +    panic("%s called", __func__);
>> >>>>>>> +}
>> >>>>>>> +
>> >>>>>>> +static inline void cstar_enter(void)
>> >>>>>>> +{
>> >>>>>>> +    panic("%s called", __func__);
>> >>>>>>> +}
>> >>>>>>> +#endif /* CONFIG_PV */
>> >>>>>> Do we really need two separate stubs (and two separate string literals)
>> >>>>>> here?
>> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
>> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
>> >>>>> change it to only one function.
>> >>>> This is the correct way to do it.  __func__ will already be in the
>> >>>> string table, and the format string (being identical) will be merged.
>> >>> Why would __func__ be in the string table already, for functions
>> >>> containing no other references to it?
>> >> What is the way forward? Do we really care if there is one more string
>> >> literal in the binary?
>> > 
>> > No.  One extra string like this is not something which needs caring
>> > about in the slightest.
>> 
>> So just to clarify - the unnecessary redundancy in the source is of more
>> relevance to me than the resulting slightly larger binary.
> 
> So you would rather have
> 
>     static inline void lcstar_enter(void)
>     {
>         panic("lstar/cstar\n");
>     }
> 
> ?

Yes. Let's not forget that this function is never to be invoked
anyway, so the less dead code and data, the better. (As a
side note, I don't understand why they're inline when you need
a pointer to them anyway. Instead they should go into
.text.cold or whatever it is.)

Jan



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

Re: [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu]

On Thu, Nov 08, 2018 at 09:22:08AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 17:09, <wei.liu2@citrix.com> wrote:
> > On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
> >> >>> On 08.11.18 at 16:36, <andrew.cooper3@citrix.com> wrote:
> >> > On 08/11/18 15:33, Wei Liu wrote:
> >> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@citrix.com> wrote:
> >> >>>> On 05/11/18 15:48, Wei Liu wrote:
> >> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@citrix.com> wrote:
> >> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >> >>>>>>>  }
> >> >>>>>>>  
> >> >>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
> >> >>>>>>> +
> >> >>>>>>> +#ifdef CONFIG_PV
> >> >>>>>>>  void lstar_enter(void);
> >> >>>>>>>  void cstar_enter(void);
> >> >>>>>>> +#else
> >> >>>>>>> +static inline void lstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> +    panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +
> >> >>>>>>> +static inline void cstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> +    panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +#endif /* CONFIG_PV */
> >> >>>>>> Do we really need two separate stubs (and two separate string literals)
> >> >>>>>> here?
> >> >>>>> I think it is clearer if we have two distinct messages. But I'm not too
> >> >>>>> fussed either way really. If you feel strongly about this, I'm happy to
> >> >>>>> change it to only one function.
> >> >>>> This is the correct way to do it.  __func__ will already be in the
> >> >>>> string table, and the format string (being identical) will be merged.
> >> >>> Why would __func__ be in the string table already, for functions
> >> >>> containing no other references to it?
> >> >> What is the way forward? Do we really care if there is one more string
> >> >> literal in the binary?
> >> > 
> >> > No.  One extra string like this is not something which needs caring
> >> > about in the slightest.
> >> 
> >> So just to clarify - the unnecessary redundancy in the source is of more
> >> relevance to me than the resulting slightly larger binary.
> > 
> > So you would rather have
> > 
> >     static inline void lcstar_enter(void)
> >     {
> >         panic("lstar/cstar\n");
> >     }
> > 
> > ?
> 
> Yes. Let's not forget that this function is never to be invoked
> anyway, so the less dead code and data, the better. (As a
> side note, I don't understand why they're inline when you need
> a pointer to them anyway. Instead they should go into
> .text.cold or whatever it is.)

OK. I will submit a followup patch for that.

Wei.

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