[PATCH 0/6] x86/irq: trap and interrupt cleanups

[PATCH 0/6] x86/irq: trap and interrupt cleanups

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

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(-)

[PATCH 1/6] x86/traps: add X86_NR_HW_TRAPS to <asm/trapnr.h>

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

The x86 architecture supports up to 32 trap vectors. Add that constant
to <asm/trapnr.h>.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
---
 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

[PATCH 2/6] x86/irqvector: add NR_EXTERNAL_VECTORS and NR_SYSTEM_VECTORS

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

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) <hpa@zytor.com>
---
 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

[PATCH 3/6] x86/idt: remove address argument to idt_invalidate()

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

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) <hpa@zytor.com>
---
 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

[PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

Move the common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE] into a common
macro.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
---
 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

[PATCH 5/6] x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

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 <tglx@linutronix.de>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
---
 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

[PATCH 6/6] x86/irq: remove unused vectors from <asm/irq_vectors.h>

[H. Peter Anvin]

From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

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 <steve.wahl@hpe.com>
Cc: Mike Travis <mike.travis@hpe.com>
Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
Cc: Russ Anderson <russ.anderson@hpe.com>
Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
---
 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

Re: [PATCH 3/6] x86/idt: remove address argument to idt_invalidate()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

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 <lkp@intel.com>


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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35108 bytes --]

Re: [PATCH 3/6] x86/idt: remove address argument to idt_invalidate()

[Thomas Gleixner]

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...
  

Re: [PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[Thomas Gleixner]

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

Re: [PATCH 5/6] x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt

[Thomas Gleixner]

On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>
> 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

Re: [PATCH 3/6] x86/idt: remove address argument to idt_invalidate()

[H. Peter Anvin]

Yup, me bad.

On May 11, 2021 7:14:47 AM PDT, Thomas Gleixner <tglx@linutronix.de> 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.

Re: [PATCH 5/6] x86/irq: WARN_ONCE() if irq_move_cleanup is called on a pending interrupt

[H. Peter Anvin]

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 <tglx@linutronix.de> wrote:
>On Mon, May 10 2021 at 17:55, H. Peter Anvin wrote:
>> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>>
>> 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.

Re: [PATCH 6/6] x86/irq: remove unused vectors from <asm/irq_vectors.h>

[Steve Wahl]

Acked-by: Steve Wahl <steve.wahl@hpe.com>

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)" <hpa@zytor.com>
> 
> 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 <steve.wahl@hpe.com>
> Cc: Mike Travis <mike.travis@hpe.com>
> Cc: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> Cc: Russ Anderson <russ.anderson@hpe.com>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> ---
>  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

Re: [PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[H. Peter Anvin]

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

Re: [PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> 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

Re: [PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]



On 5/12/21 1:38 AM, Ingo Molnar wrote:
> 
> * H. Peter Anvin <hpa@zytor.com> 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


[-- Attachment #2: 0001-x86-irq-merge-and-functionalize-common-code-in-DECLA.patch --]
[-- Type: text/x-patch, Size: 17742 bytes --]

From e52200ed187e30f45d32639612e09281a544c062 Mon Sep 17 00:00:00 2001
From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
Date: Wed, 5 May 2021 17:43:46 -0700
Subject: [PATCH] x86/irq: merge and functionalize common code in
 DECLARE/DEFINE_IDTENTRY_*

Move as much of the common code in the _IDTENTRY_ and
run_*_on_irq_stack() into inline functions instead of macros. A lot of
them differ only in types and/or the presence or absence of an
additional argument; the differential amount of code is neglible and
after bending the types a little bit, they unify nicely.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
---
 arch/x86/entry/common.c          |   5 +-
 arch/x86/include/asm/idtentry.h  | 170 +++++++++++++++----------------
 arch/x86/include/asm/irq_stack.h |  73 +++++--------
 arch/x86/kernel/apic/apic.c      |   2 +-
 arch/x86/kernel/irq.c            |   1 +
 arch/x86/kernel/sev-es.c         |   6 +-
 arch/x86/kernel/traps.c          |   2 +-
 7 files changed, 117 insertions(+), 142 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 7b2542b13ebd..1e46a1a35b20 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -253,7 +253,8 @@ static __always_inline bool get_and_clear_inhcall(void) { return false; }
 static __always_inline void restore_inhcall(bool inhcall) { }
 #endif
 
-static void __xen_pv_evtchn_do_upcall(struct pt_regs *regs)
+static void __xen_pv_evtchn_do_upcall(struct pt_regs *regs,
+				      u32 __dummy __always_unused)
 {
 	struct pt_regs *old_regs = set_irq_regs(regs);
 
@@ -269,7 +270,7 @@ __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
 	irqentry_state_t state = irqentry_enter(regs);
 	bool inhcall;
 
-	run_sysvec_on_irqstack_cond(__xen_pv_evtchn_do_upcall, regs);
+	run_on_irqstack_cond(__xen_pv_evtchn_do_upcall, regs, 0);
 
 	inhcall = get_and_clear_inhcall();
 	if (inhcall && !WARN_ON_ONCE(state.exit_rcu)) {
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index c03a18cac78e..85f79a429a39 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -11,6 +11,65 @@
 
 #include <asm/irq_stack.h>
 
+/**
+ * idtentry_call - Common code for non-trivial IDT entry points
+ * @func:	What should ultimately be called
+ * @regs:	Pointer to entry struct pt_regs
+ * @arg:	Extra argument to func (vector, error_code)
+ * @flush:	If kvm_set_cpu_l1tf_flush_l1d() should be called
+ * @wrapper:	Wrapper to call func; NULL for just call
+ */
+static __always_inline void
+idtentry_call(irq_func_t func, struct pt_regs *regs, bool flush,
+	      void (*wrapper)(irq_func_t, struct pt_regs *, u32),
+	      u32 arg)
+{
+	irqentry_state_t state = irqentry_enter(regs);
+
+	instrumentation_begin();
+	if (flush)
+		kvm_set_cpu_l1tf_flush_l1d();
+	if (wrapper)
+		wrapper(func, regs, arg);
+	else
+		func(regs, arg);
+	instrumentation_end();
+	irqentry_exit(regs, state);
+}
+
+/**
+ * _DEFINE_IDTENTRY - Common macro for defining idtentries with no argument
+ * @func:	Function name of the entry point
+ * @flush:	If wrapper should call kvm_set_cpu_l1tf_flush_l1d()
+ * @inline_opt: __always_inline or noinline as appropriate for __func
+ * @wrapper:	Wrapper function for calling __func
+ *
+ */
+#define _DEFINE_IDTENTRY(func, flush, inline_opt, wrapper)		\
+static inline_opt void __##func(struct pt_regs *regs, u32);		\
+__visible noinstr void func(struct pt_regs *regs)			\
+{									\
+	idtentry_call(__##func, regs, flush, wrapper, 0);		\
+}									\
+static inline_opt void							\
+__##func(struct pt_regs *regs, u32 __dummy __always_unused)
+
+/**
+ * _DEFINE_IDTENTRY_ERRORCODE - Common macro for defining idtentries with argument
+ * @func:	Function name of the entry point
+ * @flush:	If wrapper should call kvm_set_cpu_l1tf_flush_l1d()
+ * @inline_opt: __always_inline or noinline as appropriate for __func
+ * @wrapper:	Wrapper function for calling __func
+ *
+ */
+#define _DEFINE_IDTENTRY_ERRORCODE(func, flush, inline_opt, wrapper)	\
+static inline_opt void __##func(struct pt_regs *regs, u32 error_code);	\
+__visible noinstr void func(struct pt_regs *regs, u32 error_code)	\
+{									\
+	idtentry_call(__##func, regs, flush, wrapper, error_code);	\
+}									\
+static inline_opt void __##func(struct pt_regs *regs, u32 error_code)
+
 /**
  * DECLARE_IDTENTRY - Declare functions for simple IDT entry points
  *		      No error code pushed by hardware
@@ -45,19 +104,7 @@
  * which has to run before returning to the low level assembly code.
  */
 #define DEFINE_IDTENTRY(func)						\
-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();					\
-	__##func (regs);						\
-	instrumentation_end();						\
-	irqentry_exit(regs, state);					\
-}									\
-									\
-static __always_inline void __##func(struct pt_regs *regs)
+	_DEFINE_IDTENTRY(func, false, __always_inline, NULL)
 
 /* Special case for 32bit IRET 'trap' */
 #define DECLARE_IDTENTRY_SW	DECLARE_IDTENTRY
@@ -80,7 +127,7 @@ static __always_inline void __##func(struct pt_regs *regs)
 #define DECLARE_IDTENTRY_ERRORCODE(vector, func)			\
 	asmlinkage void asm_##func(void);				\
 	asmlinkage void xen_asm_##func(void);				\
-	__visible void func(struct pt_regs *regs, unsigned long error_code)
+	__visible void func(struct pt_regs *regs, u32 error_code)
 
 /**
  * DEFINE_IDTENTRY_ERRORCODE - Emit code for simple IDT entry points
@@ -90,22 +137,7 @@ static __always_inline void __##func(struct pt_regs *regs)
  * Same as DEFINE_IDTENTRY, but has an extra error_code argument
  */
 #define DEFINE_IDTENTRY_ERRORCODE(func)					\
-static __always_inline void __##func(struct pt_regs *regs,		\
-				     unsigned long error_code);		\
-									\
-__visible noinstr void func(struct pt_regs *regs,			\
-			    unsigned long error_code)			\
-{									\
-	irqentry_state_t state = irqentry_enter(regs);			\
-									\
-	instrumentation_begin();					\
-	__##func (regs, error_code);					\
-	instrumentation_end();						\
-	irqentry_exit(regs, state);					\
-}									\
-									\
-static __always_inline void __##func(struct pt_regs *regs,		\
-				     unsigned long error_code)
+	_DEFINE_IDTENTRY_ERRORCODE(func, false, __always_inline, NULL)
 
 /**
  * DECLARE_IDTENTRY_RAW - Declare functions for raw IDT entry points
@@ -161,7 +193,7 @@ __visible noinstr void func(struct pt_regs *regs)
  * is required before the enter/exit() helpers are invoked.
  */
 #define DEFINE_IDTENTRY_RAW_ERRORCODE(func)				\
-__visible noinstr void func(struct pt_regs *regs, unsigned long error_code)
+__visible noinstr void func(struct pt_regs *regs, u32 error_code)
 
 /**
  * DECLARE_IDTENTRY_IRQ - Declare functions for device interrupt IDT entry
@@ -187,25 +219,11 @@ __visible noinstr void func(struct pt_regs *regs, unsigned long error_code)
  * has to be done in the function body if necessary.
  */
 #define DEFINE_IDTENTRY_IRQ(func)					\
-static void __##func(struct pt_regs *regs, u32 vector);			\
-									\
-__visible noinstr void func(struct pt_regs *regs,			\
-			    unsigned long error_code)			\
-{									\
-	irqentry_state_t state = irqentry_enter(regs);			\
-	u32 vector = (u32)(u8)error_code;				\
-									\
-	instrumentation_begin();					\
-	kvm_set_cpu_l1tf_flush_l1d();					\
-	run_irq_on_irqstack_cond(__##func, regs, vector);		\
-	instrumentation_end();						\
-	irqentry_exit(regs, state);					\
-}									\
-									\
-static noinline void __##func(struct pt_regs *regs, u32 vector)
+	_DEFINE_IDTENTRY_ERRORCODE(func, true, noinline, run_on_irqstack_cond)
 
 /**
- * DECLARE_IDTENTRY_SYSVEC - Declare functions for system vector entry points
+ * DECLARE_IDTENTRY - Declare functions for simple IDT entry points
+ *		      No error code pushed by hardware
  * @vector:	Vector number (ignored for C)
  * @func:	Function name of the entry point
  *
@@ -214,9 +232,11 @@ static noinline void __##func(struct pt_regs *regs, u32 vector)
  * - The XEN PV trap entry point: xen_##func (maybe unused)
  * - The C handler called from the ASM entry point
  *
- * Maps to DECLARE_IDTENTRY().
+ * Note: This is the C variant of DECLARE_IDTENTRY(). As the name says it
+ * declares the entry points for usage in C code. There is an ASM variant
+ * as well which is used to emit the entry stubs in entry_32/64.S.
  */
-#define DECLARE_IDTENTRY_SYSVEC(vector, func)				\
+#define DECLARE_IDTENTRY_SYSVEC(vector, func)	\
 	DECLARE_IDTENTRY(vector, func)
 
 /**
@@ -229,20 +249,7 @@ static noinline void __##func(struct pt_regs *regs, u32 vector)
  * Runs the function on the interrupt stack if the entry hit kernel mode
  */
 #define DEFINE_IDTENTRY_SYSVEC(func)					\
-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);					\
-}									\
-									\
-static noinline void __##func(struct pt_regs *regs)
+	_DEFINE_IDTENTRY(func, true, noinline, run_on_irqstack_cond)
 
 /**
  * DEFINE_IDTENTRY_SYSVEC_SIMPLE - Emit code for simple system vector IDT
@@ -255,23 +262,16 @@ static noinline void __##func(struct pt_regs *regs)
  * Only use for 'empty' vectors like reschedule IPI and KVM posted
  * interrupt vectors.
  */
+static __always_inline void
+call_sysvec_simple(irq_func_t func, struct pt_regs *regs, u32 arg)
+{
+	__irq_enter_raw();
+	func(regs, arg);
+	__irq_exit_raw();
+}
+
 #define DEFINE_IDTENTRY_SYSVEC_SIMPLE(func)				\
-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);					\
-}									\
-									\
-static __always_inline void __##func(struct pt_regs *regs)
+	_DEFINE_IDTENTRY(func, true, __always_inline, call_sysvec_simple)
 
 /**
  * DECLARE_IDTENTRY_XENCB - Declare functions for XEN HV callback entry point
@@ -312,8 +312,8 @@ static __always_inline void __##func(struct pt_regs *regs)
  */
 #define DECLARE_IDTENTRY_VC(vector, func)				\
 	DECLARE_IDTENTRY_RAW_ERRORCODE(vector, func);			\
-	__visible noinstr void ist_##func(struct pt_regs *regs, unsigned long error_code);	\
-	__visible noinstr void safe_stack_##func(struct pt_regs *regs, unsigned long error_code)
+	__visible noinstr void ist_##func(struct pt_regs *regs, u32 error_code);	\
+	__visible noinstr void safe_stack_##func(struct pt_regs *regs, u32 error_code)
 
 /**
  * DEFINE_IDTENTRY_IST - Emit code for IST entry points
@@ -396,8 +396,7 @@ static __always_inline void __##func(struct pt_regs *regs)
  */
 #define DECLARE_IDTENTRY_DF(vector, func)				\
 	asmlinkage void asm_##func(void);				\
-	__visible void func(struct pt_regs *regs,			\
-			    unsigned long error_code,			\
+	__visible void func(struct pt_regs *regs, u32 error_code,	\
 			    unsigned long address)
 
 /**
@@ -408,8 +407,7 @@ static __always_inline void __##func(struct pt_regs *regs)
  * cr2 in the address argument.
  */
 #define DEFINE_IDTENTRY_DF(func)					\
-__visible noinstr void func(struct pt_regs *regs,			\
-			    unsigned long error_code,			\
+__visible noinstr void func(struct pt_regs *regs, u32 error_code,	\
 			    unsigned long address)
 
 #endif	/* !CONFIG_X86_64 */
diff --git a/arch/x86/include/asm/irq_stack.h b/arch/x86/include/asm/irq_stack.h
index 562854c60808..305ca95b2743 100644
--- a/arch/x86/include/asm/irq_stack.h
+++ b/arch/x86/include/asm/irq_stack.h
@@ -6,6 +6,8 @@
 
 #include <asm/processor.h>
 
+typedef void (*irq_func_t)(struct pt_regs *regs, u32 arg);
+
 #ifdef CONFIG_X86_64
 
 /*
@@ -132,7 +134,7 @@
 }
 
 /*
- * Function call sequence for __call_on_irqstack() for system vectors.
+ * Function call sequence for __call_on_irqstack() for irqs and system vectors.
  *
  * Note that irq_enter_rcu() and irq_exit_rcu() do not use the input
  * mechanism because these functions are global and cannot be optimized out
@@ -145,27 +147,6 @@
  * call to idtentry_exit() anyway, it's likely that it does not cause extra
  * effort for this asm magic.
  */
-#define ASM_CALL_SYSVEC							\
-	"call irq_enter_rcu				\n"		\
-	"movq	%[arg1], %%rdi				\n"		\
-	"call %P[__func]				\n"		\
-	"call irq_exit_rcu				\n"
-
-#define SYSVEC_CONSTRAINTS	, [arg1] "r" (regs)
-
-#define run_sysvec_on_irqstack_cond(func, regs)				\
-{									\
-	assert_function_type(func, void (*)(struct pt_regs *));		\
-	assert_arg_type(regs, struct pt_regs *);			\
-									\
-	call_on_irqstack_cond(func, regs, ASM_CALL_SYSVEC,		\
-			      SYSVEC_CONSTRAINTS, regs);		\
-}
-
-/*
- * As in ASM_CALL_SYSVEC above the clobbers force the compiler to store
- * @regs and @vector in callee saved registers.
- */
 #define ASM_CALL_IRQ							\
 	"call irq_enter_rcu				\n"		\
 	"movq	%[arg1], %%rdi				\n"		\
@@ -173,16 +154,13 @@
 	"call %P[__func]				\n"		\
 	"call irq_exit_rcu				\n"
 
-#define IRQ_CONSTRAINTS	, [arg1] "r" (regs), [arg2] "r" (vector)
+#define IRQ_CONSTRAINTS	, [arg1] "r" (regs), [arg2] "r" (arg)
 
-#define run_irq_on_irqstack_cond(func, regs, vector)			\
-{									\
-	assert_function_type(func, void (*)(struct pt_regs *, u32));	\
-	assert_arg_type(regs, struct pt_regs *);			\
-	assert_arg_type(vector, u32);					\
-									\
-	call_on_irqstack_cond(func, regs, ASM_CALL_IRQ,			\
-			      IRQ_CONSTRAINTS, regs, vector);		\
+static __always_inline void
+run_on_irqstack_cond(irq_func_t func, struct pt_regs *regs, u32 arg)
+{
+	call_on_irqstack_cond(func, regs, ASM_CALL_IRQ,
+			      IRQ_CONSTRAINTS, regs, arg);
 }
 
 #define ASM_CALL_SOFTIRQ						\
@@ -194,28 +172,25 @@
  * interrupts are pending to be processed. The interrupt stack cannot be in
  * use here.
  */
-#define do_softirq_own_stack()						\
-{									\
-	__this_cpu_write(hardirq_stack_inuse, true);			\
-	call_on_irqstack(__do_softirq, ASM_CALL_SOFTIRQ);		\
-	__this_cpu_write(hardirq_stack_inuse, false);			\
+static __always_inline void do_softirq_own_stack(void)
+{
+	__this_cpu_write(hardirq_stack_inuse, true);
+	call_on_irqstack(__do_softirq, ASM_CALL_SOFTIRQ);
+	__this_cpu_write(hardirq_stack_inuse, false);
 }
 
 #else /* CONFIG_X86_64 */
-/* System vector handlers always run on the stack they interrupted. */
-#define run_sysvec_on_irqstack_cond(func, regs)				\
-{									\
-	irq_enter_rcu();						\
-	func(regs);							\
-	irq_exit_rcu();							\
-}
 
-/* Switches to the irq stack within func() */
-#define run_irq_on_irqstack_cond(func, regs, vector)			\
-{									\
-	irq_enter_rcu();						\
-	func(regs, vector);						\
-	irq_exit_rcu();							\
+/*
+ * System vector handlers always run on the stack they interrupted;
+ * irqs switch to the irq stack withing func().
+ */
+static __always_inline void
+run_on_irqstack_cond(irq_func_t func, struct pt_regs *regs, u32 arg)
+{
+	irq_enter_rcu();
+	func(regs, arg);
+	irq_exit_rcu();
 }
 
 #endif /* !CONFIG_X86_64 */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4a39fb429f15..64832869e1a1 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2188,7 +2188,7 @@ static noinline void handle_spurious_interrupt(u8 vector)
  */
 DEFINE_IDTENTRY_IRQ(spurious_interrupt)
 {
-	handle_spurious_interrupt(vector);
+	handle_spurious_interrupt((u8)error_code);
 }
 
 DEFINE_IDTENTRY_SYSVEC(sysvec_spurious_apic_interrupt)
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index e28f6a5d14f1..27994818d9b1 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -239,6 +239,7 @@ static __always_inline void handle_irq(struct irq_desc *desc,
  */
 DEFINE_IDTENTRY_IRQ(common_interrupt)
 {
+	const u8 vector = error_code;
 	struct pt_regs *old_regs = set_irq_regs(regs);
 	struct irq_desc *desc;
 
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c
index 73873b007838..bdd75b71d0f3 100644
--- a/arch/x86/kernel/sev-es.c
+++ b/arch/x86/kernel/sev-es.c
@@ -1335,15 +1335,15 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
 		vc_finish_insn(&ctxt);
 		break;
 	case ES_UNSUPPORTED:
-		pr_err_ratelimited("Unsupported exit-code 0x%02lx in early #VC exception (IP: 0x%lx)\n",
+		pr_err_ratelimited("Unsupported exit-code 0x%02x in early #VC exception (IP: 0x%lx)\n",
 				   error_code, regs->ip);
 		goto fail;
 	case ES_VMM_ERROR:
-		pr_err_ratelimited("Failure in communication with VMM (exit-code 0x%02lx IP: 0x%lx)\n",
+		pr_err_ratelimited("Failure in communication with VMM (exit-code 0x%02x IP: 0x%lx)\n",
 				   error_code, regs->ip);
 		goto fail;
 	case ES_DECODE_FAILED:
-		pr_err_ratelimited("Failed to decode instruction (exit-code 0x%02lx IP: 0x%lx)\n",
+		pr_err_ratelimited("Failed to decode instruction (exit-code 0x%02x IP: 0x%lx)\n",
 				   error_code, regs->ip);
 		goto fail;
 	case ES_EXCEPTION:
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 853ea7a80806..f5aecbb44bc7 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -461,7 +461,7 @@ DEFINE_IDTENTRY_DF(exc_double_fault)
 	}
 #endif
 
-	pr_emerg("PANIC: double fault, error_code: 0x%lx\n", error_code);
+	pr_emerg("PANIC: double fault, error_code: 0x%x\n", error_code);
 	die("double fault", regs, error_code);
 	panic("Machine halted.");
 	instrumentation_end();
-- 
2.31.1

Re: [PATCH 4/6] x86/irq: merge common code in DEFINE_IDTENTRY_SYSVEC[_SIMPLE]

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> 
> 
> On 5/12/21 1:38 AM, Ingo Molnar wrote:
> > 
> > * H. Peter Anvin <hpa@zytor.com> 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

Re: [PATCH 1/6] x86/traps: add X86_NR_HW_TRAPS to <asm/trapnr.h>

[Andy Lutomirski]

On 5/10/21 5:55 PM, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
> 
> The x86 architecture supports up to 32 trap vectors. Add that constant
> to <asm/trapnr.h>.
> 
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> ---
>  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 <luto@kernel.org>

Re: [PATCH 2/6] x86/irqvector: add NR_EXTERNAL_VECTORS and NR_SYSTEM_VECTORS

[Andy Lutomirski]

On 5/10/21 5:55 PM, H. Peter Anvin wrote:
> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
> 
> 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 <luto@kernel.org>