Re: [Xen-devel] [PATCH v2 2/2] x86/desc: Build boot_{, compat_}gdt[] in C

[Jan Beulich <jbeulich@suse.com>]

On 09.08.2019 14:19, Andrew Cooper wrote:
> On 09/08/2019 11:40, Jan Beulich wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Introduce SEL2GDT(). Correct GDT indices in public header.
> 
> "Correct" here is ambiguous because it implies there is a breakage.
> 
> You appear to have reversed FLAT_RING3_CS64 and DS32 (retaining the
> original comments) and changed the comments for FLAT_RING3_SS{32,64}.

Well - the comments were what was wrong after all.

> Except that now they are out of their logical order (CS 32 then 64, DS
> 32 then 64, SS 32 then 64).
> 
> What is the reasoning for the new order?  It isn't sorted by index.

It is, as much as it looks to have been the original author's
intent. I.e. I didn't re-order things across the nul selector
between the two groups.I can pull FLAT_RING3_SS* up if that's
what you want. I'm also happy with any other order that you
may prefer - just let me know which one. All I care about is
for the comments to be in sync with the actual values.

>> --- /dev/null
>> +++ b/xen/arch/x86/desc.c
>> @@ -0,0 +1,109 @@
>> +
>> +#define SEL2GDT(sel) (((sel) >> 3) - FIRST_RESERVED_GDT_ENTRY)
>> +
>> +__section(".data.page_aligned") __aligned(PAGE_SIZE)
>> +seg_desc_t boot_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
>> +{
>> +    /* 0xe008 - Ring 0 code, 64bit mode */
>> +    [SEL2GDT(__HYPERVISOR_CS64)] = { 0x00af9b000000ffff },
>> +
>> +    /* 0xe010 - Ring 0 data */
>> +    [SEL2GDT(__HYPERVISOR_DS32)] = { 0x00cf93000000ffff },
>> +
>> +    /* 0xe018 - reserved */
>> +
>> +    /* 0xe023 - Ring 3 code, compatibility */
>> +    [SEL2GDT(FLAT_RING3_CS32)] = { 0x00cffb000000ffff },
>> +
>> +    /* 0xe02b - Ring 3 data */
>> +    [SEL2GDT(FLAT_RING3_DS32)] = { 0x00cff3000000ffff },
>> +
>> +    /* 0xe033 - Ring 3 code, 64-bit mode */
>> +    [SEL2GDT(FLAT_RING3_CS64)] = { 0x00affb000000ffff },
>> +
>> +    /* 0xe038 - Ring 0 code, compatibility */
>> +    [SEL2GDT(__HYPERVISOR_CS32)] = { 0x00cf9b000000ffff },
>> +
>> +    /* 0xe040 - TSS */
>> +    /* 0xe050 - LDT */
>> +
>> +    /* 0xe060 - per-CPU entry (limit == cpu) */
>> +    [SEL2GDT(PER_CPU_GDT_SELECTOR)] = { 0x0000910000000000 },
> 
> It would be better if the = { } were vertically aligned.  It makes
> reading them easier.
> 
> Also, now that SEL2GDT() is present, we need a BUILD_BUG_ON() to check
> that the size doesn't vary from one page.  At the moment, changing a
> selector to use 0xfxxx will cause this to grow beyond 1 page without any
> compiler diagnostic.  I'd go with
> 
> static void __init __maybe_unused
> build_assertions(void)
> 
> {
>      BUILD_BUG_ON(sizeof(boot_gdt) != PAGE_SIZE);
>      BUILD_BUG_ON(sizeof(boot_compat_gdt) != PAGE_SIZE);
> }

Will do, albeit for the build assertions this isn't really the
right place imo, because this isn't the place where we depend
on them being just single pages. I'll put it there nevertheless,
but I'll add a comment for why they're there.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel