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
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
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.
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
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);
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.
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
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();
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.
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
/*
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.
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, \
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.
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
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.
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
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
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.
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.