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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
