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

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

On 09/08/2019 14:18, Jan Beulich wrote:
> On 09.08.2019 15:07, Andrew Cooper wrote:
>> On 09/08/2019 13:43, Jan Beulich wrote:
>>> On 09.08.2019 14:19, Andrew Cooper wrote:
>>>> On 09/08/2019 11:40, Jan Beulich wrote:
>>>>> --- /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.
>>
>> IMO this is the right place, because it is right beside where the array
>> is specifically defined to be [PAGE_SIZE / sizeof()].
>
> I was about to ask why we then need build_assertions() at all,
> until I also saw ...
>
>> What it is doing is working around what is arguably a compiler bug by
>> allowing foo[x] = { [x + 1] = ... } to work.
>
> ... this. Which made me go check, and both gcc 4.3 and gcc 9.1
> correctly complain "array index in initializer exceeds array
> bounds".
>
>> Anyway, with these assertions and the tweaked constant clenaup,
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Thanks, but as per above I'm now irritated: With the explicit
> specification of the array size, build_assertions() should either
> be dropped again, or its comment be extended to cover why it's
> needed _despite_ the specified array size (i.e. in which case
> your example above would not cause a build failure).

This comes down to a bug I encountered while doing the CPUID work, which
is specifically why we have cpuid.c's build assertions.

Given that we get failures from at least one compiler in CI, we can drop
the extra build assertions.

At some point which isn't now, I'll try to work out exactly what went
wrong in the CPUID case, but I can't reproduce it from memory.

~Andrew

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