Mailing List Archive

[PATCH 5/5] x86: don't build unused entry code when !PV32
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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
Re: [PATCH 5/5] x86: don't build unused entry code when !PV32 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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