Re: [PATCH 6/8] x86/IDT: Generate bsp_idt[] at build time

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/02/2025 12:39 pm, Jan Beulich wrote:
> On 24.02.2025 17:05, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/gen-idt.h
>> @@ -0,0 +1,121 @@
>> +/*
>> + * Generator for IDT entries.
>> + *
>> + * Caller to provide GEN(vector, symbol, dpl, autogen) macro
>> + *
>> + * Symbols are 'entry_0xYY' if there is no better name available.  Regular
>> + * handlers set autogen=1, while manual (autogen=0) require the symbol to be
>> + * implemented somewhere else.
>> + */
> Doesn't this need something for Eclair to spot the deliberate absence of a
> header guard?

Eclair doesn't complain, although I'm not entirely sure why.

>> +#define DPL0 0
>> +#define DPL1 1
>> +#define DPL3 3
>> +
>> +#define manual 0
>> +#define autogen 1
>> +
>> +#define GEN16(i) \
>> +    GEN(0x ## i ## 0, entry_0x ## i ## 0, DPL0, autogen) \
>> +    GEN(0x ## i ## 1, entry_0x ## i ## 1, DPL0, autogen) \
>> +    GEN(0x ## i ## 2, entry_0x ## i ## 2, DPL0, autogen) \
>> +    GEN(0x ## i ## 3, entry_0x ## i ## 3, DPL0, autogen) \
>> +    GEN(0x ## i ## 4, entry_0x ## i ## 4, DPL0, autogen) \
>> +    GEN(0x ## i ## 5, entry_0x ## i ## 5, DPL0, autogen) \
>> +    GEN(0x ## i ## 6, entry_0x ## i ## 6, DPL0, autogen) \
>> +    GEN(0x ## i ## 7, entry_0x ## i ## 7, DPL0, autogen) \
>> +    GEN(0x ## i ## 8, entry_0x ## i ## 8, DPL0, autogen) \
>> +    GEN(0x ## i ## 9, entry_0x ## i ## 9, DPL0, autogen) \
>> +    GEN(0x ## i ## a, entry_0x ## i ## a, DPL0, autogen) \
>> +    GEN(0x ## i ## b, entry_0x ## i ## b, DPL0, autogen) \
>> +    GEN(0x ## i ## c, entry_0x ## i ## c, DPL0, autogen) \
>> +    GEN(0x ## i ## d, entry_0x ## i ## d, DPL0, autogen) \
>> +    GEN(0x ## i ## e, entry_0x ## i ## e, DPL0, autogen) \
>> +    GEN(0x ## i ## f, entry_0x ## i ## f, DPL0, autogen)
>> +
>> +
>> +GEN(0x00, entry_DE,         DPL0, manual)
>> +GEN(0x01, entry_DB,         DPL0, manual)
>> +GEN(0x02, entry_NMI,        DPL0, manual)
>> +GEN(0x03, entry_BP,         DPL3, manual)
>> +GEN(0x04, entry_OF,         DPL3, manual)
> Would this better be
>
> #ifdef CONFIG_PV32
> GEN(0x04, entry_OF,         DPL3, manual)
> #else
> GEN(0x04, entry_0x04,       DPL0, autogen)
> #endif
>
> ? (Not necessarily in this patch, but in principle.)

No.  INTO can still be used in compatibility mode segment.

Furthermore, for any exception we know about, we want a manual one to
avoid the error-code realignment logic where possible.

>
>> --- /dev/null
>> +++ b/xen/arch/x86/include/asm/gen-idt.lds.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Linker file fragment to help format the IDT correctly
>> + *
>> + * The IDT, having grown compatibly since the 16 bit days, has the 
>> entrypoint
>> + * address field split into 3.  x86 ELF lacks the @lo/@hi/etc relocation 
>> forms
>> + * commonly found in other architectures for accessing a part of a resolved
>> + * symbol address.
>> + *
>> + * However, the linker can perform the necessary calculations and provide 
>> them
>> + * under new symbol names.  We use this to generate the low and next 16 bits
>> + * of the address for each handler.
>> + *
>> + * The upper 32 bits are always a constant as Xen's .text/data/rodata sits 
>> in
>> + * a single aligned 1G range, so do not need calculating in this manner.
>> + */
>> +#ifndef X86_IDT_GEN_LDS_H
>> +#define X86_IDT_GEN_LDS_H
>> +
>> +#define GEN(vec, sym, dpl, auto)                                        \
>> +    PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR1 = ABSOLUTE(((sym) & 0xffff))); \
>> +    PROVIDE_HIDDEN(IDT_ ## sym ## _ADDR2 = ABSOLUTE(((sym >> 16) & 
>> 0xffff)));
> Not sure if Eclair gets to see this at all, but maybe better parenthesize
> sym also in the latter instance?

Oh, yes.

> As to the final semicolon - ideally this would be on the use sites of GEN(),
> for things to look more C-ish. Yet I won't insist, as gen-idt.h ends up
> looking sufficiently uniform for this to not be a major concern.

I'm afraid it's necessary (and too in entry stubs).

It's the GEN16() macro, which expands 16x GEN() on the same line.

I could drop the GEN16() macro and do everything longhand, but I suspect
you'd like that even less.

~Andrew