From: "H. Peter Anvin (Intel)" <[email protected]>
A collection of trap/interrupt-related patches, almost all
cleanups. The only patches that should have any possible effect at all
are:
4/6 - x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]
This patch reverses kvm_set_cpu_l1tf_flush_l1d() and
__irq_enter_raw() in DEFINE_IDTENTRY_SYSVEC_SIMPLE() in order
to be able to unify the code with DEFINE_IDTENTRY_SYSVEC().
The reason for unification is mainly to avoid the possibility
of inadvertent divergence rather than the rather modest amount
of code.
5/6 - x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt
This condition is believed to be impossible after many
improvements to the IRQ vector allocation code since this
function was written. Per discussion with tglx, add a
WARN_ONCE() if this happens as a first step towards excising
this hack.
---
x86/traps: add X86_NR_HW_TRAPS to <asm/trapnr.h>
x86/irqvector: add NR_EXTERNAL_VECTORS and NR_SYSTEM_VECTORS
x86/idt: remove address argument to idt_invalidate()
x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]
x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt
x86/irq: remove unused vectors from <asm/irq_vectors.h>
arch/x86/include/asm/desc.h | 2 +-
arch/x86/include/asm/idtentry.h | 39 +++++++++++++++++---------------
arch/x86/include/asm/irq_vectors.h | 7 ++++--
arch/x86/include/asm/trapnr.h | 1 +
arch/x86/kernel/apic/vector.c | 5 ++++
arch/x86/kernel/idt.c | 5 ++--
arch/x86/kernel/machine_kexec_32.c | 4 ++--
arch/x86/kernel/reboot.c | 2 +-
tools/arch/x86/include/asm/irq_vectors.h | 7 ++++--
9 files changed, 43 insertions(+), 29 deletions(-)
From: "H. Peter Anvin (Intel)" <[email protected]>
Move the common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE] into a common
macro.
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/include/asm/idtentry.h | 35 ++++++++++++++++++---------------
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index c03a18cac78e..de3727db021a 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -219,6 +219,23 @@ static noinline void __##func(struct pt_regs *regs, u32 vector)
#define DECLARE_IDTENTRY_SYSVEC(vector, func) \
DECLARE_IDTENTRY(vector, func)
+/**
+ * IDTENTRY_INVOKE_SYSVEC - Common code for system vector IDT entry points
+ * @what: What should be called
+ *
+ * Common code for DEFINE_IDTENTRY_SYSVEC and _SYSVEC_SIMPLE
+ */
+#define IDTENTRY_INVOKE_SYSVEC(what) \
+do { \
+ irqentry_state_t state = irqentry_enter(regs); \
+ \
+ instrumentation_begin(); \
+ kvm_set_cpu_l1tf_flush_l1d(); \
+ what; \
+ instrumentation_end(); \
+ irqentry_exit(regs, state); \
+} while (0) \
+
/**
* DEFINE_IDTENTRY_SYSVEC - Emit code for system vector IDT entry points
* @func: Function name of the entry point
@@ -233,13 +250,7 @@ static void __##func(struct pt_regs *regs); \
\
__visible noinstr void func(struct pt_regs *regs) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
- \
- instrumentation_begin(); \
- kvm_set_cpu_l1tf_flush_l1d(); \
- run_sysvec_on_irqstack_cond(__##func, regs); \
- instrumentation_end(); \
- irqentry_exit(regs, state); \
+ IDTENTRY_INVOKE_SYSVEC(run_sysvec_on_irqstack_cond(__##func, regs)); \
} \
\
static noinline void __##func(struct pt_regs *regs)
@@ -260,15 +271,7 @@ static __always_inline void __##func(struct pt_regs *regs); \
\
__visible noinstr void func(struct pt_regs *regs) \
{ \
- irqentry_state_t state = irqentry_enter(regs); \
- \
- instrumentation_begin(); \
- __irq_enter_raw(); \
- kvm_set_cpu_l1tf_flush_l1d(); \
- __##func (regs); \
- __irq_exit_raw(); \
- instrumentation_end(); \
- irqentry_exit(regs, state); \
+ IDTENTRY_INVOKE_SYSVEC(__irq_enter_raw(); __##func(regs); __irq_exit_raw()); \
} \
\
static __always_inline void __##func(struct pt_regs *regs)
--
2.31.1
From: "H. Peter Anvin (Intel)" <[email protected]>
The x86 architecture supports up to 32 trap vectors. Add that constant
to <asm/trapnr.h>.
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/include/asm/trapnr.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/include/asm/trapnr.h b/arch/x86/include/asm/trapnr.h
index f5d2325aa0b7..f0baf92da20b 100644
--- a/arch/x86/include/asm/trapnr.h
+++ b/arch/x86/include/asm/trapnr.h
@@ -27,6 +27,7 @@
#define X86_TRAP_VE 20 /* Virtualization Exception */
#define X86_TRAP_CP 21 /* Control Protection Exception */
#define X86_TRAP_VC 29 /* VMM Communication Exception */
+#define X86_NR_HW_TRAPS 32 /* Max hardware trap number */
#define X86_TRAP_IRET 32 /* IRET Exception */
#endif
--
2.31.1
From: "H. Peter Anvin (Intel)" <[email protected]>
There is no reason to specify any specific address to
idt_invalidate(). It looks mostly like an artifact of unifying code
done differently by accident.
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/include/asm/desc.h | 2 +-
arch/x86/kernel/idt.c | 5 ++---
arch/x86/kernel/machine_kexec_32.c | 4 ++--
arch/x86/kernel/reboot.c | 2 +-
4 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 476082a83d1c..b8429ae50b71 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -427,6 +427,6 @@ static inline void idt_setup_early_pf(void) { }
static inline void idt_setup_ist_traps(void) { }
#endif
-extern void idt_invalidate(void *addr);
+extern void idt_invalidate(void);
#endif /* _ASM_X86_DESC_H */
diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
index d552f177eca0..17f824462f5e 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -331,11 +331,10 @@ void __init idt_setup_early_handler(void)
/**
* idt_invalidate - Invalidate interrupt descriptor table
- * @addr: The virtual address of the 'invalid' IDT
*/
-void idt_invalidate(void *addr)
+void idt_invalidate(void)
{
- struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 };
+ struct desc_ptr idt = { .address = 0, .size = 0 };
load_idt(&idt);
}
diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
index 64b00b0d7fe8..6ba90f47d8c3 100644
--- a/arch/x86/kernel/machine_kexec_32.c
+++ b/arch/x86/kernel/machine_kexec_32.c
@@ -232,8 +232,8 @@ void machine_kexec(struct kimage *image)
* The gdt & idt are now invalid.
* If you want to load them you must set up your own idt & gdt.
*/
- idt_invalidate(phys_to_virt(0));
- set_gdt(phys_to_virt(0), 0);
+ idt_invalidate();
+ set_gdt(0, 0);
/* now call it */
image->start = relocate_kernel_ptr((unsigned long)image->head,
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index b29657b76e3f..ebfb91108232 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -669,7 +669,7 @@ static void native_machine_emergency_restart(void)
break;
case BOOT_TRIPLE:
- idt_invalidate(NULL);
+ idt_invalidate();
__asm__ __volatile__("int3");
/* We're probably dead after this, but... */
--
2.31.1
From: "H. Peter Anvin (Intel)" <[email protected]>
The current IRQ vector allocation code should be "clean" and never
issue a IRQ_MOVE_CLEANUP_VECTOR IPI for an interrupt that could still
be pending. This should make it possible to move it to the "normal"
system IRQ vector range. This should probably be a three-step process:
1. Introduce this WARN_ONCE() on this event ever occurring.
2. Move the IRQ_MOVE_CLEANUP_VECTOR to the sysvec range.
3. Remove the self-IPI hack.
This implements step 1.
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/kernel/apic/vector.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 6dbdc7c22bb7..7ba2982a3585 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -939,9 +939,14 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_irq_move_cleanup)
* to this CPU. IRQ_MOVE_CLEANUP_VECTOR is the lowest
* priority external vector, so on return from this
* interrupt the device interrupt will happen first.
+ *
+ * *** This should never happen with the current IRQ
+ * cleanup code, so WARN_ONCE() for now, and
+ * eventually get rid of this hack.
*/
irr = apic_read(APIC_IRR + (vector / 32 * 0x10));
if (irr & (1U << (vector % 32))) {
+ WARN_ONCE(1, "irq_move_cleanup called on still pending interrupt\n");
apic->send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
continue;
}
--
2.31.1
From: "H. Peter Anvin (Intel)" <[email protected]>
Add defines for the number of external vectors and number of system
vectors instead of requiring the use of (FIRST_SYSTEM_VECTOR -
FIRST_EXTERNAL_VECTOR) and (NR_VECTORS - FIRST_SYSTEM_VECTOR)
respectively.
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/include/asm/idtentry.h | 4 ++--
arch/x86/include/asm/irq_vectors.h | 3 +++
tools/arch/x86/include/asm/irq_vectors.h | 3 +++
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 73d45b0dfff2..c03a18cac78e 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -504,7 +504,7 @@ __visible noinstr void func(struct pt_regs *regs, \
.align 8
SYM_CODE_START(irq_entries_start)
vector=FIRST_EXTERNAL_VECTOR
- .rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
+ .rept NR_EXTERNAL_VECTORS
UNWIND_HINT_IRET_REGS
0 :
.byte 0x6a, vector
@@ -520,7 +520,7 @@ SYM_CODE_END(irq_entries_start)
.align 8
SYM_CODE_START(spurious_entries_start)
vector=FIRST_SYSTEM_VECTOR
- .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
+ .rept NR_SYSTEM_VECTORS
UNWIND_HINT_IRET_REGS
0 :
.byte 0x6a, vector
diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index 889f8b1b5b7f..d2ef35927770 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -114,6 +114,9 @@
#define FIRST_SYSTEM_VECTOR NR_VECTORS
#endif
+#define NR_EXTERNAL_VECTORS (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
+#define NR_SYSTEM_VECTORS (NR_VECTORS - FIRST_SYSTEM_VECTOR)
+
/*
* Size the maximum number of interrupts.
*
diff --git a/tools/arch/x86/include/asm/irq_vectors.h b/tools/arch/x86/include/asm/irq_vectors.h
index 889f8b1b5b7f..d2ef35927770 100644
--- a/tools/arch/x86/include/asm/irq_vectors.h
+++ b/tools/arch/x86/include/asm/irq_vectors.h
@@ -114,6 +114,9 @@
#define FIRST_SYSTEM_VECTOR NR_VECTORS
#endif
+#define NR_EXTERNAL_VECTORS (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
+#define NR_SYSTEM_VECTORS (NR_VECTORS - FIRST_SYSTEM_VECTOR)
+
/*
* Size the maximum number of interrupts.
*
--
2.31.1
From: "H. Peter Anvin (Intel)" <[email protected]>
UV_BAU_MESSAGE is defined but not used anywhere in the
kernel. Presumably this is a stale vector number that can be
reclaimed.
MCE_VECTOR is not an actual vector: #MC is an exception, not an
interrupt vector, and as such is correctly described as
X86_TRAP_MC. MCE_VECTOR is not used anywhere is the kernel.
Note that NMI_VECTOR *is* used; specifically it is the vector number
programmed into the APIC LVT when an NMI interrupt is configured. At
the moment it is always numerically identical to X86_TRAP_NMI, that is
not necessarily going to be the case indefinitely.
Cc: Steve Wahl <[email protected]>
Cc: Mike Travis <[email protected]>
Cc: Dimitri Sivanich <[email protected]>
Cc: Russ Anderson <[email protected]>
Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
---
arch/x86/include/asm/irq_vectors.h | 4 ++--
tools/arch/x86/include/asm/irq_vectors.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
index d2ef35927770..43dcb9284208 100644
--- a/arch/x86/include/asm/irq_vectors.h
+++ b/arch/x86/include/asm/irq_vectors.h
@@ -26,8 +26,8 @@
* This file enumerates the exact layout of them:
*/
+/* This is used as an interrupt vector when programming the APIC. */
#define NMI_VECTOR 0x02
-#define MCE_VECTOR 0x12
/*
* IDT vectors usable for external interrupt sources start at 0x20.
@@ -84,7 +84,7 @@
*/
#define IRQ_WORK_VECTOR 0xf6
-#define UV_BAU_MESSAGE 0xf5
+/* 0xf5 - unused, was UV_BAU_MESSAGE */
#define DEFERRED_ERROR_VECTOR 0xf4
/* Vector on which hypervisor callbacks will be delivered */
diff --git a/tools/arch/x86/include/asm/irq_vectors.h b/tools/arch/x86/include/asm/irq_vectors.h
index d2ef35927770..43dcb9284208 100644
--- a/tools/arch/x86/include/asm/irq_vectors.h
+++ b/tools/arch/x86/include/asm/irq_vectors.h
@@ -26,8 +26,8 @@
* This file enumerates the exact layout of them:
*/
+/* This is used as an interrupt vector when programming the APIC. */
#define NMI_VECTOR 0x02
-#define MCE_VECTOR 0x12
/*
* IDT vectors usable for external interrupt sources start at 0x20.
@@ -84,7 +84,7 @@
*/
#define IRQ_WORK_VECTOR 0xf6
-#define UV_BAU_MESSAGE 0xf5
+/* 0xf5 - unused, was UV_BAU_MESSAGE */
#define DEFERRED_ERROR_VECTOR 0xf4
/* Vector on which hypervisor callbacks will be delivered */
--
2.31.1
Hi Peter,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tip/x86/core]
[also build test WARNING on tip/master linus/master v5.13-rc1 next-20210510]
[cannot apply to bp/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/H-Peter-Anvin/x86-irq-trap-and-interrupt-cleanups/20210511-085650
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2c88d45edbb89029c1190bb3b136d2602f057c98
config: i386-randconfig-s002-20210511 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/c9c68d8ad1ef0923798c18f7e9a9570fa23aee0e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review H-Peter-Anvin/x86-irq-trap-and-interrupt-cleanups/20210511-085650
git checkout c9c68d8ad1ef0923798c18f7e9a9570fa23aee0e
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
sparse warnings: (new ones prefixed by >>)
>> arch/x86/kernel/machine_kexec_32.c:236:17: sparse: sparse: Using plain integer as NULL pointer
vim +236 arch/x86/kernel/machine_kexec_32.c
188
189 save_ftrace_enabled = __ftrace_enabled_save();
190
191 /* Interrupts aren't acceptable while we reboot */
192 local_irq_disable();
193 hw_breakpoint_disable();
194
195 if (image->preserve_context) {
196 #ifdef CONFIG_X86_IO_APIC
197 /*
198 * We need to put APICs in legacy mode so that we can
199 * get timer interrupts in second kernel. kexec/kdump
200 * paths already have calls to restore_boot_irq_mode()
201 * in one form or other. kexec jump path also need one.
202 */
203 clear_IO_APIC();
204 restore_boot_irq_mode();
205 #endif
206 }
207
208 control_page = page_address(image->control_code_page);
209 memcpy(control_page, relocate_kernel, KEXEC_CONTROL_CODE_MAX_SIZE);
210
211 relocate_kernel_ptr = control_page;
212 page_list[PA_CONTROL_PAGE] = __pa(control_page);
213 page_list[VA_CONTROL_PAGE] = (unsigned long)control_page;
214 page_list[PA_PGD] = __pa(image->arch.pgd);
215
216 if (image->type == KEXEC_TYPE_DEFAULT)
217 page_list[PA_SWAP_PAGE] = (page_to_pfn(image->swap_page)
218 << PAGE_SHIFT);
219
220 /*
221 * The segment registers are funny things, they have both a
222 * visible and an invisible part. Whenever the visible part is
223 * set to a specific selector, the invisible part is loaded
224 * with from a table in memory. At no other time is the
225 * descriptor table in memory accessed.
226 *
227 * I take advantage of this here by force loading the
228 * segments, before I zap the gdt with an invalid value.
229 */
230 load_segments();
231 /*
232 * The gdt & idt are now invalid.
233 * If you want to load them you must set up your own idt & gdt.
234 */
235 idt_invalidate();
> 236 set_gdt(0, 0);
237
238 /* now call it */
239 image->start = relocate_kernel_ptr((unsigned long)image->head,
240 (unsigned long)page_list,
241 image->start,
242 boot_cpu_has(X86_FEATURE_PAE),
243 image->preserve_context);
244
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
> diff --git a/arch/x86/kernel/machine_kexec_32.c b/arch/x86/kernel/machine_kexec_32.c
> index 64b00b0d7fe8..6ba90f47d8c3 100644
> --- a/arch/x86/kernel/machine_kexec_32.c
> +++ b/arch/x86/kernel/machine_kexec_32.c
> @@ -232,8 +232,8 @@ void machine_kexec(struct kimage *image)
> * The gdt & idt are now invalid.
> * If you want to load them you must set up your own idt & gdt.
> */
> - idt_invalidate(phys_to_virt(0));
> - set_gdt(phys_to_virt(0), 0);
> + idt_invalidate();
> + set_gdt(0, 0);
(NULL, 0)
first argument it a pointer...
On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
> +/**
> + * IDTENTRY_INVOKE_SYSVEC - Common code for system vector IDT entry points
> + * @what: What should be called
> + *
> + * Common code for DEFINE_IDTENTRY_SYSVEC and _SYSVEC_SIMPLE
> + */
> +#define IDTENTRY_INVOKE_SYSVEC(what) \
> +do { \
> + irqentry_state_t state = irqentry_enter(regs); \
> + \
> + instrumentation_begin(); \
> + kvm_set_cpu_l1tf_flush_l1d(); \
> + what; \
> + instrumentation_end(); \
> + irqentry_exit(regs, state); \
> +} while (0) \
> +
> /**
> * DEFINE_IDTENTRY_SYSVEC - Emit code for system vector IDT entry points
> * @func: Function name of the entry point
> @@ -233,13 +250,7 @@ static void __##func(struct pt_regs *regs); \
> \
> __visible noinstr void func(struct pt_regs *regs) \
> { \
> - irqentry_state_t state = irqentry_enter(regs); \
> - \
> - instrumentation_begin(); \
> - kvm_set_cpu_l1tf_flush_l1d(); \
> - run_sysvec_on_irqstack_cond(__##func, regs); \
> - instrumentation_end(); \
> - irqentry_exit(regs, state); \
> + IDTENTRY_INVOKE_SYSVEC(run_sysvec_on_irqstack_cond(__##func, regs)); \
> } \
> \
> static noinline void __##func(struct pt_regs *regs)
> @@ -260,15 +271,7 @@ static __always_inline void __##func(struct pt_regs *regs); \
> \
> __visible noinstr void func(struct pt_regs *regs) \
> { \
> - irqentry_state_t state = irqentry_enter(regs); \
> - \
> - instrumentation_begin(); \
> - __irq_enter_raw(); \
> - kvm_set_cpu_l1tf_flush_l1d(); \
> - __##func (regs); \
> - __irq_exit_raw(); \
> - instrumentation_end(); \
> - irqentry_exit(regs, state); \
> + IDTENTRY_INVOKE_SYSVEC(__irq_enter_raw(); __##func(regs); __irq_exit_raw()); \
That's not really making the code more readable. Something like the
below perhaps?
#define IDTENTRY_INVOKE_SYSVEC(func, regs, raw) \
do { \
irqentry_state_t state = irqentry_enter(regs); \
\
instrumentation_begin(); \
kvm_set_cpu_l1tf_flush_l1d(); \
if (raw) { \
__irq_enter_raw(); \
func(regs); \
__irq_exit_raw(); \
} else { \
run_sysvec_on_irqstack_cond(func, regs); \
} \
instrumentation_end(); \
irqentry_exit(regs, state); \
} while (0) \
Thanks,
tglx
On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <[email protected]>
>
> The current IRQ vector allocation code should be "clean" and never
> issue a IRQ_MOVE_CLEANUP_VECTOR IPI for an interrupt that could still
> be pending. This should make it possible to move it to the "normal"
> system IRQ vector range. This should probably be a three-step process:
>
> 1. Introduce this WARN_ONCE() on this event ever occurring.
> 2. Move the IRQ_MOVE_CLEANUP_VECTOR to the sysvec range.
> 3. Remove the self-IPI hack.
Actually 2+3 must be combined because _if_ this ever happens then the
self-IPI loops forever.
Thanks,
tglx
Yup, me bad.
On May 11, 2021 7:14:47 AM PDT, Thomas Gleixner <[email protected]> wrote:
>On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
>> diff --git a/arch/x86/kernel/machine_kexec_32.c
>b/arch/x86/kernel/machine_kexec_32.c
>> index 64b00b0d7fe8..6ba90f47d8c3 100644
>> --- a/arch/x86/kernel/machine_kexec_32.c
>> +++ b/arch/x86/kernel/machine_kexec_32.c
>> @@ -232,8 +232,8 @@ void machine_kexec(struct kimage *image)
>> * The gdt & idt are now invalid.
>> * If you want to load them you must set up your own idt & gdt.
>> */
>> - idt_invalidate(phys_to_virt(0));
>> - set_gdt(phys_to_virt(0), 0);
>> + idt_invalidate();
>> + set_gdt(0, 0);
>
> (NULL, 0)
>
>first argument it a pointer...
>
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
You are, of course, correct – or 2 and 3 can be reversed, I believe.
On May 11, 2021 7:23:53 AM PDT, Thomas Gleixner <[email protected]> wrote:
>On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
>> From: "H. Peter Anvin (Intel)" <[email protected]>
>>
>> The current IRQ vector allocation code should be "clean" and never
>> issue a IRQ_MOVE_CLEANUP_VECTOR IPI for an interrupt that could still
>> be pending. This should make it possible to move it to the "normal"
>> system IRQ vector range. This should probably be a three-step
>process:
>>
>> 1. Introduce this WARN_ONCE() on this event ever occurring.
>> 2. Move the IRQ_MOVE_CLEANUP_VECTOR to the sysvec range.
>> 3. Remove the self-IPI hack.
>
>Actually 2+3 must be combined because _if_ this ever happens then the
>self-IPI loops forever.
>
>Thanks,
>
> tglx
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Acked-by: Steve Wahl <[email protected]>
Yes, we believe UV_BAU_MESSAGE is one we missed in some earlier code
cleanup, and agree with its removal.
On Mon, May 10, 2021 at 05:55:31PM -0700, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <[email protected]>
>
> UV_BAU_MESSAGE is defined but not used anywhere in the
> kernel. Presumably this is a stale vector number that can be
> reclaimed.
>
> MCE_VECTOR is not an actual vector: #MC is an exception, not an
> interrupt vector, and as such is correctly described as
> X86_TRAP_MC. MCE_VECTOR is not used anywhere is the kernel.
>
> Note that NMI_VECTOR *is* used; specifically it is the vector number
> programmed into the APIC LVT when an NMI interrupt is configured. At
> the moment it is always numerically identical to X86_TRAP_NMI, that is
> not necessarily going to be the case indefinitely.
>
> Cc: Steve Wahl <[email protected]>
> Cc: Mike Travis <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Russ Anderson <[email protected]>
> Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
> ---
> arch/x86/include/asm/irq_vectors.h | 4 ++--
> tools/arch/x86/include/asm/irq_vectors.h | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/irq_vectors.h b/arch/x86/include/asm/irq_vectors.h
> index d2ef35927770..43dcb9284208 100644
> --- a/arch/x86/include/asm/irq_vectors.h
> +++ b/arch/x86/include/asm/irq_vectors.h
> @@ -26,8 +26,8 @@
> * This file enumerates the exact layout of them:
> */
>
> +/* This is used as an interrupt vector when programming the APIC. */
> #define NMI_VECTOR 0x02
> -#define MCE_VECTOR 0x12
>
> /*
> * IDT vectors usable for external interrupt sources start at 0x20.
> @@ -84,7 +84,7 @@
> */
> #define IRQ_WORK_VECTOR 0xf6
>
> -#define UV_BAU_MESSAGE 0xf5
> +/* 0xf5 - unused, was UV_BAU_MESSAGE */
> #define DEFERRED_ERROR_VECTOR 0xf4
>
> /* Vector on which hypervisor callbacks will be delivered */
> diff --git a/tools/arch/x86/include/asm/irq_vectors.h b/tools/arch/x86/include/asm/irq_vectors.h
> index d2ef35927770..43dcb9284208 100644
> --- a/tools/arch/x86/include/asm/irq_vectors.h
> +++ b/tools/arch/x86/include/asm/irq_vectors.h
> @@ -26,8 +26,8 @@
> * This file enumerates the exact layout of them:
> */
>
> +/* This is used as an interrupt vector when programming the APIC. */
> #define NMI_VECTOR 0x02
> -#define MCE_VECTOR 0x12
>
> /*
> * IDT vectors usable for external interrupt sources start at 0x20.
> @@ -84,7 +84,7 @@
> */
> #define IRQ_WORK_VECTOR 0xf6
>
> -#define UV_BAU_MESSAGE 0xf5
> +/* 0xf5 - unused, was UV_BAU_MESSAGE */
> #define DEFERRED_ERROR_VECTOR 0xf4
>
> /* Vector on which hypervisor callbacks will be delivered */
> --
> 2.31.1
>
--
Steve Wahl, Hewlett Packard Enterprise
On 5/11/21 7:22 AM, Thomas Gleixner wrote:
>
> That's not really making the code more readable. Something like the
> below perhaps?
>
> #define IDTENTRY_INVOKE_SYSVEC(func, regs, raw) \
> do { \
> irqentry_state_t state = irqentry_enter(regs); \
> \
> instrumentation_begin(); \
> kvm_set_cpu_l1tf_flush_l1d(); \
> if (raw) { \
> __irq_enter_raw(); \
> func(regs); \
> __irq_exit_raw(); \
> } else { \
> run_sysvec_on_irqstack_cond(func, regs); \
> } \
> instrumentation_end(); \
> irqentry_exit(regs, state); \
> } while (0) \
>
Digging more into it, it looks like a *lot* of the macros in
<asm/irq_stack.h> and <asm/idtentry.h> can be replaced with inlines
without any change in functionality or generated code.
-hpa
* H. Peter Anvin <[email protected]> wrote:
> On 5/11/21 7:22 AM, Thomas Gleixner wrote:
> >
> > That's not really making the code more readable. Something like the
> > below perhaps?
> >
> > #define IDTENTRY_INVOKE_SYSVEC(func, regs, raw) \
> > do { \
> > irqentry_state_t state = irqentry_enter(regs); \
> > \
> > instrumentation_begin(); \
> > kvm_set_cpu_l1tf_flush_l1d(); \
> > if (raw) { \
> > __irq_enter_raw(); \
> > func(regs); \
> > __irq_exit_raw(); \
> > } else { \
> > run_sysvec_on_irqstack_cond(func, regs); \
> > } \
> > instrumentation_end(); \
> > irqentry_exit(regs, state); \
> > } while (0) \
> >
>
> Digging more into it, it looks like a *lot* of the macros in
> <asm/irq_stack.h> and <asm/idtentry.h> can be replaced with inlines without
> any change in functionality or generated code.
That would be a much preferred outcome ...
Thanks,
Ingo
On 5/12/21 1:38 AM, Ingo Molnar wrote:
>
> * H. Peter Anvin <[email protected]> wrote:
>
>> On 5/11/21 7:22 AM, Thomas Gleixner wrote:
>>>
>>> That's not really making the code more readable. Something like the
>>> below perhaps?
>>>
>>> #define IDTENTRY_INVOKE_SYSVEC(func, regs, raw) \
>>> do { \
>>> irqentry_state_t state = irqentry_enter(regs); \
>>> \
>>> instrumentation_begin(); \
>>> kvm_set_cpu_l1tf_flush_l1d(); \
>>> if (raw) { \
>>> __irq_enter_raw(); \
>>> func(regs); \
>>> __irq_exit_raw(); \
>>> } else { \
>>> run_sysvec_on_irqstack_cond(func, regs); \
>>> } \
>>> instrumentation_end(); \
>>> irqentry_exit(regs, state); \
>>> } while (0) \
>>>
>>
>> Digging more into it, it looks like a *lot* of the macros in
>> <asm/irq_stack.h> and <asm/idtentry.h> can be replaced with inlines without
>> any change in functionality or generated code.
>
> That would be a much preferred outcome ...
Well, here is an RFC. This is obviously a much bigger change and I don't
feel it is mature yet, but it would be good to know if you (plural) feel
it is in the right direction.
I will clean up the rest of the patchset and resubmit.
-hpa
* H. Peter Anvin <[email protected]> wrote:
>
>
> On 5/12/21 1:38 AM, Ingo Molnar wrote:
> >
> > * H. Peter Anvin <[email protected]> wrote:
> >
> > > On 5/11/21 7:22 AM, Thomas Gleixner wrote:
> > > >
> > > > That's not really making the code more readable. Something like the
> > > > below perhaps?
> > > >
> > > > #define IDTENTRY_INVOKE_SYSVEC(func, regs, raw) \
> > > > do { \
> > > > irqentry_state_t state = irqentry_enter(regs); \
> > > > \
> > > > instrumentation_begin(); \
> > > > kvm_set_cpu_l1tf_flush_l1d(); \
> > > > if (raw) { \
> > > > __irq_enter_raw(); \
> > > > func(regs); \
> > > > __irq_exit_raw(); \
> > > > } else { \
> > > > run_sysvec_on_irqstack_cond(func, regs); \
> > > > } \
> > > > instrumentation_end(); \
> > > > irqentry_exit(regs, state); \
> > > > } while (0) \
> > > >
> > >
> > > Digging more into it, it looks like a *lot* of the macros in
> > > <asm/irq_stack.h> and <asm/idtentry.h> can be replaced with inlines without
> > > any change in functionality or generated code.
> >
> > That would be a much preferred outcome ...
>
> Well, here is an RFC. This is obviously a much bigger change and I don't
> feel it is mature yet, but it would be good to know if you (plural) feel it
> is in the right direction.
Looks much cleaner IMO, and there's also some linecount reduction:
> 7 files changed, 117 insertions(+), 142 deletions(-)
Thanks,
Ingo
On 5/10/21 5:55 PM, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <[email protected]>
>
> Add defines for the number of external vectors and number of system
> vectors instead of requiring the use of (FIRST_SYSTEM_VECTOR -
> FIRST_EXTERNAL_VECTOR) and (NR_VECTORS - FIRST_SYSTEM_VECTOR)
> respectively.
Acked-by: Andy Lutomirski <[email protected]>
On 5/10/21 5:55 PM, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <[email protected]>
>
> The x86 architecture supports up to 32 trap vectors. Add that constant
> to <asm/trapnr.h>.
>
> Signed-off-by: H. Peter Anvin (Intel) <[email protected]>
> ---
> arch/x86/include/asm/trapnr.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/include/asm/trapnr.h b/arch/x86/include/asm/trapnr.h
> index f5d2325aa0b7..f0baf92da20b 100644
> --- a/arch/x86/include/asm/trapnr.h
> +++ b/arch/x86/include/asm/trapnr.h
> @@ -27,6 +27,7 @@
> #define X86_TRAP_VE 20 /* Virtualization Exception */
> #define X86_TRAP_CP 21 /* Control Protection Exception */
> #define X86_TRAP_VC 29 /* VMM Communication Exception */
> +#define X86_NR_HW_TRAPS 32 /* Max hardware trap number */
> #define X86_TRAP_IRET 32 /* IRET Exception */
>
> #endif
>
Acked-by: Andy Lutomirski <[email protected]>