[PATCH 0/5] x86: asm-offsets.h and !PV32 adjustments

[PATCH 0/5] x86: asm-offsets.h and !PV32 adjustments

[Jan Beulich]

The 2nd and 3rd patches here effectively called for the latter
two to also be created, hence why they all live together.

1: build: limit rebuilding of asm-offsets.s
2: build: limit #include-ing by asm-offsets.h
3: build: restrict contents of asm-offsets.h when !HVM / !PV
4: hypercall vector is unused when !PV32
5: don't build unused entry code when !PV32

Jan

[PATCH 1/5] x86/build: limit rebuilding of asm-offsets.h

[Jan Beulich]

This file has a long dependencies list (through asm-offsets.s) and a
long list of dependents. IOW if any of the former changes, all of the
latter will be rebuilt, even if there's no actual change to the
generated file. This is the primary scenario we have the move-if-changed
macro for.

Since debug information may easily cause the file contents to change in
benign ways, also avoid emitting this into the output file.

Finally already before this change *.new files needed including in what
gets removed by the "clean" target.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Perhaps Arm would want doing the same. In fact perhaps the rules should
be unified by moving to common code?

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -235,7 +235,8 @@ efi/buildid.o efi/relocs-dummy.o: $(BASE
 efi/buildid.o efi/relocs-dummy.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h
-	$(CC) $(filter-out -Wa$(comma)% -flto,$(c_flags)) -S -o $@ $<
+	$(CC) $(filter-out -Wa$(comma)% -flto,$(c_flags)) -S -g0 -o $@.new $<
+	$(call move-if-changed,$@.new,$@)
 
 asm-macros.i: CFLAGS-y += -D__ASSEMBLY__ -P
 
@@ -262,7 +263,7 @@ efi/mkreloc: efi/mkreloc.c
 
 .PHONY: clean
 clean::
-	rm -f asm-offsets.s *.lds boot/*.o boot/*~ boot/core boot/mkelf32
+	rm -f asm-offsets.s *.lds *.new boot/*.o boot/*~ boot/core boot/mkelf32
 	rm -f asm-macros.i $(BASEDIR)/include/asm-x86/asm-macros.*
 	rm -f $(BASEDIR)/.xen-syms.[0-9]* boot/.*.d $(BASEDIR)/.xen.elf32
 	rm -f $(BASEDIR)/.xen.efi.[0-9]* efi/*.efi efi/mkreloc

Re: [PATCH 1/5] x86/build: limit rebuilding of asm-offsets.h

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:45:56AM +0100, Jan Beulich wrote:
> This file has a long dependencies list (through asm-offsets.s) and a
> long list of dependents. IOW if any of the former changes, all of the
> latter will be rebuilt, even if there's no actual change to the
> generated file. This is the primary scenario we have the move-if-changed
> macro for.
> 
> Since debug information may easily cause the file contents to change in
> benign ways, also avoid emitting this into the output file.
> 
> Finally already before this change *.new files needed including in what
> gets removed by the "clean" target.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> ---
> Perhaps Arm would want doing the same. In fact perhaps the rules should
> be unified by moving to common code?

Having the rule in common code would be my preference, the
prerequisites are slightly different, but I think we can sort this
out?

Thanks, Roger.

Re: [PATCH 1/5] x86/build: limit rebuilding of asm-offsets.h

[Jan Beulich]

On 28.12.2020 13:00, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:45:56AM +0100, Jan Beulich wrote:
>> This file has a long dependencies list (through asm-offsets.s) and a
>> long list of dependents. IOW if any of the former changes, all of the
>> latter will be rebuilt, even if there's no actual change to the
>> generated file. This is the primary scenario we have the move-if-changed
>> macro for.
>>
>> Since debug information may easily cause the file contents to change in
>> benign ways, also avoid emitting this into the output file.
>>
>> Finally already before this change *.new files needed including in what
>> gets removed by the "clean" target.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Perhaps Arm would want doing the same. In fact perhaps the rules should
>> be unified by moving to common code?
> 
> Having the rule in common code would be my preference, the
> prerequisites are slightly different, but I think we can sort this
> out?

Well, that's the nice thing about make rules: Dependencies / prereqs
and the actual rule can be specified independently. I.e. I'd envision
per-arch dependency specifications and a common rule (with common
dependencies of course living there as well).

Jan

[PATCH 2/5] x86/build: limit #include-ing by asm-offsets.c

[Jan Beulich]

This file has a long dependencies list and asm-offsets.h, generated from
it, has a long list of dependents. IOW if any of the former changes, all
of the latter will be rebuilt, even if there's no actual change to the
generated file. Therefore avoid including headers we don't actually need
(generally or configuration dependent).

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

--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -5,11 +5,13 @@
  */
 #define COMPILE_OFFSETS
 
+#ifdef CONFIG_PERF_COUNTERS
 #include <xen/perfc.h>
+#endif
 #include <xen/sched.h>
-#include <xen/bitops.h>
+#ifdef CONFIG_PV
 #include <compat/xen.h>
-#include <asm/fixmap.h>
+#endif
 #include <asm/hardirq.h>
 #include <xen/multiboot.h>
 #include <xen/multiboot2.h>
@@ -101,7 +103,6 @@ void __dummy__(void)
 #ifdef CONFIG_PV
     OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
     BLANK();
-#endif
 
     OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
     OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
@@ -110,6 +111,7 @@ void __dummy__(void)
     OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
     OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
     BLANK();
+#endif
 
     OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, guest_cpu_user_regs);
     OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);

Re: [PATCH 2/5] x86/build: limit #include-ing by asm-offsets.c

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:49:21AM +0100, Jan Beulich wrote:
> This file has a long dependencies list and asm-offsets.h, generated from
> it, has a long list of dependents. IOW if any of the former changes, all
> of the latter will be rebuilt, even if there's no actual change to the
> generated file. Therefore avoid including headers we don't actually need
> (generally or configuration dependent).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -5,11 +5,13 @@
>   */
>  #define COMPILE_OFFSETS
>  
> +#ifdef CONFIG_PERF_COUNTERS
>  #include <xen/perfc.h>
> +#endif
>  #include <xen/sched.h>
> -#include <xen/bitops.h>
> +#ifdef CONFIG_PV
>  #include <compat/xen.h>
> -#include <asm/fixmap.h>
> +#endif
>  #include <asm/hardirq.h>
>  #include <xen/multiboot.h>
>  #include <xen/multiboot2.h>
> @@ -101,7 +103,6 @@ void __dummy__(void)
>  #ifdef CONFIG_PV
>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>      BLANK();
> -#endif
>  
>      OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
>      OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
> @@ -110,6 +111,7 @@ void __dummy__(void)
>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
>      BLANK();
> +#endif

Since you are playing with this, the TRAPINFO/TRAPBOUNCE also seem
like ones to gate with CONFIG_PV. And the VCPU_svm/vmx could be gated
on CONFIG_HVM AFAICT?

Thanks, Roger.

Re: [PATCH 2/5] x86/build: limit #include-ing by asm-offsets.c

[Jan Beulich]

On 28.12.2020 13:54, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:49:21AM +0100, Jan Beulich wrote:
>> This file has a long dependencies list and asm-offsets.h, generated from
>> it, has a long list of dependents. IOW if any of the former changes, all
>> of the latter will be rebuilt, even if there's no actual change to the
>> generated file. Therefore avoid including headers we don't actually need
>> (generally or configuration dependent).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>> @@ -5,11 +5,13 @@
>>   */
>>  #define COMPILE_OFFSETS
>>  
>> +#ifdef CONFIG_PERF_COUNTERS
>>  #include <xen/perfc.h>
>> +#endif
>>  #include <xen/sched.h>
>> -#include <xen/bitops.h>
>> +#ifdef CONFIG_PV
>>  #include <compat/xen.h>
>> -#include <asm/fixmap.h>
>> +#endif
>>  #include <asm/hardirq.h>
>>  #include <xen/multiboot.h>
>>  #include <xen/multiboot2.h>
>> @@ -101,7 +103,6 @@ void __dummy__(void)
>>  #ifdef CONFIG_PV
>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>>      BLANK();
>> -#endif
>>  
>>      OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
>>      OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
>> @@ -110,6 +111,7 @@ void __dummy__(void)
>>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
>>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
>>      BLANK();
>> +#endif
> 
> Since you are playing with this, the TRAPINFO/TRAPBOUNCE also seem
> like ones to gate with CONFIG_PV. And the VCPU_svm/vmx could be gated
> on CONFIG_HVM AFAICT?

Maybe. I didn't want to make too big a first step.

Jan

[PATCH 3/5] x86/build: restrict contents of asm-offsets.h when !HVM / !PV

[Jan Beulich]

This file has a long dependencies list (through asm-offsets.[cs]) and a
long list of dependents. IOW if any of the former changes, all of the
latter will be rebuilt, even if there's no actual change to the
generated file. Therefore avoid producing symbols we don't actually
need, depending on configuration.

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

--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -84,6 +84,7 @@ void __dummy__(void)
     DEFINE(_VGCF_syscall_disables_events,  _VGCF_syscall_disables_events);
     BLANK();
 
+#ifdef CONFIG_HVM
     OFFSET(VCPU_svm_vmcb_pa, struct vcpu, arch.hvm.svm.vmcb_pa);
     OFFSET(VCPU_svm_vmcb, struct vcpu, arch.hvm.svm.vmcb);
     BLANK();
@@ -99,6 +100,7 @@ void __dummy__(void)
     OFFSET(VCPU_nhvm_p2m, struct vcpu, arch.hvm.nvcpu.nv_p2m);
     OFFSET(VCPU_nsvm_hap_enabled, struct vcpu, arch.hvm.nvcpu.u.nsvm.ns_hap_enabled);
     BLANK();
+#endif
 
 #ifdef CONFIG_PV
     OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
@@ -128,6 +130,7 @@ void __dummy__(void)
     DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
     BLANK();
 
+#ifdef CONFIG_PV
     OFFSET(TRAPINFO_eip, struct trap_info, address);
     OFFSET(TRAPINFO_cs, struct trap_info, cs);
     OFFSET(TRAPINFO_flags, struct trap_info, flags);
@@ -139,6 +142,7 @@ void __dummy__(void)
     OFFSET(TRAPBOUNCE_cs, struct trap_bounce, cs);
     OFFSET(TRAPBOUNCE_eip, struct trap_bounce, eip);
     BLANK();
+#endif
 
     OFFSET(VCPUMSR_spec_ctrl_raw, struct vcpu_msrs, spec_ctrl.raw);
     BLANK();

Re: [PATCH 3/5] x86/build: restrict contents of asm-offsets.h when !HVM / !PV

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:49:54AM +0100, Jan Beulich wrote:
> This file has a long dependencies list (through asm-offsets.[cs]) and a
> long list of dependents. IOW if any of the former changes, all of the
> latter will be rebuilt, even if there's no actual change to the
> generated file. Therefore avoid producing symbols we don't actually
> need, depending on configuration.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I think that replies my question on the previous patch, hence you can
add:

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

To both.

Thanks, Roger.

[PATCH 4/5] x86: hypercall vector is unused when !PV32

[Jan Beulich]

This vector can be used as an ordinary interrupt handling one in this
case. To be sure no references are left, make the #define itself
conditional.

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

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -436,8 +436,12 @@ int __init init_irq_data(void)
         irq_to_desc(irq)->irq = irq;
 
 #ifdef CONFIG_PV
-    /* Never allocate the hypercall vector or Linux/BSD fast-trap vector. */
+    /* Never allocate the Linux/BSD fast-trap vector. */
     set_bit(LEGACY_SYSCALL_VECTOR, used_vectors);
+#endif
+
+#ifdef CONFIG_PV32
+    /* Never allocate the hypercall vector. */
     set_bit(HYPERCALL_VECTOR, used_vectors);
 #endif
     
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -30,6 +30,7 @@
 #include <asm/traps.h>
 #include <irq_vectors.h>
 
+#ifdef CONFIG_PV32
 void do_entry_int82(struct cpu_user_regs *regs)
 {
     if ( unlikely(untrusted_msi) )
@@ -37,6 +38,7 @@ void do_entry_int82(struct cpu_user_regs
 
     pv_hypercall(regs);
 }
+#endif
 
 void pv_inject_event(const struct x86_event *event)
 {
@@ -155,9 +157,11 @@ static void nmi_softirq(void)
 
 void __init pv_trap_init(void)
 {
+#ifdef CONFIG_PV32
     /* The 32-on-64 hypercall vector is only accessible from ring 1. */
     _set_gate(idt_table + HYPERCALL_VECTOR,
               SYS_DESC_trap_gate, 1, entry_int82);
+#endif
 
     /* Fast trap for int80 (faster than taking the #GP-fixup path). */
     _set_gate(idt_table + LEGACY_SYSCALL_VECTOR, SYS_DESC_trap_gate, 3,
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -11,6 +11,8 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+#ifdef CONFIG_PV32
+
 ENTRY(entry_int82)
         ASM_CLAC
         pushq $0
@@ -27,6 +29,8 @@ ENTRY(entry_int82)
         mov   %rsp, %rdi
         call  do_entry_int82
 
+#endif /* CONFIG_PV32 */
+
 /* %rbx: struct vcpu */
 ENTRY(compat_test_all_events)
         ASSERT_NOT_IN_ATOMIC
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -982,8 +982,10 @@ autogen_stubs: /* Automatically generate
         .rept X86_NR_VECTORS
 
         /* Common interrupts, heading towards do_IRQ(). */
-#ifdef CONFIG_PV
+#if defined(CONFIG_PV32)
         .if vec >= FIRST_IRQ_VECTOR && vec != HYPERCALL_VECTOR && vec != LEGACY_SYSCALL_VECTOR
+#elif defined(CONFIG_PV)
+        .if vec >= FIRST_IRQ_VECTOR && vec != LEGACY_SYSCALL_VECTOR
 #else
         .if vec >= FIRST_IRQ_VECTOR
 #endif
--- a/xen/include/asm-x86/mach-default/irq_vectors.h
+++ b/xen/include/asm-x86/mach-default/irq_vectors.h
@@ -22,7 +22,10 @@
 #define FIRST_LEGACY_VECTOR     FIRST_DYNAMIC_VECTOR
 #define LAST_LEGACY_VECTOR      (FIRST_LEGACY_VECTOR + 0xf)
 
-#define HYPERCALL_VECTOR	0x82
+#ifdef CONFIG_PV32
+#define HYPERCALL_VECTOR        0x82
+#endif
+
 #define LEGACY_SYSCALL_VECTOR   0x80
 
 /*

Re: [PATCH 4/5] x86: hypercall vector is unused when !PV32

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:50:51AM +0100, Jan Beulich wrote:
> This vector can be used as an ordinary interrupt handling one in this
> case. To be sure no references are left, make the #define itself
> conditional.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Thanks, Roger.

[PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

Except for the initial part of cstar_enter compat/entry.S is all dead
code in this case. Further, along the lines of the PV conditionals we
already have in entry.S, make code PV32-conditional there too (to a
fair part because this code actually references compat/entry.S).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: I'm on the fence of whether (in a separate patch) to also make
     conditional struct pv_domain's is_32bit field.

--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -9,7 +9,7 @@
 #include <xen/perfc.h>
 #endif
 #include <xen/sched.h>
-#ifdef CONFIG_PV
+#ifdef CONFIG_PV32
 #include <compat/xen.h>
 #endif
 #include <asm/hardirq.h>
@@ -102,19 +102,21 @@ void __dummy__(void)
     BLANK();
 #endif
 
-#ifdef CONFIG_PV
+#ifdef CONFIG_PV32
     OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
     BLANK();
 
-    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
-    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
-    BLANK();
-
     OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
     OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
     BLANK();
 #endif
 
+#ifdef CONFIG_PV
+    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
+    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
+    BLANK();
+#endif
+
     OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, guest_cpu_user_regs);
     OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);
     OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -29,8 +29,6 @@ ENTRY(entry_int82)
         mov   %rsp, %rdi
         call  do_entry_int82
 
-#endif /* CONFIG_PV32 */
-
 /* %rbx: struct vcpu */
 ENTRY(compat_test_all_events)
         ASSERT_NOT_IN_ATOMIC
@@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
         xor   %eax, %eax
         ret
 
+#endif /* CONFIG_PV32 */
+
         .section .text.entry, "ax", @progbits
 
 /* See lstar_enter for entry register state. */
@@ -230,6 +230,13 @@ ENTRY(cstar_enter)
         sti
 
         movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
+
+#ifndef CONFIG_PV32
+
+        jmp   switch_to_kernel
+
+#else
+
         movq  VCPU_domain(%rbx),%rcx
         cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
         je    switch_to_kernel
@@ -393,3 +400,5 @@ compat_crash_page_fault:
         jmp   .Lft14
 .previous
         _ASM_EXTABLE(.Lft14, .Lfx14)
+
+#endif /* CONFIG_PV32 */
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -328,8 +328,10 @@ UNLIKELY_END(sysenter_gpf)
         movq  VCPU_domain(%rbx),%rdi
         movq  %rax,TRAPBOUNCE_eip(%rdx)
         movb  %cl,TRAPBOUNCE_flags(%rdx)
+#ifdef CONFIG_PV32
         cmpb  $0, DOMAIN_is_32bit_pv(%rdi)
         jne   compat_sysenter
+#endif
         jmp   .Lbounce_exception
 
 ENTRY(int80_direct_trap)
@@ -370,6 +372,7 @@ UNLIKELY_END(msi_check)
         mov    0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
         movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
 
+#ifdef CONFIG_PV32
         mov   %ecx, %edx
         and   $~3, %edx
 
@@ -378,6 +381,10 @@ UNLIKELY_END(msi_check)
 
         test  %rdx, %rdx
         jz    int80_slow_path
+#else
+        test  %rdi, %rdi
+        jz    int80_slow_path
+#endif
 
         /* Construct trap_bounce from trap_ctxt[0x80]. */
         lea   VCPU_trap_bounce(%rbx), %rdx
@@ -390,8 +397,10 @@ UNLIKELY_END(msi_check)
         lea   (, %rcx, TBF_INTERRUPT), %ecx
         mov   %cl, TRAPBOUNCE_flags(%rdx)
 
+#ifdef CONFIG_PV32
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         jne   compat_int80_direct_trap
+#endif
 
         call  create_bounce_frame
         jmp   test_all_events
@@ -541,12 +550,16 @@ ENTRY(dom_crash_sync_extable)
         GET_STACK_END(ax)
         leaq  STACK_CPUINFO_FIELD(guest_cpu_user_regs)(%rax),%rsp
         # create_bounce_frame() temporarily clobbers CS.RPL. Fix up.
+#ifdef CONFIG_PV32
         movq  STACK_CPUINFO_FIELD(current_vcpu)(%rax), %rax
         movq  VCPU_domain(%rax),%rax
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         sete  %al
         leal  (%rax,%rax,2),%eax
         orb   %al,UREGS_cs(%rsp)
+#else
+        orb   $3, UREGS_cs(%rsp)
+#endif
         xorl  %edi,%edi
         jmp   asm_domain_crash_synchronous /* Does not return */
         .popsection
@@ -562,11 +575,15 @@ ENTRY(ret_from_intr)
         GET_CURRENT(bx)
         testb $3, UREGS_cs(%rsp)
         jz    restore_all_xen
+#ifdef CONFIG_PV32
         movq  VCPU_domain(%rbx), %rax
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         je    test_all_events
         jmp   compat_test_all_events
 #else
+        jmp   test_all_events
+#endif
+#else
         ASSERT_CONTEXT_IS_XEN
         jmp   restore_all_xen
 #endif
@@ -652,7 +669,7 @@ handle_exception_saved:
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
 
-#ifdef CONFIG_PV
+#if defined(CONFIG_PV32)
         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
@@ -692,7 +709,7 @@ handle_exception_saved:
         test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
         jz    compat_test_all_events
 .Lcr4_pv32_done:
-#else
+#elif !defined(CONFIG_PV)
         ASSERT_CONTEXT_IS_XEN
 #endif /* CONFIG_PV */
         sti
@@ -711,9 +728,11 @@ handle_exception_saved:
 #ifdef CONFIG_PV
         testb $3,UREGS_cs(%rsp)
         jz    restore_all_xen
+#ifdef CONFIG_PV32
         movq  VCPU_domain(%rbx),%rax
         cmpb  $0, DOMAIN_is_32bit_pv(%rax)
         jne   compat_test_all_events
+#endif
         jmp   test_all_events
 #else
         ASSERT_CONTEXT_IS_XEN
@@ -947,11 +966,16 @@ handle_ist_exception:
         je    1f
         movl  $EVENT_CHECK_VECTOR,%edi
         call  send_IPI_self
-1:      movq  VCPU_domain(%rbx),%rax
+1:
+#ifdef CONFIG_PV32
+        movq  VCPU_domain(%rbx),%rax
         cmpb  $0,DOMAIN_is_32bit_pv(%rax)
         je    restore_all_guest
         jmp   compat_restore_all_guest
 #else
+        jmp   restore_all_guest
+#endif
+#else
         ASSERT_CONTEXT_IS_XEN
         jmp   restore_all_xen
 #endif
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -333,7 +333,7 @@ static always_inline void stac(void)
         subq  $-(UREGS_error_code-UREGS_r15+\adj), %rsp
 .endm
 
-#ifdef CONFIG_PV
+#ifdef CONFIG_PV32
 #define CR4_PV32_RESTORE                               \
     ALTERNATIVE_2 "",                                  \
         "call cr4_pv32_restore", X86_FEATURE_XEN_SMEP, \

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
> Except for the initial part of cstar_enter compat/entry.S is all dead
> code in this case. Further, along the lines of the PV conditionals we
> already have in entry.S, make code PV32-conditional there too (to a
> fair part because this code actually references compat/entry.S).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: I'm on the fence of whether (in a separate patch) to also make
>      conditional struct pv_domain's is_32bit field.
> 
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -9,7 +9,7 @@
>  #include <xen/perfc.h>
>  #endif
>  #include <xen/sched.h>
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>  #include <compat/xen.h>
>  #endif
>  #include <asm/hardirq.h>
> @@ -102,19 +102,21 @@ void __dummy__(void)
>      BLANK();
>  #endif
>  
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>      BLANK();
>  
> -    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
> -    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
> -    BLANK();
> -
>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
>      BLANK();
>  #endif
>  
> +#ifdef CONFIG_PV
> +    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
> +    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
> +    BLANK();
> +#endif
> +
>      OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, guest_cpu_user_regs);
>      OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);
>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -29,8 +29,6 @@ ENTRY(entry_int82)
>          mov   %rsp, %rdi
>          call  do_entry_int82
>  
> -#endif /* CONFIG_PV32 */
> -
>  /* %rbx: struct vcpu */
>  ENTRY(compat_test_all_events)
>          ASSERT_NOT_IN_ATOMIC
> @@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
>          xor   %eax, %eax
>          ret
>  
> +#endif /* CONFIG_PV32 */

I've also wondered, it feels weird to add CONFIG_PV32 gates to the
compat entry.S, since that's supposed to be only used when there's
support for 32bit PV guests?

Wouldn't this file only get built when such support is enabled?

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -328,8 +328,10 @@ UNLIKELY_END(sysenter_gpf)
>          movq  VCPU_domain(%rbx),%rdi
>          movq  %rax,TRAPBOUNCE_eip(%rdx)
>          movb  %cl,TRAPBOUNCE_flags(%rdx)
> +#ifdef CONFIG_PV32
>          cmpb  $0, DOMAIN_is_32bit_pv(%rdi)
>          jne   compat_sysenter
> +#endif
>          jmp   .Lbounce_exception
>  
>  ENTRY(int80_direct_trap)
> @@ -370,6 +372,7 @@ UNLIKELY_END(msi_check)
>          mov    0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
>          movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
>  
> +#ifdef CONFIG_PV32
>          mov   %ecx, %edx
>          and   $~3, %edx
>  
> @@ -378,6 +381,10 @@ UNLIKELY_END(msi_check)
>  
>          test  %rdx, %rdx
>          jz    int80_slow_path
> +#else
> +        test  %rdi, %rdi
> +        jz    int80_slow_path
> +#endif
>  
>          /* Construct trap_bounce from trap_ctxt[0x80]. */
>          lea   VCPU_trap_bounce(%rbx), %rdx
> @@ -390,8 +397,10 @@ UNLIKELY_END(msi_check)
>          lea   (, %rcx, TBF_INTERRUPT), %ecx
>          mov   %cl, TRAPBOUNCE_flags(%rdx)
>  
> +#ifdef CONFIG_PV32
>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>          jne   compat_int80_direct_trap
> +#endif
>  
>          call  create_bounce_frame
>          jmp   test_all_events
> @@ -541,12 +550,16 @@ ENTRY(dom_crash_sync_extable)
>          GET_STACK_END(ax)
>          leaq  STACK_CPUINFO_FIELD(guest_cpu_user_regs)(%rax),%rsp
>          # create_bounce_frame() temporarily clobbers CS.RPL. Fix up.
> +#ifdef CONFIG_PV32
>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rax), %rax
>          movq  VCPU_domain(%rax),%rax
>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>          sete  %al
>          leal  (%rax,%rax,2),%eax
>          orb   %al,UREGS_cs(%rsp)
> +#else
> +        orb   $3, UREGS_cs(%rsp)
> +#endif
>          xorl  %edi,%edi
>          jmp   asm_domain_crash_synchronous /* Does not return */
>          .popsection
> @@ -562,11 +575,15 @@ ENTRY(ret_from_intr)
>          GET_CURRENT(bx)
>          testb $3, UREGS_cs(%rsp)
>          jz    restore_all_xen
> +#ifdef CONFIG_PV32
>          movq  VCPU_domain(%rbx), %rax
>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>          je    test_all_events
>          jmp   compat_test_all_events
>  #else
> +        jmp   test_all_events
> +#endif
> +#else
>          ASSERT_CONTEXT_IS_XEN
>          jmp   restore_all_xen
>  #endif
> @@ -652,7 +669,7 @@ handle_exception_saved:
>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>          jz    exception_with_ints_disabled
>  
> -#ifdef CONFIG_PV
> +#if defined(CONFIG_PV32)
>          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
> @@ -692,7 +709,7 @@ handle_exception_saved:
>          test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
>          jz    compat_test_all_events
>  .Lcr4_pv32_done:
> -#else
> +#elif !defined(CONFIG_PV)
>          ASSERT_CONTEXT_IS_XEN
>  #endif /* CONFIG_PV */
>          sti
> @@ -711,9 +728,11 @@ handle_exception_saved:
>  #ifdef CONFIG_PV
>          testb $3,UREGS_cs(%rsp)
>          jz    restore_all_xen
> +#ifdef CONFIG_PV32
>          movq  VCPU_domain(%rbx),%rax
>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>          jne   compat_test_all_events
> +#endif
>          jmp   test_all_events
>  #else
>          ASSERT_CONTEXT_IS_XEN
> @@ -947,11 +966,16 @@ handle_ist_exception:
>          je    1f
>          movl  $EVENT_CHECK_VECTOR,%edi
>          call  send_IPI_self
> -1:      movq  VCPU_domain(%rbx),%rax
> +1:
> +#ifdef CONFIG_PV32
> +        movq  VCPU_domain(%rbx),%rax
>          cmpb  $0,DOMAIN_is_32bit_pv(%rax)
>          je    restore_all_guest
>          jmp   compat_restore_all_guest
>  #else
> +        jmp   restore_all_guest
> +#endif
> +#else
>          ASSERT_CONTEXT_IS_XEN
>          jmp   restore_all_xen
>  #endif

I would like to have Andrew's opinion on this one (as you and him tend
to modify more asm code than myself). There are quite a lot of
addition to the assembly code, and IMO it makes the code more complex
which I think we should try to avoid, as assembly is already hard
enough.

Thanks, Roger.

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 28.12.2020 16:30, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -29,8 +29,6 @@ ENTRY(entry_int82)
>>          mov   %rsp, %rdi
>>          call  do_entry_int82
>>  
>> -#endif /* CONFIG_PV32 */
>> -
>>  /* %rbx: struct vcpu */
>>  ENTRY(compat_test_all_events)
>>          ASSERT_NOT_IN_ATOMIC
>> @@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
>>          xor   %eax, %eax
>>          ret
>>  
>> +#endif /* CONFIG_PV32 */
> 
> I've also wondered, it feels weird to add CONFIG_PV32 gates to the
> compat entry.S, since that's supposed to be only used when there's
> support for 32bit PV guests?
> 
> Wouldn't this file only get built when such support is enabled?

No. We need cstar_enter also for 64-bit guests' 32-bit
user space possibly making system calls via SYSCALL.

>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -328,8 +328,10 @@ UNLIKELY_END(sysenter_gpf)
>>          movq  VCPU_domain(%rbx),%rdi
>>          movq  %rax,TRAPBOUNCE_eip(%rdx)
>>          movb  %cl,TRAPBOUNCE_flags(%rdx)
>> +#ifdef CONFIG_PV32
>>          cmpb  $0, DOMAIN_is_32bit_pv(%rdi)
>>          jne   compat_sysenter
>> +#endif
>>          jmp   .Lbounce_exception
>>  
>>  ENTRY(int80_direct_trap)
>> @@ -370,6 +372,7 @@ UNLIKELY_END(msi_check)
>>          mov    0x80 * TRAPINFO_sizeof + TRAPINFO_eip(%rsi), %rdi
>>          movzwl 0x80 * TRAPINFO_sizeof + TRAPINFO_cs (%rsi), %ecx
>>  
>> +#ifdef CONFIG_PV32
>>          mov   %ecx, %edx
>>          and   $~3, %edx
>>  
>> @@ -378,6 +381,10 @@ UNLIKELY_END(msi_check)
>>  
>>          test  %rdx, %rdx
>>          jz    int80_slow_path
>> +#else
>> +        test  %rdi, %rdi
>> +        jz    int80_slow_path
>> +#endif
>>  
>>          /* Construct trap_bounce from trap_ctxt[0x80]. */
>>          lea   VCPU_trap_bounce(%rbx), %rdx
>> @@ -390,8 +397,10 @@ UNLIKELY_END(msi_check)
>>          lea   (, %rcx, TBF_INTERRUPT), %ecx
>>          mov   %cl, TRAPBOUNCE_flags(%rdx)
>>  
>> +#ifdef CONFIG_PV32
>>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>>          jne   compat_int80_direct_trap
>> +#endif
>>  
>>          call  create_bounce_frame
>>          jmp   test_all_events
>> @@ -541,12 +550,16 @@ ENTRY(dom_crash_sync_extable)
>>          GET_STACK_END(ax)
>>          leaq  STACK_CPUINFO_FIELD(guest_cpu_user_regs)(%rax),%rsp
>>          # create_bounce_frame() temporarily clobbers CS.RPL. Fix up.
>> +#ifdef CONFIG_PV32
>>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rax), %rax
>>          movq  VCPU_domain(%rax),%rax
>>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>>          sete  %al
>>          leal  (%rax,%rax,2),%eax
>>          orb   %al,UREGS_cs(%rsp)
>> +#else
>> +        orb   $3, UREGS_cs(%rsp)
>> +#endif
>>          xorl  %edi,%edi
>>          jmp   asm_domain_crash_synchronous /* Does not return */
>>          .popsection
>> @@ -562,11 +575,15 @@ ENTRY(ret_from_intr)
>>          GET_CURRENT(bx)
>>          testb $3, UREGS_cs(%rsp)
>>          jz    restore_all_xen
>> +#ifdef CONFIG_PV32
>>          movq  VCPU_domain(%rbx), %rax
>>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>>          je    test_all_events
>>          jmp   compat_test_all_events
>>  #else
>> +        jmp   test_all_events
>> +#endif
>> +#else
>>          ASSERT_CONTEXT_IS_XEN
>>          jmp   restore_all_xen
>>  #endif
>> @@ -652,7 +669,7 @@ handle_exception_saved:
>>          testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
>>          jz    exception_with_ints_disabled
>>  
>> -#ifdef CONFIG_PV
>> +#if defined(CONFIG_PV32)
>>          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
>> @@ -692,7 +709,7 @@ handle_exception_saved:
>>          test  $~(PFEC_write_access|PFEC_insn_fetch),%eax
>>          jz    compat_test_all_events
>>  .Lcr4_pv32_done:
>> -#else
>> +#elif !defined(CONFIG_PV)
>>          ASSERT_CONTEXT_IS_XEN
>>  #endif /* CONFIG_PV */
>>          sti
>> @@ -711,9 +728,11 @@ handle_exception_saved:
>>  #ifdef CONFIG_PV
>>          testb $3,UREGS_cs(%rsp)
>>          jz    restore_all_xen
>> +#ifdef CONFIG_PV32
>>          movq  VCPU_domain(%rbx),%rax
>>          cmpb  $0, DOMAIN_is_32bit_pv(%rax)
>>          jne   compat_test_all_events
>> +#endif
>>          jmp   test_all_events
>>  #else
>>          ASSERT_CONTEXT_IS_XEN
>> @@ -947,11 +966,16 @@ handle_ist_exception:
>>          je    1f
>>          movl  $EVENT_CHECK_VECTOR,%edi
>>          call  send_IPI_self
>> -1:      movq  VCPU_domain(%rbx),%rax
>> +1:
>> +#ifdef CONFIG_PV32
>> +        movq  VCPU_domain(%rbx),%rax
>>          cmpb  $0,DOMAIN_is_32bit_pv(%rax)
>>          je    restore_all_guest
>>          jmp   compat_restore_all_guest
>>  #else
>> +        jmp   restore_all_guest
>> +#endif
>> +#else
>>          ASSERT_CONTEXT_IS_XEN
>>          jmp   restore_all_xen
>>  #endif
> 
> I would like to have Andrew's opinion on this one (as you and him tend
> to modify more asm code than myself). There are quite a lot of
> addition to the assembly code, and IMO it makes the code more complex
> which I think we should try to avoid, as assembly is already hard
> enough.

Well, while I can see your point (and I indeed asked myself the same
question when making this change), this merely follows the route
started with the addition on CONFIG_PV conditionals. If we think that
prior step didn't set a good precedent, we ought to undo it.
Otherwise I see no good argument against doing the same kind of
transformation a 2nd time (and further ones, if need be down the
road).

Jan

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Roger Pau Monné]

On Mon, Jan 04, 2021 at 02:56:12PM +0100, Jan Beulich wrote:
> On 28.12.2020 16:30, Roger Pau Monné wrote:
> > I would like to have Andrew's opinion on this one (as you and him tend
> > to modify more asm code than myself). There are quite a lot of
> > addition to the assembly code, and IMO it makes the code more complex
> > which I think we should try to avoid, as assembly is already hard
> > enough.
> 
> Well, while I can see your point (and I indeed asked myself the same
> question when making this change), this merely follows the route
> started with the addition on CONFIG_PV conditionals. If we think that
> prior step didn't set a good precedent, we ought to undo it.
> Otherwise I see no good argument against doing the same kind of
> transformation a 2nd time (and further ones, if need be down the
> road).

I think we need to apply some common sense and reach consensus about
where it's fine to make code conditional at build time as to not make
the existing code much harder to read and reason about. This is mostly
a subjective decision, so I understand your concern.

I still think I would like Andrew opinion on this one, as said you and
him are the ones mostly doing assembly coding. I find it already hard
to follow myself without the conditionals.

Thanks, Roger.

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 04.01.2021 16:53, Roger Pau Monné wrote:
> On Mon, Jan 04, 2021 at 02:56:12PM +0100, Jan Beulich wrote:
>> On 28.12.2020 16:30, Roger Pau Monné wrote:
>>> I would like to have Andrew's opinion on this one (as you and him tend
>>> to modify more asm code than myself). There are quite a lot of
>>> addition to the assembly code, and IMO it makes the code more complex
>>> which I think we should try to avoid, as assembly is already hard
>>> enough.
>>
>> Well, while I can see your point (and I indeed asked myself the same
>> question when making this change), this merely follows the route
>> started with the addition on CONFIG_PV conditionals. If we think that
>> prior step didn't set a good precedent, we ought to undo it.
>> Otherwise I see no good argument against doing the same kind of
>> transformation a 2nd time (and further ones, if need be down the
>> road).
> 
> I think we need to apply some common sense and reach consensus about
> where it's fine to make code conditional at build time as to not make
> the existing code much harder to read and reason about. This is mostly
> a subjective decision, so I understand your concern.
> 
> I still think I would like Andrew opinion on this one, as said you and
> him are the ones mostly doing assembly coding. I find it already hard
> to follow myself without the conditionals.

Oh, sure - my prior response in no way meant to be an objection to
your request for Andrew's take on this.

Jan

Ping: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 04.01.2021 16:53, Roger Pau Monné wrote:
> On Mon, Jan 04, 2021 at 02:56:12PM +0100, Jan Beulich wrote:
>> On 28.12.2020 16:30, Roger Pau Monné wrote:
>>> I would like to have Andrew's opinion on this one (as you and him tend
>>> to modify more asm code than myself). There are quite a lot of
>>> addition to the assembly code, and IMO it makes the code more complex
>>> which I think we should try to avoid, as assembly is already hard
>>> enough.
>>
>> Well, while I can see your point (and I indeed asked myself the same
>> question when making this change), this merely follows the route
>> started with the addition on CONFIG_PV conditionals. If we think that
>> prior step didn't set a good precedent, we ought to undo it.
>> Otherwise I see no good argument against doing the same kind of
>> transformation a 2nd time (and further ones, if need be down the
>> road).
> 
> I think we need to apply some common sense and reach consensus about
> where it's fine to make code conditional at build time as to not make
> the existing code much harder to read and reason about. This is mostly
> a subjective decision, so I understand your concern.
> 
> I still think I would like Andrew opinion on this one, as said you and
> him are the ones mostly doing assembly coding. I find it already hard
> to follow myself without the conditionals.

Seeing no feedback for about 3 months, I intend to commit this once
the tree is fully open again (if need be without any acks), unless
I hear otherwise by then.

Jan

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Roger Pau Monné]

On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
> Except for the initial part of cstar_enter compat/entry.S is all dead
> code in this case. Further, along the lines of the PV conditionals we
> already have in entry.S, make code PV32-conditional there too (to a
> fair part because this code actually references compat/entry.S).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: I'm on the fence of whether (in a separate patch) to also make
>      conditional struct pv_domain's is_32bit field.
> 
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -9,7 +9,7 @@
>  #include <xen/perfc.h>
>  #endif
>  #include <xen/sched.h>
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>  #include <compat/xen.h>
>  #endif
>  #include <asm/hardirq.h>
> @@ -102,19 +102,21 @@ void __dummy__(void)
>      BLANK();
>  #endif
>  
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>      BLANK();
>  
> -    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
> -    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
> -    BLANK();
> -
>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
>      BLANK();
>  #endif
>  
> +#ifdef CONFIG_PV
> +    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
> +    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
> +    BLANK();
> +#endif
> +
>      OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, guest_cpu_user_regs);
>      OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);
>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -29,8 +29,6 @@ ENTRY(entry_int82)
>          mov   %rsp, %rdi
>          call  do_entry_int82
>  
> -#endif /* CONFIG_PV32 */
> -
>  /* %rbx: struct vcpu */
>  ENTRY(compat_test_all_events)
>          ASSERT_NOT_IN_ATOMIC
> @@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
>          xor   %eax, %eax
>          ret
>  
> +#endif /* CONFIG_PV32 */
> +
>          .section .text.entry, "ax", @progbits
>  
>  /* See lstar_enter for entry register state. */
> @@ -230,6 +230,13 @@ ENTRY(cstar_enter)
>          sti
>  
>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
> +
> +#ifndef CONFIG_PV32
> +
> +        jmp   switch_to_kernel
> +
> +#else
> +
>          movq  VCPU_domain(%rbx),%rcx
>          cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
>          je    switch_to_kernel
> @@ -393,3 +400,5 @@ compat_crash_page_fault:
>          jmp   .Lft14
>  .previous
>          _ASM_EXTABLE(.Lft14, .Lfx14)
> +
> +#endif /* CONFIG_PV32 */

Seeing this chunk, would it make sense to move the cstar_enter part
relevant to !is_32bit_pv into the common entry.S and leave the rest
here as compat_cstar_enter or some such?

AFAICT we only need a tiny part of the compat stuff when !CONFIG_PV32,
so I think we could make the hole compat/entry.S depend on
CONFIG_PV32.

Thanks, Roger.

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 01.04.2021 16:01, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
>> @@ -230,6 +230,13 @@ ENTRY(cstar_enter)
>>          sti
>>  
>>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
>> +
>> +#ifndef CONFIG_PV32
>> +
>> +        jmp   switch_to_kernel
>> +
>> +#else
>> +
>>          movq  VCPU_domain(%rbx),%rcx
>>          cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
>>          je    switch_to_kernel
>> @@ -393,3 +400,5 @@ compat_crash_page_fault:
>>          jmp   .Lft14
>>  .previous
>>          _ASM_EXTABLE(.Lft14, .Lfx14)
>> +
>> +#endif /* CONFIG_PV32 */
> 
> Seeing this chunk, would it make sense to move the cstar_enter part
> relevant to !is_32bit_pv into the common entry.S and leave the rest
> here as compat_cstar_enter or some such?
> 
> AFAICT we only need a tiny part of the compat stuff when !CONFIG_PV32,
> so I think we could make the hole compat/entry.S depend on
> CONFIG_PV32.

To be honest I was meaning to see whether this would work out reasonably
well in a separate, follow-on change, as the code movement might make it
harder to judge on the change here compared to the #ifdef insertions.
But maybe I was wrong and should give this a try right away.

Jan

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 01.04.2021 16:01, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
>> Except for the initial part of cstar_enter compat/entry.S is all dead
>> code in this case. Further, along the lines of the PV conditionals we
>> already have in entry.S, make code PV32-conditional there too (to a
>> fair part because this code actually references compat/entry.S).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: I'm on the fence of whether (in a separate patch) to also make
>>      conditional struct pv_domain's is_32bit field.
>>
>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>> @@ -9,7 +9,7 @@
>>  #include <xen/perfc.h>
>>  #endif
>>  #include <xen/sched.h>
>> -#ifdef CONFIG_PV
>> +#ifdef CONFIG_PV32
>>  #include <compat/xen.h>
>>  #endif
>>  #include <asm/hardirq.h>
>> @@ -102,19 +102,21 @@ void __dummy__(void)
>>      BLANK();
>>  #endif
>>  
>> -#ifdef CONFIG_PV
>> +#ifdef CONFIG_PV32
>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>>      BLANK();
>>  
>> -    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
>> -    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
>> -    BLANK();
>> -
>>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, evtchn_upcall_pending);
>>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, evtchn_upcall_mask);
>>      BLANK();
>>  #endif
>>  
>> +#ifdef CONFIG_PV
>> +    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, evtchn_upcall_pending);
>> +    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
>> +    BLANK();
>> +#endif
>> +
>>      OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, guest_cpu_user_regs);
>>      OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);
>>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -29,8 +29,6 @@ ENTRY(entry_int82)
>>          mov   %rsp, %rdi
>>          call  do_entry_int82
>>  
>> -#endif /* CONFIG_PV32 */
>> -
>>  /* %rbx: struct vcpu */
>>  ENTRY(compat_test_all_events)
>>          ASSERT_NOT_IN_ATOMIC
>> @@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
>>          xor   %eax, %eax
>>          ret
>>  
>> +#endif /* CONFIG_PV32 */
>> +
>>          .section .text.entry, "ax", @progbits
>>  
>>  /* See lstar_enter for entry register state. */
>> @@ -230,6 +230,13 @@ ENTRY(cstar_enter)
>>          sti
>>  
>>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
>> +
>> +#ifndef CONFIG_PV32
>> +
>> +        jmp   switch_to_kernel
>> +
>> +#else
>> +
>>          movq  VCPU_domain(%rbx),%rcx
>>          cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
>>          je    switch_to_kernel
>> @@ -393,3 +400,5 @@ compat_crash_page_fault:
>>          jmp   .Lft14
>>  .previous
>>          _ASM_EXTABLE(.Lft14, .Lfx14)
>> +
>> +#endif /* CONFIG_PV32 */
> 
> Seeing this chunk, would it make sense to move the cstar_enter part
> relevant to !is_32bit_pv into the common entry.S and leave the rest
> here as compat_cstar_enter or some such?
> 
> AFAICT we only need a tiny part of the compat stuff when !CONFIG_PV32,
> so I think we could make the hole compat/entry.S depend on
> CONFIG_PV32.

While it grows the patch, doing things this way looks to work out
nicely. v2 in the works (making in fact compat/ as a whole build
only when PV32, as it's really only the one object file that gets
built there) ...

Jan

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Andrew Cooper]

On 25/11/2020 08:51, Jan Beulich wrote:
> Except for the initial part of cstar_enter compat/entry.S is all dead
> code in this case. Further, along the lines of the PV conditionals we
> already have in entry.S, make code PV32-conditional there too (to a
> fair part because this code actually references compat/entry.S).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: I'm on the fence of whether (in a separate patch) to also make
>      conditional struct pv_domain's is_32bit field.
>
> --- a/xen/arch/x86/x86_64/asm-offsets.c
> +++ b/xen/arch/x86/x86_64/asm-offsets.c
> @@ -9,7 +9,7 @@
>  #include <xen/perfc.h>
>  #endif
>  #include <xen/sched.h>
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>  #include <compat/xen.h>
>  #endif
>  #include <asm/hardirq.h>
> @@ -102,19 +102,21 @@ void __dummy__(void)
>      BLANK();
>  #endif
>  
> -#ifdef CONFIG_PV
> +#ifdef CONFIG_PV32
>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);

Even if PV32 is compiled out, the is_32bit field still exists, and is
still necessary for crash analysis.  XCA parses this offset information
as part of dissecting /proc/vmcore.

It's one single bool in a fixed size allocation which we've got plenty
of room in.  It can and should stay to avoid impacting the existing
diagnostic tools.

~Andrew

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 01.04.2021 16:31, Andrew Cooper wrote:
> On 25/11/2020 08:51, Jan Beulich wrote:
>> @@ -102,19 +102,21 @@ void __dummy__(void)
>>      BLANK();
>>  #endif
>>  
>> -#ifdef CONFIG_PV
>> +#ifdef CONFIG_PV32
>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
> 
> Even if PV32 is compiled out, the is_32bit field still exists, and is
> still necessary for crash analysis.  XCA parses this offset information
> as part of dissecting /proc/vmcore.
> 
> It's one single bool in a fixed size allocation which we've got plenty
> of room in.  It can and should stay to avoid impacting the existing
> diagnostic tools.

I'm afraid I don't understand at all: I'm not removing the field.
All I'm removing is the entry for it in asm-offsets.h. I'm also
unaware that the offset information gets added anywhere in the
binary, except encoded in instructions. If a tool needs to know
the offset, it ought to parse debug info?

Jan

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Andrew Cooper]

On 01/04/2021 15:37, Jan Beulich wrote:
> On 01.04.2021 16:31, Andrew Cooper wrote:
>> On 25/11/2020 08:51, Jan Beulich wrote:
>>> @@ -102,19 +102,21 @@ void __dummy__(void)
>>>      BLANK();
>>>  #endif
>>>  
>>> -#ifdef CONFIG_PV
>>> +#ifdef CONFIG_PV32
>>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>> Even if PV32 is compiled out, the is_32bit field still exists, and is
>> still necessary for crash analysis.  XCA parses this offset information
>> as part of dissecting /proc/vmcore.
>>
>> It's one single bool in a fixed size allocation which we've got plenty
>> of room in.  It can and should stay to avoid impacting the existing
>> diagnostic tools.
> I'm afraid I don't understand at all: I'm not removing the field.

You talked about removing it in the commit message.

> All I'm removing is the entry for it in asm-offsets.h.

Yes, and that will break XCA, which is used by several downstreams, not
just XenServer.

For RPM package reasons, you can't use debuginfo packages, because
what's on disk doesn't match what's in memory until you've rebooted. 
Livepatching adds an extra dimension of fun here.  There's not enough
space in the vmcoreinfo page to pass enough structure information, so
asm offsets is appended to the symbol table.  Yes its a gross hack, but
its how things currently work.

~Andrew

Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich]

On 06.04.2021 19:34, Andrew Cooper wrote:
> On 01/04/2021 15:37, Jan Beulich wrote:
>> On 01.04.2021 16:31, Andrew Cooper wrote:
>>> On 25/11/2020 08:51, Jan Beulich wrote:
>>>> @@ -102,19 +102,21 @@ void __dummy__(void)
>>>>      BLANK();
>>>>  #endif
>>>>  
>>>> -#ifdef CONFIG_PV
>>>> +#ifdef CONFIG_PV32
>>>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>>> Even if PV32 is compiled out, the is_32bit field still exists, and is
>>> still necessary for crash analysis.  XCA parses this offset information
>>> as part of dissecting /proc/vmcore.
>>>
>>> It's one single bool in a fixed size allocation which we've got plenty
>>> of room in.  It can and should stay to avoid impacting the existing
>>> diagnostic tools.
>> I'm afraid I don't understand at all: I'm not removing the field.
> 
> You talked about removing it in the commit message.

Oh, in a post-commit message TBD remark, yes. Is your objection here
then merely to this possible further plan of removing that field, but
not against the changes in this patch?

>> All I'm removing is the entry for it in asm-offsets.h.
> 
> Yes, and that will break XCA, which is used by several downstreams, not
> just XenServer.
> 
> For RPM package reasons, you can't use debuginfo packages, because
> what's on disk doesn't match what's in memory until you've rebooted. 
> Livepatching adds an extra dimension of fun here.  There's not enough
> space in the vmcoreinfo page to pass enough structure information, so
> asm offsets is appended to the symbol table.  Yes its a gross hack, but
> its how things currently work.

Would you mind pointing me at where this appending is happening? You
can't mean the ELF symbol table in xen-syms - there's no entry for
DOMAIN_is_32bit_pv there. Plus I don't see the problem here, as the
asm-offsets entry already is conditional - it's merely the condition
which gets changed. Or if you mean the is_32bit field - there's no
representation there either in xen-syms (except, when enabled, in
debug info).

Jan