Re: [PATCH] x86/desc: build boot GDT descriptors with a macro

[Furkan Çalışkan <frn1furkan10@xxxxxxxxx>]

On 10/26/26 15:15, Jan Beulich wrote:
> On 20.10.2026 15:57, Furkan Caliskan wrote:
>> The raw 64-bit constants in boot_gdt[] and boot_compat_gdt[] hide what
>> each descriptor actually is.  Replace them with a GDTE() macro, modelled
>> on Linux's GDT_ENTRY_INIT(), plus named GDTE_FLAGS_* constants and a
>> GDTE_FLAT() helper for flat segments.
>>
>> No functional change.
> 
> Can this be "upgraded" to "No change in generated code" or some such? Else:
> Are there changes in generated code?

I compared the generated code before and after, and it is identical.
I'll state that in v2.

> 
>> --- a/xen/arch/x86/desc.c
>> +++ b/xen/arch/x86/desc.c
>> @@ -27,32 +27,67 @@
>>  
>>  #define SEL2GDT(sel) (((sel) >> 3) - FIRST_RESERVED_GDT_ENTRY)
>>  
>> +/*
>> + * Expands to a brace-enclosed initializer for seg_desc_t; only use it
>> + * inside an initializer list.
>> + *
>> + * flags: bit layout matches its position in the descriptor's high dword
>> + *   bits 0-3   : type
>> + *   bit  4     : s     (0 = system, 1 = code/data)
>> + *   bits 5-6   : dpl
>> + *   bit  7     : p     (present)
>> + *   bit  12    : avl
>> + *   bit  13    : l     (64-bit code segment)
>> + *   bit  14    : d/b
>> + *   bit  15    : g     (granularity: 0 = byte, 1 = 4KiB)
>> + */
>> +#define GDTE(flags, base, limit)                                        \
>> +    { (((uint64_t)(limit) & 0x0ffffULL)            |                    \
>> +       (((uint64_t)(base)  & 0x0ffffffULL)  << 16) |                    \
>> +       (((uint64_t)(flags) & 0x0ffULL)      << 40) |                    \
>> +       (((uint64_t)(limit) & 0xf0000ULL)    << 32) |                    \
>> +       (((uint64_t)(flags) & 0xf000ULL)     << 40) |                    \
>> +       (((uint64_t)(base)  & 0xff000000ULL) << 32)) }
> 
> As you only ever use this for GDTs, the macro name here is okay. However, ...
> 
>> +/* Common flag combinations (same values Linux uses for equivalent 
>> segments) */
>> +#define GDTE_FLAGS_RING0_CS64   0xa09b   /* P DPL0 S code  RX A, L, G */
>> +#define GDTE_FLAGS_RING0_DS32   0xc093   /* P DPL0 S data  RW A,    G */
>> +#define GDTE_FLAGS_RING1_CS32   0xc0bb   /* P DPL1 S code  RX A,    G */
>> +#define GDTE_FLAGS_RING1_DS32   0xc0b3   /* P DPL1 S data  RW A,    G */
>> +#define GDTE_FLAGS_RING3_CS32   0xc0fb   /* P DPL3 S code  RX A,    G */
>> +#define GDTE_FLAGS_RING3_DS32   0xc0f3   /* P DPL3 S data  RW A,    G */
>> +#define GDTE_FLAGS_RING3_CS64   0xa0fb   /* P DPL3 S code  RX A, L, G */
>> +#define GDTE_FLAGS_PERCPU       0x0091   /* P DPL0 S data  RO A       */
>> +
>> +/* Xen maps every flat segment across all of memory. */
>> +#define GDTE_FLAT(flags) GDTE(flags, 0, 0xfffff)
> 
> ... nothing in any of these constants is GDT-specific, and the FLAGS infix
> isn't very useful there either (albeit I probably also wouldn't outright
> insist on its dropping).

Agreed.  Following Andrew's feedback as well, v2 will drop the packed
flag words and the GDTE_FLAGS_* names altogether, and instead spell out
each descriptor's attributes symbolically at the use site.
> 
> As to the GDTE_FLAGS_* constants' comments: The P and S bits (which are only
> ever set) are mentioned, while the D/B bit (which varies) isn't. For the
> constants themselves, there's clearly redundancy with asm/desc.h's _SEGMENT_*
> constants. Possibly those could be used here, perhaps eliminating the need
> for those comments.

I'll look at reusing the _SEGMENT_* constants from asm/desc.h for the
attribute bits rather than introducing new ones, and make sure D/B is
covered, since it varies between the entries.
> 
> Jan

Thanks,
Furkan