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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2019 16:48, Jan Beulich wrote:
> On 05.08.2019 14:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/x86_64.S
>> +++ b/xen/arch/x86/boot/x86_64.S
>> @@ -43,44 +43,11 @@ ENTRY(__high_start)
>>   multiboot_ptr:
>>           .long   0
>>   -        .word   0
>> -GLOBAL(boot_gdtr)
>> -        .word   LAST_RESERVED_GDT_BYTE
>> -        .quad   boot_gdt - FIRST_RESERVED_GDT_BYTE
>
> Just as a remark: The intentional misalignment here is lost with
> the transition to C.

And it is used exactly once on each CPU.  I didn't even consider that
worth remarking on in the commit message.

>
>> --- /dev/null
>> +++ b/xen/arch/x86/desc.c
>> @@ -0,0 +1,75 @@
>> +#include <xen/types.h>
>> +#include <xen/lib.h>
>> +#include <xen/percpu.h>
>> +#include <xen/mm.h>
>> +
>> +#include <asm/desc.h>
>> +
>> +/*
>> + * Native and Compat GDTs used by Xen.
>> + *
>> + * The R1 and R3 descriptors are fixed in Xen's ABI for PV guests. 
>> All other
>> + * descriptors are in principle variable, with the following
>> restrictions.
>> + *
>> + * All R0 descriptors must line up in both GDTs to allow for correct
>> + * interrupt/exception handling.
>> + *
>> + * The SYSCALL/SYSRET GDT layout requires:
>> + *  - R0 long mode code followed by R0 data.
>> + *  - R3 compat code, followed by R3 data, followed by R3 long mode
>> code.
>> + *
>> + * The SYSENTER GDT layout requirements are compatible with
>> SYSCALL.  Xen does
>> + * not use the SYSEXIT instruction, and does not provide a
>> compatible GDT.
>> + *
>> + * These tables are used directly by CPU0, and used as the template
>> for the
>> + * GDTs of other CPUs.  Everything from the TSS onwards is unique
>> per CPU.
>> + */
>> +__section(".data.page_aligned") __aligned(PAGE_SIZE)
>> +seg_desc_t boot_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
>> +{
>> +    [ 1] = { 0x00af9a000000ffff }, /* 0xe008 - Ring 0 code, 64bit
>> mode      */
>> +    [ 2] = { 0x00cf92000000ffff }, /* 0xe010 - Ring 0
>> data                  */
>> +                                   /* 0xe018 -
>> reserved                     */
>> +    [ 4] = { 0x00cffa000000ffff }, /* 0xe023 - Ring 3 code,
>> compatibility   */
>> +    [ 5] = { 0x00cff2000000ffff }, /* 0xe02b - Ring 3
>> data                  */
>> +    [ 6] = { 0x00affa000000ffff }, /* 0xe033 - Ring 3 code, 64-bit
>> mode     */
>> +    [ 7] = { 0x00cf9a000000ffff }, /* 0xe038 - Ring 0 code,
>> compatibility   */
>> +                                   /* 0xe040 -
>> TSS                          */
>> +                                   /* 0xe050 -
>> LDT                          */
>> +    [12] = { 0x0000910000000000 }, /* 0xe060 - per-CPU entry (limit
>> == cpu) */
>> +};
>> +
>> +__section(".data.page_aligned") __aligned(PAGE_SIZE)
>> +seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
>> +{
>> +    [ 1] = { 0x00af9a000000ffff }, /* 0xe008 - Ring 0 code, 64-bit
>> mode     */
>> +    [ 2] = { 0x00cf92000000ffff }, /* 0xe010 - Ring 0
>> data                  */
>> +    [ 3] = { 0x00cfba000000ffff }, /* 0xe019 - Ring 1 code,
>> compatibility   */
>> +    [ 4] = { 0x00cfb2000000ffff }, /* 0xe021 - Ring 1
>> data                  */
>> +    [ 5] = { 0x00cffa000000ffff }, /* 0xe02b - Ring 3 code,
>> compatibility   */
>> +    [ 6] = { 0x00cff2000000ffff }, /* 0xe033 - Ring 3
>> data                  */
>> +    [ 7] = { 0x00cf9a000000ffff }, /* 0xe038 - Ring 0 code,
>> compatibility   */
>> +                                   /* 0xe040 -
>> TSS                          */
>> +                                   /* 0xe050 -
>> LDT                          */
>> +    [12] = { 0x0000910000000000 }, /* 0xe060 - per-CPU entry (limit
>> == cpu) */
>> +};
>
> However unlikely it may be that we're going to change the order of
> descriptors, I think there shouldn't be literal-number array indices
> here. I think we want a local macro here to translate a selector (of
> non-numeric form, e.g. __HYPERVISOR_CS64) to an array index.

I tried this, and then backtracked.  Our various constants are not in a
consistent-enough form to do this at this point.

More clean-up will come later, but as it stands, this is a
functionally-equivalent transform, and all I've got time for right now.

> This way
> adjustments to selector values that aren't part of the PV ABI (i.e.
> anything from __HYPERVISOR_CS32 onwards) would be propagated here
> right away. __HYPERVISOR_CS32 is a good example actually: We don't
> seem to use it for anything, so we ought to be able to drop it.

I did comment on this...

> How would one easily notice to also remove the initializer here without
> the array index getting calculated from its (fundamental) selector?
>
> While an orthogonal change - did you consider taking the opportunity
> and set the accessed bits everywhere? Only the per-CPU entry has it
> set right now.

I'd not even spotted that.  It should be broken out into an earlier
patch for backport.

~Andrew

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