* [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes
@ 2023-07-28 19:43 Andrew Cooper
2023-07-28 19:43 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Andrew Cooper @ 2023-07-28 19:43 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini
'debug' and 'str' account for an awefully large number of shadowed variable
violations.
Andrew Cooper (3):
x86/traps: Move do_general_protection() earlier
x86/entry: Rename the exception entrypoints
x86: Delete str()
xen/arch/x86/hvm/svm/svm.c | 2 -
xen/arch/x86/include/asm/desc.h | 9 --
xen/arch/x86/include/asm/processor.h | 34 +---
xen/arch/x86/pv/traps.c | 2 +
xen/arch/x86/traps.c | 227 ++++++++++++++-------------
xen/arch/x86/x86_64/entry.S | 36 ++---
6 files changed, 142 insertions(+), 168 deletions(-)
base-commit: dbd566ab729d2839c3c0be5c47cdcc06c2c477f0
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] x86/traps: Move do_general_protection() earlier
2023-07-28 19:43 [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Andrew Cooper
@ 2023-07-28 19:43 ` Andrew Cooper
2023-07-28 19:43 ` [PATCH 2/3] x86/entry: Rename the exception entrypoints Andrew Cooper
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2023-07-28 19:43 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini
... in order to clean up the declarations without needing to forward declare
it for handle_gdt_ldt_mapping_fault()
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/arch/x86/traps.c | 157 +++++++++++++++++++++----------------------
1 file changed, 78 insertions(+), 79 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4229bda159ce..e05e8964482e 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1224,6 +1224,84 @@ void do_int3(struct cpu_user_regs *regs)
pv_inject_hw_exception(X86_EXC_BP, X86_EVENT_NO_EC);
}
+void do_general_protection(struct cpu_user_regs *regs)
+{
+#ifdef CONFIG_PV
+ struct vcpu *v = current;
+#endif
+
+ if ( regs->error_code & X86_XEC_EXT )
+ goto hardware_gp;
+
+ if ( !guest_mode(regs) )
+ goto gp_in_kernel;
+
+#ifdef CONFIG_PV
+ /*
+ * Cunning trick to allow arbitrary "INT n" handling.
+ *
+ * We set DPL == 0 on all vectors in the IDT. This prevents any INT <n>
+ * instruction from trapping to the appropriate vector, when that might not
+ * be expected by Xen or the guest OS. For example, that entry might be for
+ * a fault handler (unlike traps, faults don't increment EIP), or might
+ * expect an error code on the stack (which a software trap never
+ * provides), or might be a hardware interrupt handler that doesn't like
+ * being called spuriously.
+ *
+ * Instead, a GPF occurs with the faulting IDT vector in the error code.
+ * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is
+ * clear (which got already checked above) to indicate that it's a software
+ * fault, not a hardware one.
+ *
+ * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
+ * okay because they can only be triggered by an explicit DPL-checked
+ * instruction. The DPL specified by the guest OS for these vectors is NOT
+ * CHECKED!!
+ */
+ if ( regs->error_code & X86_XEC_IDT )
+ {
+ /* This fault must be due to <INT n> instruction. */
+ uint8_t vector = regs->error_code >> 3;
+ const struct trap_info *ti = &v->arch.pv.trap_ctxt[vector];
+
+ if ( permit_softint(TI_GET_DPL(ti), v, regs) )
+ {
+ regs->rip += 2;
+ pv_inject_sw_interrupt(vector);
+ return;
+ }
+ }
+ else if ( is_pv_32bit_vcpu(v) && regs->error_code )
+ {
+ pv_emulate_gate_op(regs);
+ return;
+ }
+
+ /* Emulate some simple privileged and I/O instructions. */
+ if ( (regs->error_code == 0) &&
+ pv_emulate_privileged_op(regs) )
+ {
+ trace_trap_one_addr(TRC_PV_EMULATE_PRIVOP, regs->rip);
+ return;
+ }
+
+ /* Pass on GPF as is. */
+ pv_inject_hw_exception(X86_EXC_GP, regs->error_code);
+ return;
+#endif
+
+ gp_in_kernel:
+ if ( likely(extable_fixup(regs, true)) )
+ return;
+
+ hardware_gp:
+ if ( debugger_trap_fatal(X86_EXC_GP, regs) )
+ return;
+
+ show_execution_state(regs);
+ panic("GENERAL PROTECTION FAULT\n[error_code=%04x]\n", regs->error_code);
+}
+
#ifdef CONFIG_PV
static int handle_ldt_mapping_fault(unsigned int offset,
struct cpu_user_regs *regs)
@@ -1589,85 +1667,6 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
}
}
-void do_general_protection(struct cpu_user_regs *regs)
-{
-#ifdef CONFIG_PV
- struct vcpu *v = current;
-#endif
-
- if ( regs->error_code & X86_XEC_EXT )
- goto hardware_gp;
-
- if ( !guest_mode(regs) )
- goto gp_in_kernel;
-
-#ifdef CONFIG_PV
- /*
- * Cunning trick to allow arbitrary "INT n" handling.
- *
- * We set DPL == 0 on all vectors in the IDT. This prevents any INT <n>
- * instruction from trapping to the appropriate vector, when that might not
- * be expected by Xen or the guest OS. For example, that entry might be for
- * a fault handler (unlike traps, faults don't increment EIP), or might
- * expect an error code on the stack (which a software trap never
- * provides), or might be a hardware interrupt handler that doesn't like
- * being called spuriously.
- *
- * Instead, a GPF occurs with the faulting IDT vector in the error code.
- * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is
- * clear (which got already checked above) to indicate that it's a software
- * fault, not a hardware one.
- *
- * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
- * okay because they can only be triggered by an explicit DPL-checked
- * instruction. The DPL specified by the guest OS for these vectors is NOT
- * CHECKED!!
- */
- if ( regs->error_code & X86_XEC_IDT )
- {
- /* This fault must be due to <INT n> instruction. */
- uint8_t vector = regs->error_code >> 3;
- const struct trap_info *ti = &v->arch.pv.trap_ctxt[vector];
-
- if ( permit_softint(TI_GET_DPL(ti), v, regs) )
- {
- regs->rip += 2;
- pv_inject_sw_interrupt(vector);
- return;
- }
- }
- else if ( is_pv_32bit_vcpu(v) && regs->error_code )
- {
- pv_emulate_gate_op(regs);
- return;
- }
-
- /* Emulate some simple privileged and I/O instructions. */
- if ( (regs->error_code == 0) &&
- pv_emulate_privileged_op(regs) )
- {
- trace_trap_one_addr(TRC_PV_EMULATE_PRIVOP, regs->rip);
- return;
- }
-
- /* Pass on GPF as is. */
- pv_inject_hw_exception(X86_EXC_GP, regs->error_code);
- return;
-#endif
-
- gp_in_kernel:
-
- if ( likely(extable_fixup(regs, true)) )
- return;
-
- hardware_gp:
- if ( debugger_trap_fatal(X86_EXC_GP, regs) )
- return;
-
- show_execution_state(regs);
- panic("GENERAL PROTECTION FAULT\n[error_code=%04x]\n", regs->error_code);
-}
-
static bool pci_serr_cont;
static bool pci_serr_nmicont(void)
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] x86/entry: Rename the exception entrypoints
2023-07-28 19:43 [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Andrew Cooper
2023-07-28 19:43 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
@ 2023-07-28 19:43 ` Andrew Cooper
2023-07-31 8:57 ` Jan Beulich
2023-07-31 9:15 ` Nicola Vetrini
2023-07-28 19:43 ` [PATCH 3/3] x86: Delete str() Andrew Cooper
2023-08-01 10:02 ` [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Jan Beulich
3 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2023-07-28 19:43 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini
This makes the names match the architectural short names that we use
elsewhere. This avoids 'debug' in particular from being a global symbol
shadowed by many local parameter names.
Remove the DECLARE_TRAP_HANDLER{,_CONST}() infrastructure. Only NMI/#MC are
referenced externally (and NMI will cease to be soon, as part of adding FRED
support). Move the entrypoint declarations into the respective traps.c where
they're used, rather than keeping them visible across ~all of Xen.
Drop the long-stale comment at the top of init_idt_traps(). It's mostly
discussing a 32bit Xen, and bogus otherwise as it's impossible to use trap
gates correctly for these purposes.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
This is half of a previous patch, cut down to just the rename and header file
cleanup.
---
xen/arch/x86/include/asm/processor.h | 34 +-------------
xen/arch/x86/pv/traps.c | 2 +
xen/arch/x86/traps.c | 70 +++++++++++++++++-----------
xen/arch/x86/x86_64/entry.S | 36 +++++++-------
4 files changed, 64 insertions(+), 78 deletions(-)
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index c0529cc3d984..0989748be6d5 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -417,38 +417,8 @@ extern void mtrr_bp_init(void);
void mcheck_init(struct cpuinfo_x86 *c, bool_t bsp);
-#define DECLARE_TRAP_HANDLER(_name) \
- void _name(void); \
- void do_ ## _name(struct cpu_user_regs *regs)
-#define DECLARE_TRAP_HANDLER_CONST(_name) \
- void _name(void); \
- void do_ ## _name(const struct cpu_user_regs *regs)
-
-DECLARE_TRAP_HANDLER(divide_error);
-DECLARE_TRAP_HANDLER(debug);
-DECLARE_TRAP_HANDLER_CONST(nmi);
-DECLARE_TRAP_HANDLER(int3);
-DECLARE_TRAP_HANDLER(overflow);
-DECLARE_TRAP_HANDLER(bounds);
-DECLARE_TRAP_HANDLER(invalid_op);
-DECLARE_TRAP_HANDLER(device_not_available);
-DECLARE_TRAP_HANDLER(double_fault);
-DECLARE_TRAP_HANDLER(invalid_TSS);
-DECLARE_TRAP_HANDLER(segment_not_present);
-DECLARE_TRAP_HANDLER(stack_segment);
-DECLARE_TRAP_HANDLER(general_protection);
-DECLARE_TRAP_HANDLER(page_fault);
-DECLARE_TRAP_HANDLER(early_page_fault);
-DECLARE_TRAP_HANDLER(coprocessor_error);
-DECLARE_TRAP_HANDLER(simd_coprocessor_error);
-DECLARE_TRAP_HANDLER_CONST(machine_check);
-DECLARE_TRAP_HANDLER(alignment_check);
-DECLARE_TRAP_HANDLER(entry_CP);
-
-DECLARE_TRAP_HANDLER(entry_int82);
-
-#undef DECLARE_TRAP_HANDLER_CONST
-#undef DECLARE_TRAP_HANDLER
+void do_nmi(const struct cpu_user_regs *regs);
+void do_machine_check(const struct cpu_user_regs *regs);
void trap_nop(void);
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index 17ca4d1d5142..74f333da7e1c 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -132,6 +132,8 @@ static void cf_check nmi_softirq(void)
*v_ptr = NULL;
}
+void nocall entry_int82(void);
+
void __init pv_trap_init(void)
{
#ifdef CONFIG_PV32
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e05e8964482e..8470561cbc27 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2107,35 +2107,49 @@ void percpu_traps_init(void)
wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
}
+/* Exception entries */
+void nocall entry_DE(void);
+void nocall entry_DB(void);
+void nocall entry_NMI(void);
+void nocall entry_BP(void);
+void nocall entry_OF(void);
+void nocall entry_BR(void);
+void nocall entry_UD(void);
+void nocall entry_NM(void);
+void nocall entry_DF(void);
+void nocall entry_TS(void);
+void nocall entry_NP(void);
+void nocall entry_SS(void);
+void nocall entry_GP(void);
+void nocall early_page_fault(void);
+void nocall entry_PF(void);
+void nocall entry_MF(void);
+void nocall entry_AC(void);
+void nocall entry_MC(void);
+void nocall entry_XM(void);
+void nocall entry_CP(void);
+
void __init init_idt_traps(void)
{
- /*
- * Note that interrupt gates are always used, rather than trap gates. We
- * must have interrupts disabled until DS/ES/FS/GS are saved because the
- * first activation must have the "bad" value(s) for these registers and
- * we may lose them if another activation is installed before they are
- * saved. The page-fault handler also needs interrupts disabled until %cr2
- * has been read and saved on the stack.
- */
- set_intr_gate(X86_EXC_DE, divide_error);
- set_intr_gate(X86_EXC_DB, debug);
- set_intr_gate(X86_EXC_NMI, nmi);
- set_swint_gate(X86_EXC_BP, int3); /* usable from all privileges */
- set_swint_gate(X86_EXC_OF, overflow); /* usable from all privileges */
- set_intr_gate(X86_EXC_BR, bounds);
- set_intr_gate(X86_EXC_UD, invalid_op);
- set_intr_gate(X86_EXC_NM, device_not_available);
- set_intr_gate(X86_EXC_DF, double_fault);
- set_intr_gate(X86_EXC_TS, invalid_TSS);
- set_intr_gate(X86_EXC_NP, segment_not_present);
- set_intr_gate(X86_EXC_SS, stack_segment);
- set_intr_gate(X86_EXC_GP, general_protection);
- set_intr_gate(X86_EXC_PF, early_page_fault);
- set_intr_gate(X86_EXC_MF, coprocessor_error);
- set_intr_gate(X86_EXC_AC, alignment_check);
- set_intr_gate(X86_EXC_MC, machine_check);
- set_intr_gate(X86_EXC_XM, simd_coprocessor_error);
- set_intr_gate(X86_EXC_CP, entry_CP);
+ set_intr_gate (X86_EXC_DE, entry_DE);
+ set_intr_gate (X86_EXC_DB, entry_DB);
+ set_intr_gate (X86_EXC_NMI, entry_NMI);
+ set_swint_gate(X86_EXC_BP, entry_BP);
+ set_swint_gate(X86_EXC_OF, entry_OF);
+ set_intr_gate (X86_EXC_BR, entry_BR);
+ set_intr_gate (X86_EXC_UD, entry_UD);
+ set_intr_gate (X86_EXC_NM, entry_NM);
+ set_intr_gate (X86_EXC_DF, entry_DF);
+ set_intr_gate (X86_EXC_TS, entry_TS);
+ set_intr_gate (X86_EXC_NP, entry_NP);
+ set_intr_gate (X86_EXC_SS, entry_SS);
+ set_intr_gate (X86_EXC_GP, entry_GP);
+ set_intr_gate (X86_EXC_PF, early_page_fault);
+ set_intr_gate (X86_EXC_MF, entry_MF);
+ set_intr_gate (X86_EXC_AC, entry_AC);
+ set_intr_gate (X86_EXC_MC, entry_MC);
+ set_intr_gate (X86_EXC_XM, entry_XM);
+ set_intr_gate (X86_EXC_CP, entry_CP);
/* Specify dedicated interrupt stacks for NMI, #DF, and #MC. */
enable_each_ist(idt_table);
@@ -2154,7 +2168,7 @@ void __init trap_init(void)
unsigned int vector;
/* Replace early pagefault with real pagefault handler. */
- set_intr_gate(X86_EXC_PF, &page_fault);
+ set_intr_gate(X86_EXC_PF, entry_PF);
pv_trap_init();
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index bca1500e2b45..81dd2c74b876 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -708,7 +708,7 @@ ENTRY(common_interrupt)
mov %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
jmp ret_from_intr
-ENTRY(page_fault)
+ENTRY(entry_PF)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
/* No special register assumptions. */
@@ -881,81 +881,81 @@ FATAL_exception_with_ints_disabled:
movq %rsp,%rdi
tailcall fatal_trap
-ENTRY(divide_error)
+ENTRY(entry_DE)
ENDBR64
pushq $0
movl $X86_EXC_DE, 4(%rsp)
jmp handle_exception
-ENTRY(coprocessor_error)
+ENTRY(entry_MF)
ENDBR64
pushq $0
movl $X86_EXC_MF, 4(%rsp)
jmp handle_exception
-ENTRY(simd_coprocessor_error)
+ENTRY(entry_XM)
ENDBR64
pushq $0
movl $X86_EXC_XM, 4(%rsp)
jmp handle_exception
-ENTRY(device_not_available)
+ENTRY(entry_NM)
ENDBR64
pushq $0
movl $X86_EXC_NM, 4(%rsp)
jmp handle_exception
-ENTRY(debug)
+ENTRY(entry_DB)
ENDBR64
pushq $0
movl $X86_EXC_DB, 4(%rsp)
jmp handle_ist_exception
-ENTRY(int3)
+ENTRY(entry_BP)
ENDBR64
pushq $0
movl $X86_EXC_BP, 4(%rsp)
jmp handle_exception
-ENTRY(overflow)
+ENTRY(entry_OF)
ENDBR64
pushq $0
movl $X86_EXC_OF, 4(%rsp)
jmp handle_exception
-ENTRY(bounds)
+ENTRY(entry_BR)
ENDBR64
pushq $0
movl $X86_EXC_BR, 4(%rsp)
jmp handle_exception
-ENTRY(invalid_op)
+ENTRY(entry_UD)
ENDBR64
pushq $0
movl $X86_EXC_UD, 4(%rsp)
jmp handle_exception
-ENTRY(invalid_TSS)
+ENTRY(entry_TS)
ENDBR64
movl $X86_EXC_TS, 4(%rsp)
jmp handle_exception
-ENTRY(segment_not_present)
+ENTRY(entry_NP)
ENDBR64
movl $X86_EXC_NP, 4(%rsp)
jmp handle_exception
-ENTRY(stack_segment)
+ENTRY(entry_SS)
ENDBR64
movl $X86_EXC_SS, 4(%rsp)
jmp handle_exception
-ENTRY(general_protection)
+ENTRY(entry_GP)
ENDBR64
movl $X86_EXC_GP, 4(%rsp)
jmp handle_exception
-ENTRY(alignment_check)
+ENTRY(entry_AC)
ENDBR64
movl $X86_EXC_AC, 4(%rsp)
jmp handle_exception
@@ -965,7 +965,7 @@ ENTRY(entry_CP)
movl $X86_EXC_CP, 4(%rsp)
jmp handle_exception
-ENTRY(double_fault)
+ENTRY(entry_DF)
ENDBR64
movl $X86_EXC_DF, 4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
@@ -989,7 +989,7 @@ ENTRY(double_fault)
movq %rsp,%rdi
tailcall do_double_fault
-ENTRY(nmi)
+ENTRY(entry_NMI)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
@@ -1117,7 +1117,7 @@ handle_ist_exception:
jmp restore_all_xen
#endif
-ENTRY(machine_check)
+ENTRY(entry_MC)
ENDBR64
pushq $0
movl $X86_EXC_MC, 4(%rsp)
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] x86: Delete str()
2023-07-28 19:43 [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Andrew Cooper
2023-07-28 19:43 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
2023-07-28 19:43 ` [PATCH 2/3] x86/entry: Rename the exception entrypoints Andrew Cooper
@ 2023-07-28 19:43 ` Andrew Cooper
2023-07-31 8:25 ` Jan Beulich
2023-07-31 9:17 ` Nicola Vetrini
2023-08-01 10:02 ` [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Jan Beulich
3 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2023-07-28 19:43 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini
This is used in an assertion only, which is somewhat dubious to begin with and
won't surivive the x86-S work (where TR will become be a NUL selector).
Delete it now. This avoids many cases where as a global symbol, it shadows
local string variables.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
xen/arch/x86/hvm/svm/svm.c | 2 --
xen/arch/x86/include/asm/desc.h | 9 ---------
2 files changed, 11 deletions(-)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 56cb2f61bb75..4d29ad3bc36a 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
/* Initialize OSVW bits to be used by guests */
svm_host_osvw_init();
- /* Minimal checking that enough CPU setup was done by now. */
- ASSERT(str() == TSS_SELECTOR);
svm_vmsave_pa(per_cpu(host_vmcb, cpu));
return 0;
diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
index 225a864c483e..a1e0807d97ed 100644
--- a/xen/arch/x86/include/asm/desc.h
+++ b/xen/arch/x86/include/asm/desc.h
@@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
__asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
}
-static inline unsigned int str(void)
-{
- unsigned int sel;
-
- __asm__ ( "str %0" : "=r" (sel) );
-
- return sel;
-}
-
#endif /* !__ASSEMBLY__ */
#endif /* __ARCH_DESC_H */
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] x86: Delete str()
2023-07-28 19:43 ` [PATCH 3/3] x86: Delete str() Andrew Cooper
@ 2023-07-31 8:25 ` Jan Beulich
2023-08-02 14:55 ` Andrew Cooper
2023-07-31 9:17 ` Nicola Vetrini
1 sibling, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-07-31 8:25 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini, Xen-devel
On 28.07.2023 21:43, Andrew Cooper wrote:
> This is used in an assertion only, which is somewhat dubious to begin with and
> won't surivive the x86-S work (where TR will become be a NUL selector).
I'm kind of okay with the removal, but I can't read anything like the
above out of the doc. Can you point me at where this is said?
Jan
> Delete it now. This avoids many cases where as a global symbol, it shadows
> local string variables.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/arch/x86/hvm/svm/svm.c | 2 --
> xen/arch/x86/include/asm/desc.h | 9 ---------
> 2 files changed, 11 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 56cb2f61bb75..4d29ad3bc36a 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
> /* Initialize OSVW bits to be used by guests */
> svm_host_osvw_init();
>
> - /* Minimal checking that enough CPU setup was done by now. */
> - ASSERT(str() == TSS_SELECTOR);
> svm_vmsave_pa(per_cpu(host_vmcb, cpu));
>
> return 0;
> diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
> index 225a864c483e..a1e0807d97ed 100644
> --- a/xen/arch/x86/include/asm/desc.h
> +++ b/xen/arch/x86/include/asm/desc.h
> @@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
> __asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
> }
>
> -static inline unsigned int str(void)
> -{
> - unsigned int sel;
> -
> - __asm__ ( "str %0" : "=r" (sel) );
> -
> - return sel;
> -}
> -
> #endif /* !__ASSEMBLY__ */
>
> #endif /* __ARCH_DESC_H */
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] x86/entry: Rename the exception entrypoints
2023-07-28 19:43 ` [PATCH 2/3] x86/entry: Rename the exception entrypoints Andrew Cooper
@ 2023-07-31 8:57 ` Jan Beulich
2023-07-31 9:15 ` Nicola Vetrini
1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-07-31 8:57 UTC (permalink / raw)
To: Andrew Cooper, Roberto Bagnara
Cc: Roger Pau Monné, Wei Liu, Nicola Vetrini, Xen-devel
On 28.07.2023 21:43, Andrew Cooper wrote:
> This makes the names match the architectural short names that we use
> elsewhere. This avoids 'debug' in particular from being a global symbol
> shadowed by many local parameter names.
>
> Remove the DECLARE_TRAP_HANDLER{,_CONST}() infrastructure. Only NMI/#MC are
> referenced externally (and NMI will cease to be soon, as part of adding FRED
> support). Move the entrypoint declarations into the respective traps.c where
> they're used, rather than keeping them visible across ~all of Xen.
Coming back to my Misra concern here: There's no way we can avoid violating
rule 8.7 (and maybe also 8.6) when it comes to symbols defined in assembly
code. Rule 8.5 otoh is a little more relaxed than I recalled when commenting
on the earlier version of the patch, but it still talks of exclusively
"header files". Roberto, could you please clarify whether putting such
declarations in .c files is indeed not a violation of 8.5?
In any event I see you've added "nocall" - I guess that's an easy
identification for (some of) the symbols defined in assembly code, and
hence needing special casing in scans.
> Drop the long-stale comment at the top of init_idt_traps(). It's mostly
> discussing a 32bit Xen, and bogus otherwise as it's impossible to use trap
> gates correctly for these purposes.
I don't think I follow the "impossible", but I'm also not going to make my
(eventual) ack dependent upon you further adjusting this part of the
description.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] x86/entry: Rename the exception entrypoints
2023-07-28 19:43 ` [PATCH 2/3] x86/entry: Rename the exception entrypoints Andrew Cooper
2023-07-31 8:57 ` Jan Beulich
@ 2023-07-31 9:15 ` Nicola Vetrini
1 sibling, 0 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-07-31 9:15 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Wei Liu, Roberto Bagnara
On 28/07/2023 21:43, Andrew Cooper wrote:
> This makes the names match the architectural short names that we use
> elsewhere. This avoids 'debug' in particular from being a global
> symbol
> shadowed by many local parameter names.
>
> Remove the DECLARE_TRAP_HANDLER{,_CONST}() infrastructure. Only
> NMI/#MC are
> referenced externally (and NMI will cease to be soon, as part of adding
> FRED
> support). Move the entrypoint declarations into the respective traps.c
> where
> they're used, rather than keeping them visible across ~all of Xen.
>
> Drop the long-stale comment at the top of init_idt_traps(). It's
> mostly
> discussing a 32bit Xen, and bogus otherwise as it's impossible to use
> trap
> gates correctly for these purposes.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
>
> This is half of a previous patch, cut down to just the rename and
> header file
> cleanup.
> ---
> xen/arch/x86/include/asm/processor.h | 34 +-------------
> xen/arch/x86/pv/traps.c | 2 +
> xen/arch/x86/traps.c | 70 +++++++++++++++++-----------
> xen/arch/x86/x86_64/entry.S | 36 +++++++-------
> 4 files changed, 64 insertions(+), 78 deletions(-)
>
With respect to Rule 5.3:
Tested-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] x86: Delete str()
2023-07-28 19:43 ` [PATCH 3/3] x86: Delete str() Andrew Cooper
2023-07-31 8:25 ` Jan Beulich
@ 2023-07-31 9:17 ` Nicola Vetrini
1 sibling, 0 replies; 12+ messages in thread
From: Nicola Vetrini @ 2023-07-31 9:17 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Wei Liu, Roberto Bagnara
On 28/07/2023 21:43, Andrew Cooper wrote:
> This is used in an assertion only, which is somewhat dubious to begin
> with and
> won't surivive the x86-S work (where TR will become be a NUL selector).
>
> Delete it now. This avoids many cases where as a global symbol, it
> shadows
> local string variables.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roberto Bagnara <roberto.bagnara@bugseng.com>
> CC: Nicola Vetrini <nicola.vetrini@bugseng.com>
> ---
> xen/arch/x86/hvm/svm/svm.c | 2 --
> xen/arch/x86/include/asm/desc.h | 9 ---------
> 2 files changed, 11 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 56cb2f61bb75..4d29ad3bc36a 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1614,8 +1614,6 @@ static int _svm_cpu_up(bool bsp)
> /* Initialize OSVW bits to be used by guests */
> svm_host_osvw_init();
>
> - /* Minimal checking that enough CPU setup was done by now. */
> - ASSERT(str() == TSS_SELECTOR);
> svm_vmsave_pa(per_cpu(host_vmcb, cpu));
>
> return 0;
> diff --git a/xen/arch/x86/include/asm/desc.h
> b/xen/arch/x86/include/asm/desc.h
> index 225a864c483e..a1e0807d97ed 100644
> --- a/xen/arch/x86/include/asm/desc.h
> +++ b/xen/arch/x86/include/asm/desc.h
> @@ -238,15 +238,6 @@ static inline void ltr(unsigned int sel)
> __asm__ __volatile__ ( "ltr %w0" :: "rm" (sel) : "memory" );
> }
>
> -static inline unsigned int str(void)
> -{
> - unsigned int sel;
> -
> - __asm__ ( "str %0" : "=r" (sel) );
> -
> - return sel;
> -}
> -
> #endif /* !__ASSEMBLY__ */
>
> #endif /* __ARCH_DESC_H */
With respect to shadowing (Rule 5.3)
Tested-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes
2023-07-28 19:43 [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Andrew Cooper
` (2 preceding siblings ...)
2023-07-28 19:43 ` [PATCH 3/3] x86: Delete str() Andrew Cooper
@ 2023-08-01 10:02 ` Jan Beulich
3 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-08-01 10:02 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini, Xen-devel
On 28.07.2023 21:43, Andrew Cooper wrote:
> 'debug' and 'str' account for an awefully large number of shadowed variable
> violations.
>
> Andrew Cooper (3):
> x86/traps: Move do_general_protection() earlier
> x86/entry: Rename the exception entrypoints
> x86: Delete str()
Series
Acked-by: Jan Beulich <jbeulich@suse.com>
with a slightly amended description in patch 3, and preferably also with
at least mentioning the Misra angle in patch 2.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] x86: Delete str()
2023-07-31 8:25 ` Jan Beulich
@ 2023-08-02 14:55 ` Andrew Cooper
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2023-08-02 14:55 UTC (permalink / raw)
To: Jan Beulich
Cc: Roger Pau Monné,
Wei Liu, Roberto Bagnara, Nicola Vetrini, Xen-devel
On 31/07/2023 9:25 am, Jan Beulich wrote:
> On 28.07.2023 21:43, Andrew Cooper wrote:
>> This is used in an assertion only, which is somewhat dubious to begin with and
>> won't surivive the x86-S work (where TR will become be a NUL selector).
> I'm kind of okay with the removal, but I can't read anything like the
> above out of the doc. Can you point me at where this is said?
A future draft of the spec.
FRED removes the IDT completely, most of the TSS, and can let you get
away with GDT/LDT limits of 0.
x86-S removes the final aspects of the TSS (the IO perm bitmap, and PVI).
Intel have agreed that being able to (effectively) `ltr $0x0000` to set
the TSS invalid (like NULL selectors do for all other segments) would be
useful, as it means you don't need to have a transiently non-empty GDT
just to load an empty TR.
~Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] x86/traps: Move do_general_protection() earlier
2023-02-20 11:59 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
@ 2023-02-21 11:48 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-02-21 11:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 20.02.2023 12:59, Andrew Cooper wrote:
> ... in order to clean up the declarations without needing to forward declare
> it for handle_gdt_ldt_mapping_fault()
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
This is okay with me as long as the Misra related comment on patch 2
can be resolved suitably. I'll defer the ack here until then ...
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] x86/traps: Move do_general_protection() earlier
2023-02-20 11:59 [PATCH 0/3] x86/entry: Cleanup and livepatch support Andrew Cooper
@ 2023-02-20 11:59 ` Andrew Cooper
2023-02-21 11:48 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Cooper @ 2023-02-20 11:59 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
... in order to clean up the declarations without needing to forward declare
it for handle_gdt_ldt_mapping_fault()
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/traps.c | 157 +++++++++++++++++++++----------------------
1 file changed, 78 insertions(+), 79 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index cade9e12f8fa..7fb0c54f884e 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1301,6 +1301,84 @@ void do_int3(struct cpu_user_regs *regs)
pv_inject_hw_exception(TRAP_int3, X86_EVENT_NO_EC);
}
+void do_general_protection(struct cpu_user_regs *regs)
+{
+#ifdef CONFIG_PV
+ struct vcpu *v = current;
+#endif
+
+ if ( regs->error_code & X86_XEC_EXT )
+ goto hardware_gp;
+
+ if ( !guest_mode(regs) )
+ goto gp_in_kernel;
+
+#ifdef CONFIG_PV
+ /*
+ * Cunning trick to allow arbitrary "INT n" handling.
+ *
+ * We set DPL == 0 on all vectors in the IDT. This prevents any INT <n>
+ * instruction from trapping to the appropriate vector, when that might not
+ * be expected by Xen or the guest OS. For example, that entry might be for
+ * a fault handler (unlike traps, faults don't increment EIP), or might
+ * expect an error code on the stack (which a software trap never
+ * provides), or might be a hardware interrupt handler that doesn't like
+ * being called spuriously.
+ *
+ * Instead, a GPF occurs with the faulting IDT vector in the error code.
+ * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is
+ * clear (which got already checked above) to indicate that it's a software
+ * fault, not a hardware one.
+ *
+ * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
+ * okay because they can only be triggered by an explicit DPL-checked
+ * instruction. The DPL specified by the guest OS for these vectors is NOT
+ * CHECKED!!
+ */
+ if ( regs->error_code & X86_XEC_IDT )
+ {
+ /* This fault must be due to <INT n> instruction. */
+ uint8_t vector = regs->error_code >> 3;
+ const struct trap_info *ti = &v->arch.pv.trap_ctxt[vector];
+
+ if ( permit_softint(TI_GET_DPL(ti), v, regs) )
+ {
+ regs->rip += 2;
+ pv_inject_sw_interrupt(vector);
+ return;
+ }
+ }
+ else if ( is_pv_32bit_vcpu(v) && regs->error_code )
+ {
+ pv_emulate_gate_op(regs);
+ return;
+ }
+
+ /* Emulate some simple privileged and I/O instructions. */
+ if ( (regs->error_code == 0) &&
+ pv_emulate_privileged_op(regs) )
+ {
+ trace_trap_one_addr(TRC_PV_EMULATE_PRIVOP, regs->rip);
+ return;
+ }
+
+ /* Pass on GPF as is. */
+ pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
+ return;
+#endif
+
+ gp_in_kernel:
+ if ( likely(extable_fixup(regs, true)) )
+ return;
+
+ hardware_gp:
+ if ( debugger_trap_fatal(TRAP_gp_fault, regs) )
+ return;
+
+ show_execution_state(regs);
+ panic("GENERAL PROTECTION FAULT\n[error_code=%04x]\n", regs->error_code);
+}
+
#ifdef CONFIG_PV
static int handle_ldt_mapping_fault(unsigned int offset,
struct cpu_user_regs *regs)
@@ -1666,85 +1744,6 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
}
}
-void do_general_protection(struct cpu_user_regs *regs)
-{
-#ifdef CONFIG_PV
- struct vcpu *v = current;
-#endif
-
- if ( regs->error_code & X86_XEC_EXT )
- goto hardware_gp;
-
- if ( !guest_mode(regs) )
- goto gp_in_kernel;
-
-#ifdef CONFIG_PV
- /*
- * Cunning trick to allow arbitrary "INT n" handling.
- *
- * We set DPL == 0 on all vectors in the IDT. This prevents any INT <n>
- * instruction from trapping to the appropriate vector, when that might not
- * be expected by Xen or the guest OS. For example, that entry might be for
- * a fault handler (unlike traps, faults don't increment EIP), or might
- * expect an error code on the stack (which a software trap never
- * provides), or might be a hardware interrupt handler that doesn't like
- * being called spuriously.
- *
- * Instead, a GPF occurs with the faulting IDT vector in the error code.
- * Bit 1 is set to indicate that an IDT entry caused the fault. Bit 0 is
- * clear (which got already checked above) to indicate that it's a software
- * fault, not a hardware one.
- *
- * NOTE: Vectors 3 and 4 are dealt with from their own handler. This is
- * okay because they can only be triggered by an explicit DPL-checked
- * instruction. The DPL specified by the guest OS for these vectors is NOT
- * CHECKED!!
- */
- if ( regs->error_code & X86_XEC_IDT )
- {
- /* This fault must be due to <INT n> instruction. */
- uint8_t vector = regs->error_code >> 3;
- const struct trap_info *ti = &v->arch.pv.trap_ctxt[vector];
-
- if ( permit_softint(TI_GET_DPL(ti), v, regs) )
- {
- regs->rip += 2;
- pv_inject_sw_interrupt(vector);
- return;
- }
- }
- else if ( is_pv_32bit_vcpu(v) && regs->error_code )
- {
- pv_emulate_gate_op(regs);
- return;
- }
-
- /* Emulate some simple privileged and I/O instructions. */
- if ( (regs->error_code == 0) &&
- pv_emulate_privileged_op(regs) )
- {
- trace_trap_one_addr(TRC_PV_EMULATE_PRIVOP, regs->rip);
- return;
- }
-
- /* Pass on GPF as is. */
- pv_inject_hw_exception(TRAP_gp_fault, regs->error_code);
- return;
-#endif
-
- gp_in_kernel:
-
- if ( likely(extable_fixup(regs, true)) )
- return;
-
- hardware_gp:
- if ( debugger_trap_fatal(TRAP_gp_fault, regs) )
- return;
-
- show_execution_state(regs);
- panic("GENERAL PROTECTION FAULT\n[error_code=%04x]\n", regs->error_code);
-}
-
static bool pci_serr_cont;
static bool pci_serr_nmicont(void)
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-08-02 14:56 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-28 19:43 [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Andrew Cooper
2023-07-28 19:43 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
2023-07-28 19:43 ` [PATCH 2/3] x86/entry: Rename the exception entrypoints Andrew Cooper
2023-07-31 8:57 ` Jan Beulich
2023-07-31 9:15 ` Nicola Vetrini
2023-07-28 19:43 ` [PATCH 3/3] x86: Delete str() Andrew Cooper
2023-07-31 8:25 ` Jan Beulich
2023-08-02 14:55 ` Andrew Cooper
2023-07-31 9:17 ` Nicola Vetrini
2023-08-01 10:02 ` [PATCH 0/3] x86: Some MISRA Rule 5.3 fixes Jan Beulich
-- strict thread matches above, loose matches on Subject: below --
2023-02-20 11:59 [PATCH 0/3] x86/entry: Cleanup and livepatch support Andrew Cooper
2023-02-20 11:59 ` [PATCH 1/3] x86/traps: Move do_general_protection() earlier Andrew Cooper
2023-02-21 11:48 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.