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

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

On 07/08/2019 11:55, Jan Beulich wrote:
> On 07.08.2019 12:46, Andrew Cooper wrote:
>> 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,
>
> Mostly, but specifically not for the gap between __HYPERVISOR_CS32
> and PER_CPU_GDT_ENTRY.
>
>> and all I've got time for right now.
>
> Once the earlier 3 patches (assuming there's a dependency) have
> gone in, would you mind me taking this and making another attempt?
> That may convince me of your statement above, or result in fewer
> hidden dependencies.

There are no functional dependencies between any patches in this series,
but a few minor textural ones.

I've just pushed the acked subset, which will address the minor textural
conflicts.  There will be a major conflict with the GDT Accessed patch,
but that will be easy to mechanically resolve.

~Andrew

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