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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 26/02/2025 2:14 pm, Jan Beulich wrote:
> On 26.02.2025 14:37, Andrew Cooper wrote:
>> 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.
> Oh, of course.
>
>> Furthermore, for any exception we know about, we want a manual one to
>> avoid the error-code realignment logic where possible.
> Why would that not apply to Co-processor Segment Overrun then?

It kinda does apply.

We've never ever had CSO handler (hence why it was autogen'd the first
time I tried making this more robust), and you didn't like my patch to
autogen the exception entries.
The CSO handler (and SPV) are the only two we can be pretty confident
will never trigger on today's hardware, yet you also didn't like my
suggestion of having them Not Present.
>>>> --- /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.
> Right, as said - the semicolons would need putting after every GEN() invocation,
> including in GEN16() (with the final one likely excluded, for the semicolon then
> to appear on its use site).

Ah, I see what you mean.  I'll see if I can make it work.

~Andrew