Logic concerning the IDT is somewhat different to the other system tables, and
in particular ought not to be in asm/processor.h. Collect it together a new
header.
While doing so, make a few minor adjustments:
* Make set_ist() use volatile rather than ACCESS_ONCE(), as
_write_gate_lower() already does, removing the need for xen/lib.h.
* Move the BUILD_BUG_ON() from subarch_percpu_traps_init() into mm.c's
build_assertions(), rather than including idt.h into x86_64/traps.c.
* Drop UL from IST constants.
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>
---
xen/arch/x86/cpu/common.c | 1 +
xen/arch/x86/crash.c | 1 +
xen/arch/x86/domain.c | 1 +
xen/arch/x86/hvm/svm/svm.c | 1 +
xen/arch/x86/hvm/vmx/vmcs.c | 1 +
xen/arch/x86/include/asm/desc.h | 76 ----------------
xen/arch/x86/include/asm/idt.h | 125 +++++++++++++++++++++++++++
xen/arch/x86/include/asm/processor.h | 37 --------
xen/arch/x86/machine_kexec.c | 1 +
xen/arch/x86/mm.c | 4 +
xen/arch/x86/pv/traps.c | 1 +
xen/arch/x86/smpboot.c | 1 +
xen/arch/x86/traps.c | 1 +
xen/arch/x86/x86_64/traps.c | 3 -
14 files changed, 138 insertions(+), 116 deletions(-)
create mode 100644 xen/arch/x86/include/asm/idt.h
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 1cc4adccb471..1540ab0007a0 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -9,6 +9,7 @@
#include <asm/cpu-policy.h>
#include <asm/current.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/mpspec.h>
#include <asm/msr.h>
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 4afe0ad859a7..5f7d7b392a1f 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -26,6 +26,7 @@
#include <asm/atomic.h>
#include <asm/elf.h>
#include <asm/hpet.h>
+#include <asm/idt.h>
#include <asm/io_apic.h>
#include <asm/nmi.h>
#include <asm/shared.h>
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7b2549091fd3..d3db76833f3c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -47,6 +47,7 @@
#include <asm/hvm/svm/svm.h>
#include <asm/hvm/viridian.h>
#include <asm/i387.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/ldt.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 62905c2c7acd..ea78da4f4210 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -18,6 +18,7 @@
#include <asm/cpufeature.h>
#include <asm/current.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/gdbsx.h>
#include <asm/hvm/emulate.h>
#include <asm/hvm/hvm.h>
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index fa9d8b3267ea..0136830ebcb7 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -25,6 +25,7 @@
#include <asm/hvm/vmx/vmcs.h>
#include <asm/hvm/vmx/vmx.h>
#include <asm/hvm/vmx/vvmx.h>
+#include <asm/idt.h>
#include <asm/monitor.h>
#include <asm/msr.h>
#include <asm/processor.h>
diff --git a/xen/arch/x86/include/asm/desc.h b/xen/arch/x86/include/asm/desc.h
index a1e0807d97ed..85fae6b2f9ae 100644
--- a/xen/arch/x86/include/asm/desc.h
+++ b/xen/arch/x86/include/asm/desc.h
@@ -115,82 +115,6 @@ typedef union {
};
} seg_desc_t;
-typedef union {
- struct {
- uint64_t a, b;
- };
- struct {
- uint16_t addr0;
- uint16_t cs;
- uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
- uint8_t type:4, s:1, dpl:2, p:1;
- uint16_t addr1;
- uint32_t addr2;
- /* 32 bits rsvd. */
- };
-} idt_entry_t;
-
-/* Write the lower 64 bits of an IDT Entry. This relies on the upper 32
- * bits of the address not changing, which is a safe assumption as all
- * functions we are likely to load will live inside the 1GB
- * code/data/bss address range.
- *
- * Ideally, we would use cmpxchg16b, but this is not supported on some
- * old AMD 64bit capable processors, and has no safe equivalent.
- */
-static inline void _write_gate_lower(volatile idt_entry_t *gate,
- const idt_entry_t *new)
-{
- ASSERT(gate->b == new->b);
- gate->a = new->a;
-}
-
-#define _set_gate(gate_addr,type,dpl,addr) \
-do { \
- (gate_addr)->a = 0; \
- smp_wmb(); /* disable gate /then/ rewrite */ \
- (gate_addr)->b = \
- ((unsigned long)(addr) >> 32); \
- smp_wmb(); /* rewrite /then/ enable gate */ \
- (gate_addr)->a = \
- (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
- ((unsigned long)(dpl) << 45) | \
- ((unsigned long)(type) << 40) | \
- ((unsigned long)(addr) & 0xFFFFUL) | \
- ((unsigned long)__HYPERVISOR_CS << 16) | \
- (1UL << 47); \
-} while (0)
-
-static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
- unsigned long dpl, void *addr)
-{
- idt_entry_t idte;
- idte.b = gate->b;
- idte.a =
- (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
- ((unsigned long)(dpl) << 45) |
- ((unsigned long)(type) << 40) |
- ((unsigned long)(addr) & 0xFFFFUL) |
- ((unsigned long)__HYPERVISOR_CS << 16) |
- (1UL << 47);
- _write_gate_lower(gate, &idte);
-}
-
-/* Update the lower half handler of an IDT Entry, without changing any
- * other configuration. */
-static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
-{
- idt_entry_t idte;
- idte.a = gate->a;
-
- idte.b = ((unsigned long)(addr) >> 32);
- idte.a &= 0x0000FFFFFFFF0000ULL;
- idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
- ((unsigned long)(addr) & 0xFFFFUL);
-
- _write_gate_lower(gate, &idte);
-}
-
#define _set_tssldt_desc(desc,addr,limit,type) \
do { \
(desc)[0].b = (desc)[1].b = 0; \
diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
new file mode 100644
index 000000000000..4ef52050a11b
--- /dev/null
+++ b/xen/arch/x86/include/asm/idt.h
@@ -0,0 +1,125 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef X86_ASM_IDT_H
+#define X86_ASM_IDT_H
+
+#include <xen/bug.h>
+#include <xen/types.h>
+
+#include <asm/x86-defns.h>
+
+#define IST_NONE 0
+#define IST_MCE 1
+#define IST_NMI 2
+#define IST_DB 3
+#define IST_DF 4
+#define IST_MAX 4
+
+typedef union {
+ struct {
+ uint64_t a, b;
+ };
+ struct {
+ uint16_t addr0;
+ uint16_t cs;
+ uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
+ uint8_t type:4, s:1, dpl:2, p:1;
+ uint16_t addr1;
+ uint32_t addr2;
+ /* 32 bits rsvd. */
+ };
+} idt_entry_t;
+
+#define IDT_ENTRIES 256
+extern idt_entry_t idt_table[];
+extern idt_entry_t *idt_tables[];
+
+/*
+ * Set the Interrupt Stack Table used by a particular IDT entry. Typically
+ * used on a live IDT, so volatile to disuade clever optimisations.
+ */
+static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
+{
+ /* IST is a 3 bit field, 32 bits into the IDT entry. */
+ ASSERT(ist <= IST_MAX);
+
+ idt->ist = ist;
+}
+
+static inline void enable_each_ist(idt_entry_t *idt)
+{
+ set_ist(&idt[X86_EXC_DF], IST_DF);
+ set_ist(&idt[X86_EXC_NMI], IST_NMI);
+ set_ist(&idt[X86_EXC_MC], IST_MCE);
+ set_ist(&idt[X86_EXC_DB], IST_DB);
+}
+
+static inline void disable_each_ist(idt_entry_t *idt)
+{
+ set_ist(&idt[X86_EXC_DF], IST_NONE);
+ set_ist(&idt[X86_EXC_NMI], IST_NONE);
+ set_ist(&idt[X86_EXC_MC], IST_NONE);
+ set_ist(&idt[X86_EXC_DB], IST_NONE);
+}
+
+/*
+ * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
+ * bits of the address not changing, which is a safe assumption as all
+ * functions we are likely to load will live inside the 1GB
+ * code/data/bss address range.
+ */
+static inline void _write_gate_lower(volatile idt_entry_t *gate,
+ const idt_entry_t *new)
+{
+ ASSERT(gate->b == new->b);
+ gate->a = new->a;
+}
+
+#define _set_gate(gate_addr,type,dpl,addr) \
+do { \
+ (gate_addr)->a = 0; \
+ smp_wmb(); /* disable gate /then/ rewrite */ \
+ (gate_addr)->b = \
+ ((unsigned long)(addr) >> 32); \
+ smp_wmb(); /* rewrite /then/ enable gate */ \
+ (gate_addr)->a = \
+ (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
+ ((unsigned long)(dpl) << 45) | \
+ ((unsigned long)(type) << 40) | \
+ ((unsigned long)(addr) & 0xFFFFUL) | \
+ ((unsigned long)__HYPERVISOR_CS << 16) | \
+ (1UL << 47); \
+} while (0)
+
+static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
+ unsigned long dpl, void *addr)
+{
+ idt_entry_t idte;
+ idte.b = gate->b;
+ idte.a =
+ (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
+ ((unsigned long)(dpl) << 45) |
+ ((unsigned long)(type) << 40) |
+ ((unsigned long)(addr) & 0xFFFFUL) |
+ ((unsigned long)__HYPERVISOR_CS << 16) |
+ (1UL << 47);
+ _write_gate_lower(gate, &idte);
+}
+
+/*
+ * Update the lower half handler of an IDT entry, without changing any other
+ * configuration.
+ */
+static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
+{
+ idt_entry_t idte;
+ idte.a = gate->a;
+
+ idte.b = ((unsigned long)(addr) >> 32);
+ idte.a &= 0x0000FFFFFFFF0000ULL;
+ idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
+ ((unsigned long)(addr) & 0xFFFFUL);
+
+ _write_gate_lower(gate, &idte);
+}
+
+#endif /* X86_ASM_IDT_H */
diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h
index d247ef8dd226..86174cce5821 100644
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -353,43 +353,6 @@ struct tss_page {
};
DECLARE_PER_CPU(struct tss_page, tss_page);
-#define IST_NONE 0UL
-#define IST_MCE 1UL
-#define IST_NMI 2UL
-#define IST_DB 3UL
-#define IST_DF 4UL
-#define IST_MAX 4UL
-
-/* Set the Interrupt Stack Table used by a particular IDT entry. */
-static inline void set_ist(idt_entry_t *idt, unsigned int ist)
-{
- /* IST is a 3 bit field, 32 bits into the IDT entry. */
- ASSERT(ist <= IST_MAX);
-
- /* Typically used on a live idt. Disuade any clever optimisations. */
- ACCESS_ONCE(idt->ist) = ist;
-}
-
-static inline void enable_each_ist(idt_entry_t *idt)
-{
- set_ist(&idt[X86_EXC_DF], IST_DF);
- set_ist(&idt[X86_EXC_NMI], IST_NMI);
- set_ist(&idt[X86_EXC_MC], IST_MCE);
- set_ist(&idt[X86_EXC_DB], IST_DB);
-}
-
-static inline void disable_each_ist(idt_entry_t *idt)
-{
- set_ist(&idt[X86_EXC_DF], IST_NONE);
- set_ist(&idt[X86_EXC_NMI], IST_NONE);
- set_ist(&idt[X86_EXC_MC], IST_NONE);
- set_ist(&idt[X86_EXC_DB], IST_NONE);
-}
-
-#define IDT_ENTRIES 256
-extern idt_entry_t idt_table[];
-extern idt_entry_t *idt_tables[];
-
DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
extern void write_ptbase(struct vcpu *v);
diff --git a/xen/arch/x86/machine_kexec.c b/xen/arch/x86/machine_kexec.c
index e20e8d0b1563..f775e526d59b 100644
--- a/xen/arch/x86/machine_kexec.c
+++ b/xen/arch/x86/machine_kexec.c
@@ -22,6 +22,7 @@
#include <asm/fixmap.h>
#include <asm/hpet.h>
+#include <asm/idt.h>
#include <asm/machine_kexec.h>
#include <asm/page.h>
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6b34b908efcd..bfdc8fb01949 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -115,6 +115,7 @@
#include <asm/fixmap.h>
#include <asm/flushtlb.h>
#include <asm/guest.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/io_apic.h>
#include <asm/ldt.h>
@@ -6639,6 +6640,9 @@ static void __init __maybe_unused build_assertions(void)
* using different PATs will not work.
*/
BUILD_BUG_ON(XEN_MSR_PAT != 0x050100070406ULL);
+
+ /* IST_MAX IST pages + at least 1 guard page + primary stack. */
+ BUILD_BUG_ON((IST_MAX + 1) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
}
/*
diff --git a/xen/arch/x86/pv/traps.c b/xen/arch/x86/pv/traps.c
index fd1597d0bdea..77b034e4dc73 100644
--- a/xen/arch/x86/pv/traps.c
+++ b/xen/arch/x86/pv/traps.c
@@ -13,6 +13,7 @@
#include <xen/softirq.h>
#include <asm/debugreg.h>
+#include <asm/idt.h>
#include <asm/irq-vectors.h>
#include <asm/pv/trace.h>
#include <asm/shared.h>
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index f904d5623272..f3d60d5bae35 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -28,6 +28,7 @@
#include <asm/div64.h>
#include <asm/flushtlb.h>
#include <asm/guest.h>
+#include <asm/idt.h>
#include <asm/io_apic.h>
#include <asm/irq-vectors.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e8d5aa9fd46b..1a53bb4aa481 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -52,6 +52,7 @@
#include <asm/hpet.h>
#include <asm/hvm/vpt.h>
#include <asm/i387.h>
+#include <asm/idt.h>
#include <asm/io.h>
#include <asm/irq-vectors.h>
#include <asm/mc146818rtc.h>
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 93f32ac66c92..8b9f0949d348 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -342,9 +342,6 @@ void subarch_percpu_traps_init(void)
unsigned char *stub_page;
unsigned int offset;
- /* IST_MAX IST pages + at least 1 guard page + primary stack. */
- BUILD_BUG_ON((IST_MAX + 1) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
-
/* No PV guests? No need to set up SYSCALL/SYSENTER infrastructure. */
if ( !IS_ENABLED(CONFIG_PV) )
return;
--
2.39.5
On 24.02.2025 17:05, Andrew Cooper wrote:
> Logic concerning the IDT is somewhat different to the other system tables, and
> in particular ought not to be in asm/processor.h. Collect it together a new
> header.
>
> While doing so, make a few minor adjustments:
>
> * Make set_ist() use volatile rather than ACCESS_ONCE(), as
> _write_gate_lower() already does, removing the need for xen/lib.h.
While I don't mind this, I'd still like to mention that one of the first things
I was told when starting to work on Linux was to avoid volatile about everywhere.
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/idt.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef X86_ASM_IDT_H
> +#define X86_ASM_IDT_H
> +
> +#include <xen/bug.h>
> +#include <xen/types.h>
> +
> +#include <asm/x86-defns.h>
> +
> +#define IST_NONE 0
> +#define IST_MCE 1
> +#define IST_NMI 2
> +#define IST_DB 3
> +#define IST_DF 4
> +#define IST_MAX 4
> +
> +typedef union {
> + struct {
> + uint64_t a, b;
> + };
> + struct {
> + uint16_t addr0;
> + uint16_t cs;
> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
> + uint8_t type:4, s:1, dpl:2, p:1;
> + uint16_t addr1;
> + uint32_t addr2;
> + /* 32 bits rsvd. */
> + };
> +} idt_entry_t;
> +
> +#define IDT_ENTRIES 256
> +extern idt_entry_t idt_table[];
> +extern idt_entry_t *idt_tables[];
> +
> +/*
> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
> + * used on a live IDT, so volatile to disuade clever optimisations.
> + */
> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
> +{
> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
> + ASSERT(ist <= IST_MAX);
> +
> + idt->ist = ist;
> +}
> +
> +static inline void enable_each_ist(idt_entry_t *idt)
> +{
> + set_ist(&idt[X86_EXC_DF], IST_DF);
> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
> + set_ist(&idt[X86_EXC_MC], IST_MCE);
> + set_ist(&idt[X86_EXC_DB], IST_DB);
> +}
> +
> +static inline void disable_each_ist(idt_entry_t *idt)
> +{
> + set_ist(&idt[X86_EXC_DF], IST_NONE);
> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
> + set_ist(&idt[X86_EXC_MC], IST_NONE);
> + set_ist(&idt[X86_EXC_DB], IST_NONE);
> +}
> +
> +/*
> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
> + * bits of the address not changing, which is a safe assumption as all
> + * functions we are likely to load will live inside the 1GB
> + * code/data/bss address range.
> + */
> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
> + const idt_entry_t *new)
> +{
> + ASSERT(gate->b == new->b);
> + gate->a = new->a;
> +}
Would this better move down a few lines, immediately ahead of its two
use sites?
> +#define _set_gate(gate_addr,type,dpl,addr) \
Moving this is questionable, as gates aren't limited to the IDT (in
principle; yes, we don't use call gates ourselves). However, as you
move it, my minimal request would be to add the missing blanks here.
Beyond that I wonder ...
> +do { \
> + (gate_addr)->a = 0; \
> + smp_wmb(); /* disable gate /then/ rewrite */ \
> + (gate_addr)->b = \
> + ((unsigned long)(addr) >> 32); \
> + smp_wmb(); /* rewrite /then/ enable gate */ \
> + (gate_addr)->a = \
> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
> + ((unsigned long)(dpl) << 45) | \
> + ((unsigned long)(type) << 40) | \
> + ((unsigned long)(addr) & 0xFFFFUL) | \
> + ((unsigned long)__HYPERVISOR_CS << 16) | \
> + (1UL << 47); \
> +} while (0)
... whether using the other half of the union would allow this to
become a little more readable. (Then it would also rightfully live
here, seeing that the union is typedef-ed to idt_entry_t.) This then
may also extend to ...
> +static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
> + unsigned long dpl, void *addr)
> +{
> + idt_entry_t idte;
> + idte.b = gate->b;
> + idte.a =
> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
> + ((unsigned long)(dpl) << 45) |
> + ((unsigned long)(type) << 40) |
> + ((unsigned long)(addr) & 0xFFFFUL) |
> + ((unsigned long)__HYPERVISOR_CS << 16) |
> + (1UL << 47);
... here and ...
> + _write_gate_lower(gate, &idte);
> +}
> +
> +/*
> + * Update the lower half handler of an IDT entry, without changing any other
> + * configuration.
> + */
> +static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
> +{
> + idt_entry_t idte;
> + idte.a = gate->a;
> +
> + idte.b = ((unsigned long)(addr) >> 32);
> + idte.a &= 0x0000FFFFFFFF0000ULL;
> + idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
> + ((unsigned long)(addr) & 0xFFFFUL);
... here. Otoh you may have reasons to keep these like they are?
Could both _set_gate_lower() and _update_gate_addr_lower() have their
last parameters each be switched to pointer-to-const (they supposedly point
into .text after all)?
Jan
On 25/02/2025 8:27 am, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> Logic concerning the IDT is somewhat different to the other system tables, and
>> in particular ought not to be in asm/processor.h. Collect it together a new
>> header.
>>
>> While doing so, make a few minor adjustments:
>>
>> * Make set_ist() use volatile rather than ACCESS_ONCE(), as
>> _write_gate_lower() already does, removing the need for xen/lib.h.
> While I don't mind this, I'd still like to mention that one of the first things
> I was told when starting to work on Linux was to avoid volatile about everywhere.
Indeed, but that's for "using volatile variables generally". Here we're
using it very specifically for a single store.
>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/idt.h
>> @@ -0,0 +1,125 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#ifndef X86_ASM_IDT_H
>> +#define X86_ASM_IDT_H
>> +
>> +#include <xen/bug.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/x86-defns.h>
>> +
>> +#define IST_NONE 0
>> +#define IST_MCE 1
>> +#define IST_NMI 2
>> +#define IST_DB 3
>> +#define IST_DF 4
>> +#define IST_MAX 4
>> +
>> +typedef union {
>> + struct {
>> + uint64_t a, b;
>> + };
>> + struct {
>> + uint16_t addr0;
>> + uint16_t cs;
>> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
>> + uint8_t type:4, s:1, dpl:2, p:1;
>> + uint16_t addr1;
>> + uint32_t addr2;
>> + /* 32 bits rsvd. */
>> + };
>> +} idt_entry_t;
>> +
>> +#define IDT_ENTRIES 256
>> +extern idt_entry_t idt_table[];
>> +extern idt_entry_t *idt_tables[];
>> +
>> +/*
>> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
>> + * used on a live IDT, so volatile to disuade clever optimisations.
>> + */
>> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
>> +{
>> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
>> + ASSERT(ist <= IST_MAX);
>> +
>> + idt->ist = ist;
>> +}
>> +
>> +static inline void enable_each_ist(idt_entry_t *idt)
>> +{
>> + set_ist(&idt[X86_EXC_DF], IST_DF);
>> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
>> + set_ist(&idt[X86_EXC_MC], IST_MCE);
>> + set_ist(&idt[X86_EXC_DB], IST_DB);
>> +}
>> +
>> +static inline void disable_each_ist(idt_entry_t *idt)
>> +{
>> + set_ist(&idt[X86_EXC_DF], IST_NONE);
>> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
>> + set_ist(&idt[X86_EXC_MC], IST_NONE);
>> + set_ist(&idt[X86_EXC_DB], IST_NONE);
>> +}
>> +
>> +/*
>> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
>> + * bits of the address not changing, which is a safe assumption as all
>> + * functions we are likely to load will live inside the 1GB
>> + * code/data/bss address range.
>> + */
>> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
>> + const idt_entry_t *new)
>> +{
>> + ASSERT(gate->b == new->b);
>> + gate->a = new->a;
>> +}
> Would this better move down a few lines, immediately ahead of its two
> use sites?
>
>> +#define _set_gate(gate_addr,type,dpl,addr) \
> Moving this is questionable, as gates aren't limited to the IDT (in
> principle; yes, we don't use call gates ourselves). However, as you
> move it, my minimal request would be to add the missing blanks here.
_set_gate() doesn't survive to the end of the series, which also fixes
the position of _write_gate_lower().
> Beyond that I wonder ...
>
>> +do { \
>> + (gate_addr)->a = 0; \
>> + smp_wmb(); /* disable gate /then/ rewrite */ \
>> + (gate_addr)->b = \
>> + ((unsigned long)(addr) >> 32); \
>> + smp_wmb(); /* rewrite /then/ enable gate */ \
>> + (gate_addr)->a = \
>> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) | \
>> + ((unsigned long)(dpl) << 45) | \
>> + ((unsigned long)(type) << 40) | \
>> + ((unsigned long)(addr) & 0xFFFFUL) | \
>> + ((unsigned long)__HYPERVISOR_CS << 16) | \
>> + (1UL << 47); \
>> +} while (0)
> ... whether using the other half of the union would allow this to
> become a little more readable. (Then it would also rightfully live
> here, seeing that the union is typedef-ed to idt_entry_t.) This then
> may also extend to ...
>
>> +static inline void _set_gate_lower(idt_entry_t *gate, unsigned long type,
>> + unsigned long dpl, void *addr)
>> +{
>> + idt_entry_t idte;
>> + idte.b = gate->b;
>> + idte.a =
>> + (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
>> + ((unsigned long)(dpl) << 45) |
>> + ((unsigned long)(type) << 40) |
>> + ((unsigned long)(addr) & 0xFFFFUL) |
>> + ((unsigned long)__HYPERVISOR_CS << 16) |
>> + (1UL << 47);
> ... here and ...
>
>> + _write_gate_lower(gate, &idte);
>> +}
>> +
>> +/*
>> + * Update the lower half handler of an IDT entry, without changing any other
>> + * configuration.
>> + */
>> +static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
>> +{
>> + idt_entry_t idte;
>> + idte.a = gate->a;
>> +
>> + idte.b = ((unsigned long)(addr) >> 32);
>> + idte.a &= 0x0000FFFFFFFF0000ULL;
>> + idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
>> + ((unsigned long)(addr) & 0xFFFFUL);
> ... here. Otoh you may have reasons to keep these like they are?
I need to draw the line somewhere on cleanups. I'm already at 50
patches and I still don't have FRED setup working.
These probably can be cleaned up, but at some later point.
~Andrew
On 26.02.2025 18:15, Andrew Cooper wrote:
> On 25/02/2025 8:27 am, Jan Beulich wrote:
>> On 24.02.2025 17:05, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/idt.h
>>> @@ -0,0 +1,125 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#ifndef X86_ASM_IDT_H
>>> +#define X86_ASM_IDT_H
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <asm/x86-defns.h>
>>> +
>>> +#define IST_NONE 0
>>> +#define IST_MCE 1
>>> +#define IST_NMI 2
>>> +#define IST_DB 3
>>> +#define IST_DF 4
>>> +#define IST_MAX 4
>>> +
>>> +typedef union {
>>> + struct {
>>> + uint64_t a, b;
>>> + };
>>> + struct {
>>> + uint16_t addr0;
>>> + uint16_t cs;
>>> + uint8_t ist; /* :3, 5 bits rsvd, but this yields far better code. */
>>> + uint8_t type:4, s:1, dpl:2, p:1;
>>> + uint16_t addr1;
>>> + uint32_t addr2;
>>> + /* 32 bits rsvd. */
>>> + };
>>> +} idt_entry_t;
>>> +
>>> +#define IDT_ENTRIES 256
>>> +extern idt_entry_t idt_table[];
>>> +extern idt_entry_t *idt_tables[];
>>> +
>>> +/*
>>> + * Set the Interrupt Stack Table used by a particular IDT entry. Typically
>>> + * used on a live IDT, so volatile to disuade clever optimisations.
>>> + */
>>> +static inline void set_ist(volatile idt_entry_t *idt, unsigned int ist)
>>> +{
>>> + /* IST is a 3 bit field, 32 bits into the IDT entry. */
>>> + ASSERT(ist <= IST_MAX);
>>> +
>>> + idt->ist = ist;
>>> +}
>>> +
>>> +static inline void enable_each_ist(idt_entry_t *idt)
>>> +{
>>> + set_ist(&idt[X86_EXC_DF], IST_DF);
>>> + set_ist(&idt[X86_EXC_NMI], IST_NMI);
>>> + set_ist(&idt[X86_EXC_MC], IST_MCE);
>>> + set_ist(&idt[X86_EXC_DB], IST_DB);
>>> +}
>>> +
>>> +static inline void disable_each_ist(idt_entry_t *idt)
>>> +{
>>> + set_ist(&idt[X86_EXC_DF], IST_NONE);
>>> + set_ist(&idt[X86_EXC_NMI], IST_NONE);
>>> + set_ist(&idt[X86_EXC_MC], IST_NONE);
>>> + set_ist(&idt[X86_EXC_DB], IST_NONE);
>>> +}
>>> +
>>> +/*
>>> + * Write the lower 64 bits of an IDT Entry. This relies on the upper 32
>>> + * bits of the address not changing, which is a safe assumption as all
>>> + * functions we are likely to load will live inside the 1GB
>>> + * code/data/bss address range.
>>> + */
>>> +static inline void _write_gate_lower(volatile idt_entry_t *gate,
>>> + const idt_entry_t *new)
>>> +{
>>> + ASSERT(gate->b == new->b);
>>> + gate->a = new->a;
>>> +}
>> Would this better move down a few lines, immediately ahead of its two
>> use sites?
>>
>>> +#define _set_gate(gate_addr,type,dpl,addr) \
>> Moving this is questionable, as gates aren't limited to the IDT (in
>> principle; yes, we don't use call gates ourselves). However, as you
>> move it, my minimal request would be to add the missing blanks here.
>
> _set_gate() doesn't survive to the end of the series, which also fixes
> the position of _write_gate_lower().
Hmm, okay:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
© 2016 - 2025 Red Hat, Inc.