[Xen-devel] [PATCH 0/4] x86/boot: Cleanup

[Xen-devel] [PATCH 0/4] x86/boot: Cleanup

[Andrew Cooper]

Various bits of cleanup intended to make the boot sequence clearer to follow,
and remove bits of asm which shouldn't be written in asm.

No changes to functionality.

Andrew Cooper (4):
  x86/asm: Include msr-index.h rather than msr.h
  x86/boot: Minor improvements to efi_arch_post_exit_boot()
  x86/desc: Shorten boot_{,comat_}gdt[] variable names
  x86/desc: Build boot_{,compat_}gdt[] in C

 xen/arch/x86/Makefile             |  1 +
 xen/arch/x86/boot/head.S          |  3 +-
 xen/arch/x86/boot/x86_64.S        | 35 +-----------------
 xen/arch/x86/cpu/common.c         |  4 +--
 xen/arch/x86/desc.c               | 75 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/domain.c             |  7 ++--
 xen/arch/x86/efi/efi-boot.h       | 17 +++++----
 xen/arch/x86/hvm/svm/svm.c        |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c       |  2 +-
 xen/arch/x86/smpboot.c            | 18 +++++-----
 xen/arch/x86/traps.c              | 30 ++++++++--------
 xen/arch/x86/x86_64/kexec_reloc.S |  2 +-
 xen/common/efi/runtime.c          |  2 +-
 xen/include/asm-x86/desc.h        | 14 ++++----
 xen/include/asm-x86/ldt.h         |  3 +-
 15 files changed, 128 insertions(+), 87 deletions(-)
 create mode 100644 xen/arch/x86/desc.c

-- 
2.11.0


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

[Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h

[Andrew Cooper]

There is nothing interesting for assembly code in msr.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/head.S          | 2 +-
 xen/arch/x86/x86_64/kexec_reloc.S | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d78bed394a..ab2d52a79d 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -6,7 +6,7 @@
 #include <asm/fixmap.h>
 #include <asm/page.h>
 #include <asm/processor.h>
-#include <asm/msr.h>
+#include <asm/msr-index.h>
 #include <asm/cpufeature.h>
 #include <public/elfnote.h>
 
diff --git a/xen/arch/x86/x86_64/kexec_reloc.S b/xen/arch/x86/x86_64/kexec_reloc.S
index 4d527dbfce..5bf61d5c2d 100644
--- a/xen/arch/x86/x86_64/kexec_reloc.S
+++ b/xen/arch/x86/x86_64/kexec_reloc.S
@@ -16,7 +16,7 @@
 #include <xen/kimage.h>
 
 #include <asm/asm_defns.h>
-#include <asm/msr.h>
+#include <asm/msr-index.h>
 #include <asm/page.h>
 #include <asm/machine_kexec.h>
 
-- 
2.11.0


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

[Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot()

[Andrew Cooper]

Split up the long asm block by commenting the logical subsections.

The movabs for obtaining __start_xen can be a rip-relative lea instead.  This
has the added advantage that objdump can now cross reference it during
disassembly.

The stack handing is confusing to follow.  %rsp is set up by reading
stack_start which is a pointer to cpu0_stack, then constructing an lret frame
under %rsp (to avoid clobbering whatever is adjacent to cpu0_stack), and uses
the Pascal form of lret to move %rsp to the base of cpu0_stack.

Remove stack_start from the mix and use a single lea to load cpu0_stack base
directly, and use the more common push/push/lretq sequence for reloading %cs.

Use unreachable() rather than an infinite for loop, which lets the compiler
discard all the epilogue code that it inserted previously.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Overall, the asm block is 10 bytes shorter, not that this was the point of the
change.

In principle, the constraints for [cs] and [ds] could be relaxed to include
"m", but Clang decided to insert 5 rip-relative memory operands for the
segment loads, which isn't a clever optimisation to make.
---
 xen/arch/x86/efi/efi-boot.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 7a13a30bc0..2f59d8bdbd 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -249,17 +249,20 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                    "or     $"__stringify(X86_CR4_PGE)", %[cr4]\n\t"
                    "mov    %[cr4], %%cr4\n\t"
 #endif
-                   "movabs $__start_xen, %[rip]\n\t"
+                   /* Load data segments. */
                    "lgdt   gdt_descr(%%rip)\n\t"
-                   "mov    stack_start(%%rip), %%rsp\n\t"
                    "mov    %[ds], %%ss\n\t"
                    "mov    %[ds], %%ds\n\t"
                    "mov    %[ds], %%es\n\t"
                    "mov    %[ds], %%fs\n\t"
                    "mov    %[ds], %%gs\n\t"
-                   "movl   %[cs], 8(%%rsp)\n\t"
-                   "mov    %[rip], (%%rsp)\n\t"
-                   "lretq  %[stkoff]-16"
+
+                   /* Switch stack, reload %cs and jump. */
+                   "lea    %c[stkoff] + cpu0_stack(%%rip), %%rsp\n\t"
+                   "lea    __start_xen(%%rip), %[rip]\n\t"
+                   "push   %[cs]\n\t"
+                   "push   %[rip]\n\t"
+                   "lretq"
                    : [rip] "=&r" (efer/* any dead 64-bit variable */),
                      [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
@@ -268,7 +271,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                      [stkoff] "i" (STACK_SIZE - sizeof(struct cpu_info)),
                      "D" (&mbi)
                    : "memory" );
-    for( ; ; ); /* not reached */
+    unreachable();
 }
 
 static void __init efi_arch_cfg_file_early(EFI_FILE_HANDLE dir_handle, char *section)
-- 
2.11.0


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

[Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names

[Andrew Cooper]

The current names, boot_cpu_{,compat_}gdt_table, have a table suffix which is
redundant with the T of GDT, and the cpu infix doesn't provide any meaningful
context.  Drop them both.

Likewise, shorten the {,compat_}gdt{,_l1e} variables.

Finally, rename gdt_descr to boot_gdtr to more clearly identify its purpose.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/x86_64.S  | 10 +++++-----
 xen/arch/x86/cpu/common.c   |  4 ++--
 xen/arch/x86/domain.c       |  7 +++----
 xen/arch/x86/efi/efi-boot.h |  2 +-
 xen/arch/x86/hvm/svm/svm.c  |  2 +-
 xen/arch/x86/hvm/vmx/vmcs.c |  2 +-
 xen/arch/x86/smpboot.c      | 18 +++++++++---------
 xen/arch/x86/traps.c        | 30 ++++++++++++++----------------
 xen/common/efi/runtime.c    |  2 +-
 xen/include/asm-x86/desc.h  | 12 ++++++------
 xen/include/asm-x86/ldt.h   |  3 +--
 11 files changed, 44 insertions(+), 48 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index cf47e019f5..3909363ca3 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -3,7 +3,7 @@
 
 ENTRY(__high_start)
         /* Install relocated data selectors. */
-        lgdt    gdt_descr(%rip)
+        lgdt    boot_gdtr(%rip)
         mov     $(__HYPERVISOR_DS64),%ecx
         mov     %ecx,%ds
         mov     %ecx,%es
@@ -44,16 +44,16 @@ multiboot_ptr:
         .long   0
 
         .word   0
-GLOBAL(gdt_descr)
+GLOBAL(boot_gdtr)
         .word   LAST_RESERVED_GDT_BYTE
-        .quad   boot_cpu_gdt_table - FIRST_RESERVED_GDT_BYTE
+        .quad   boot_gdt - FIRST_RESERVED_GDT_BYTE
 
 GLOBAL(stack_start)
         .quad   cpu0_stack
 
         .section .data.page_aligned, "aw", @progbits
         .align PAGE_SIZE, 0
-GLOBAL(boot_cpu_gdt_table)
+GLOBAL(boot_gdt)
         .quad 0x0000000000000000     /* unused */
         .quad 0x00af9a000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
         .quad 0x00cf92000000ffff     /* 0xe010 ring 0 data                */
@@ -68,7 +68,7 @@ GLOBAL(boot_cpu_gdt_table)
         .align PAGE_SIZE, 0
 /* NB. Even rings != 0 get access to the full 4Gb, as only the            */
 /*     (compatibility) machine->physical mapping table lives there.       */
-GLOBAL(boot_cpu_compat_gdt_table)
+GLOBAL(boot_compat_gdt)
         .quad 0x0000000000000000     /* unused */
         .quad 0x00af9a000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
         .quad 0x00cf92000000ffff     /* 0xe010 ring 0 data                */
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 7478e21177..dc2dea4d6d 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -709,9 +709,9 @@ void load_system_tables(void)
 
 	struct tss_struct *tss = &this_cpu(init_tss);
 	seg_desc_t *gdt =
-		this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY;
+		this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY;
 	seg_desc_t *compat_gdt =
-		this_cpu(compat_gdt_table) - FIRST_RESERVED_GDT_ENTRY;
+		this_cpu(compat_gdt) - FIRST_RESERVED_GDT_ENTRY;
 
 	const struct desc_ptr gdtr = {
 		.base = (unsigned long)gdt,
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 5933b3f51b..612afb683f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1666,8 +1666,8 @@ static inline bool need_full_gdt(const struct domain *d)
 static void update_xen_slot_in_full_gdt(const struct vcpu *v, unsigned int cpu)
 {
     l1e_write(pv_gdt_ptes(v) + FIRST_RESERVED_GDT_PAGE,
-              !is_pv_32bit_vcpu(v) ? per_cpu(gdt_table_l1e, cpu)
-                                   : per_cpu(compat_gdt_table_l1e, cpu));
+              !is_pv_32bit_vcpu(v) ? per_cpu(gdt_l1e, cpu)
+                                   : per_cpu(compat_gdt_l1e, cpu));
 }
 
 static void load_full_gdt(const struct vcpu *v, unsigned int cpu)
@@ -1686,8 +1686,7 @@ static void load_default_gdt(unsigned int cpu)
 {
     struct desc_ptr gdt_desc = {
         .limit = LAST_RESERVED_GDT_BYTE,
-        .base  = (unsigned long)(per_cpu(gdt_table, cpu) -
-                                 FIRST_RESERVED_GDT_ENTRY),
+        .base  = (unsigned long)(per_cpu(gdt, cpu) - FIRST_RESERVED_GDT_ENTRY),
     };
 
     lgdt(&gdt_desc);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 2f59d8bdbd..5324149f93 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -250,7 +250,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
                    "mov    %[cr4], %%cr4\n\t"
 #endif
                    /* Load data segments. */
-                   "lgdt   gdt_descr(%%rip)\n\t"
+                   "lgdt   boot_gdtr(%%rip)\n\t"
                    "mov    %[ds], %%ss\n\t"
                    "mov    %[ds], %%ds\n\t"
                    "mov    %[ds], %%es\n\t"
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index d81401dbc0..deafa3864e 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1568,7 +1568,7 @@ bool svm_load_segs(unsigned int ldt_ents, unsigned long ldt_base,
     {
         /* Keep GDT in sync. */
         seg_desc_t *desc =
-            this_cpu(gdt_table) + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
+            this_cpu(gdt) + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
 
         _set_tssldt_desc(desc, ldt_base, ldt_ents * 8 - 1, SYS_DESC_ldt);
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 45d18493df..098613822a 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -793,7 +793,7 @@ static void vmx_set_host_env(struct vcpu *v)
     unsigned int cpu = smp_processor_id();
 
     __vmwrite(HOST_GDTR_BASE,
-              (unsigned long)(this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY));
+              (unsigned long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY));
     __vmwrite(HOST_IDTR_BASE, (unsigned long)idt_tables[cpu]);
 
     __vmwrite(HOST_TR_BASE, (unsigned long)&per_cpu(init_tss, cpu));
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 65e9ceeece..8d5fef0012 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -944,11 +944,11 @@ static void cpu_smpboot_free(unsigned int cpu, bool remove)
             free_domheap_page(mfn_to_page(mfn));
     }
 
-    FREE_XENHEAP_PAGE(per_cpu(compat_gdt_table, cpu));
+    FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
 
     if ( remove )
     {
-        FREE_XENHEAP_PAGE(per_cpu(gdt_table, cpu));
+        FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
         FREE_XENHEAP_PAGE(idt_tables[cpu]);
 
         if ( stack_base[cpu] )
@@ -976,22 +976,22 @@ static int cpu_smpboot_alloc(unsigned int cpu)
         goto out;
     memguard_guard_stack(stack_base[cpu]);
 
-    gdt = per_cpu(gdt_table, cpu) ?: alloc_xenheap_pages(0, memflags);
+    gdt = per_cpu(gdt, cpu) ?: alloc_xenheap_pages(0, memflags);
     if ( gdt == NULL )
         goto out;
-    per_cpu(gdt_table, cpu) = gdt;
-    per_cpu(gdt_table_l1e, cpu) =
+    per_cpu(gdt, cpu) = gdt;
+    per_cpu(gdt_l1e, cpu) =
         l1e_from_pfn(virt_to_mfn(gdt), __PAGE_HYPERVISOR_RW);
-    memcpy(gdt, boot_cpu_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
+    memcpy(gdt, boot_gdt, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     BUILD_BUG_ON(NR_CPUS > 0x10000);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
-    per_cpu(compat_gdt_table, cpu) = gdt = alloc_xenheap_pages(0, memflags);
+    per_cpu(compat_gdt, cpu) = gdt = alloc_xenheap_pages(0, memflags);
     if ( gdt == NULL )
         goto out;
-    per_cpu(compat_gdt_table_l1e, cpu) =
+    per_cpu(compat_gdt_l1e, cpu) =
         l1e_from_pfn(virt_to_mfn(gdt), __PAGE_HYPERVISOR_RW);
-    memcpy(gdt, boot_cpu_compat_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
+    memcpy(gdt, boot_compat_gdt, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
     if ( idt_tables[cpu] == NULL )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 38d12013db..162f708ac3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -96,10 +96,10 @@ string_param("nmi", opt_nmi);
 DEFINE_PER_CPU(uint64_t, efer);
 static DEFINE_PER_CPU(unsigned long, last_extable_addr);
 
-DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, gdt_table);
-DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, gdt_table_l1e);
-DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, compat_gdt_table);
-DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, compat_gdt_table_l1e);
+DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, gdt);
+DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, gdt_l1e);
+DEFINE_PER_CPU_READ_MOSTLY(seg_desc_t *, compat_gdt);
+DEFINE_PER_CPU_READ_MOSTLY(l1_pgentry_t, compat_gdt_l1e);
 
 /* Master table, used by CPU0. */
 idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
@@ -1899,17 +1899,17 @@ void load_TR(void)
 {
     struct tss_struct *tss = &this_cpu(init_tss);
     struct desc_ptr old_gdt, tss_gdt = {
-        .base = (long)(this_cpu(gdt_table) - FIRST_RESERVED_GDT_ENTRY),
+        .base = (long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY),
         .limit = LAST_RESERVED_GDT_BYTE
     };
 
     _set_tssldt_desc(
-        this_cpu(gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
+        this_cpu(gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
         offsetof(struct tss_struct, __cacheline_filler) - 1,
         SYS_DESC_tss_avail);
     _set_tssldt_desc(
-        this_cpu(compat_gdt_table) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
+        this_cpu(compat_gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
         offsetof(struct tss_struct, __cacheline_filler) - 1,
         SYS_DESC_tss_busy);
@@ -2000,8 +2000,8 @@ void __init init_idt_traps(void)
     /* CPU0 uses the master IDT. */
     idt_tables[0] = idt_table;
 
-    this_cpu(gdt_table) = boot_cpu_gdt_table;
-    this_cpu(compat_gdt_table) = boot_cpu_compat_gdt_table;
+    this_cpu(gdt) = boot_gdt;
+    this_cpu(compat_gdt) = boot_compat_gdt;
 }
 
 extern void (*const autogen_entrypoints[NR_VECTORS])(void);
@@ -2029,13 +2029,11 @@ void __init trap_init(void)
         }
     }
 
-    /* Cache {,compat_}gdt_table_l1e now that physically relocation is done. */
-    this_cpu(gdt_table_l1e) =
-        l1e_from_pfn(virt_to_mfn(boot_cpu_gdt_table),
-                     __PAGE_HYPERVISOR_RW);
-    this_cpu(compat_gdt_table_l1e) =
-        l1e_from_pfn(virt_to_mfn(boot_cpu_compat_gdt_table),
-                     __PAGE_HYPERVISOR_RW);
+    /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
+    this_cpu(gdt_l1e) =
+        l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
+    this_cpu(compat_gdt_l1e) =
+        l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
 
     percpu_traps_init();
 
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 3d118d571d..ab53ebcc55 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -104,7 +104,7 @@ struct efi_rs_state efi_rs_enter(void)
     {
         struct desc_ptr gdt_desc = {
             .limit = LAST_RESERVED_GDT_BYTE,
-            .base  = (unsigned long)(per_cpu(gdt_table, smp_processor_id()) -
+            .base  = (unsigned long)(per_cpu(gdt, smp_processor_id()) -
                                      FIRST_RESERVED_GDT_ENTRY)
         };
 
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index c011c03ae2..0be9348d29 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -204,12 +204,12 @@ struct __packed desc_ptr {
 	unsigned long base;
 };
 
-extern seg_desc_t boot_cpu_gdt_table[];
-DECLARE_PER_CPU(seg_desc_t *, gdt_table);
-DECLARE_PER_CPU(l1_pgentry_t, gdt_table_l1e);
-extern seg_desc_t boot_cpu_compat_gdt_table[];
-DECLARE_PER_CPU(seg_desc_t *, compat_gdt_table);
-DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_table_l1e);
+extern seg_desc_t boot_gdt[];
+DECLARE_PER_CPU(seg_desc_t *, gdt);
+DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e);
+extern seg_desc_t boot_compat_gdt[];
+DECLARE_PER_CPU(seg_desc_t *, compat_gdt);
+DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);
 DECLARE_PER_CPU(bool, full_gdt_loaded);
 
 extern void load_TR(void);
diff --git a/xen/include/asm-x86/ldt.h b/xen/include/asm-x86/ldt.h
index da502329fb..1383f55308 100644
--- a/xen/include/asm-x86/ldt.h
+++ b/xen/include/asm-x86/ldt.h
@@ -13,8 +13,7 @@ static inline void load_LDT(struct vcpu *v)
         lldt(0);
     else
     {
-        desc = (!is_pv_32bit_vcpu(v)
-                ? this_cpu(gdt_table) : this_cpu(compat_gdt_table))
+        desc = (!is_pv_32bit_vcpu(v) ? this_cpu(gdt) : this_cpu(compat_gdt))
                + LDT_ENTRY - FIRST_RESERVED_GDT_ENTRY;
         _set_tssldt_desc(desc, LDT_VIRT_START(v), ents*8-1, SYS_DESC_ldt);
         lldt(LDT_ENTRY << 3);
-- 
2.11.0


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

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

[Andrew Cooper]

... where we can at least get the compiler to fill in the surrounding space
without having to do it manually.  This also reults in the symbols having
proper type/size information in the debug symbols.

Reorder 'raw' in the seg_desc_t union to allow for easier initialisation.

Leave a comment explaining the various restrictions we have on altering the
GDT layout.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

There is plenty more cleanup which can be done in the future.  As we are
64-bit, there is no need for load_TR() to keep the TSS in sync between the two
GDTs, which means it can drop all sgdt/lgdt instructions.  Also,
__HYPERVISOR_CS32 is unused and could be dropped.

As is demonstrated by the required includes, asm/desc.h is a tangle in need of
some cleanup.

The SYSEXIT GDT requirements are:
  R0 long code, R0 data, R3 compat code, R3 data, R3 long code, R3 data.

We could make Xen compatible by shifting the R0 code/data down by one slot,
moving R0 compat code elsewhere and replacing it with another R3 data.

However, this seems like a fairly pointless exercise, as the only thing it
will do is change the magic numbers which developers have become accustom to
seeing in backtraces.
---
 xen/arch/x86/Makefile      |  1 +
 xen/arch/x86/boot/head.S   |  1 -
 xen/arch/x86/boot/x86_64.S | 33 --------------------
 xen/arch/x86/desc.c        | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/desc.h |  2 +-
 5 files changed, 77 insertions(+), 35 deletions(-)
 create mode 100644 xen/arch/x86/desc.c

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5e3840084b..2443fd2cc5 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_PV) += compat.o x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
 obj-y += debug.o
 obj-y += delay.o
+obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
 obj-y += domctl.o
 obj-y += domain.o
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ab2d52a79d..782deac0d4 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -2,7 +2,6 @@
 #include <xen/multiboot2.h>
 #include <public/xen.h>
 #include <asm/asm_defns.h>
-#include <asm/desc.h>
 #include <asm/fixmap.h>
 #include <asm/page.h>
 #include <asm/processor.h>
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 3909363ca3..f762dfea11 100644
--- 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
-
 GLOBAL(stack_start)
         .quad   cpu0_stack
 
         .section .data.page_aligned, "aw", @progbits
         .align PAGE_SIZE, 0
-GLOBAL(boot_gdt)
-        .quad 0x0000000000000000     /* unused */
-        .quad 0x00af9a000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
-        .quad 0x00cf92000000ffff     /* 0xe010 ring 0 data                */
-        .quad 0x0000000000000000     /* reserved                          */
-        .quad 0x00cffa000000ffff     /* 0xe023 ring 3 code, compatibility */
-        .quad 0x00cff2000000ffff     /* 0xe02b ring 3 data                */
-        .quad 0x00affa000000ffff     /* 0xe033 ring 3 code, 64-bit mode   */
-        .quad 0x00cf9a000000ffff     /* 0xe038 ring 0 code, compatibility */
-        .fill (PER_CPU_GDT_ENTRY - __HYPERVISOR_CS32 / 8 - 1), 8, 0
-        .quad 0x0000910000000000     /* per-CPU entry (limit == cpu)      */
-
-        .align PAGE_SIZE, 0
-/* NB. Even rings != 0 get access to the full 4Gb, as only the            */
-/*     (compatibility) machine->physical mapping table lives there.       */
-GLOBAL(boot_compat_gdt)
-        .quad 0x0000000000000000     /* unused */
-        .quad 0x00af9a000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
-        .quad 0x00cf92000000ffff     /* 0xe010 ring 0 data                */
-        .quad 0x00cfba000000ffff     /* 0xe019 ring 1 code, compatibility */
-        .quad 0x00cfb2000000ffff     /* 0xe021 ring 1 data                */
-        .quad 0x00cffa000000ffff     /* 0xe02b ring 3 code, compatibility */
-        .quad 0x00cff2000000ffff     /* 0xe033 ring 3 data                */
-        .quad 0x00cf9a000000ffff     /* 0xe038 ring 0 code, compatibility */
-        .fill (PER_CPU_GDT_ENTRY - __HYPERVISOR_CS32 / 8 - 1), 8, 0
-        .quad 0x0000910000000000     /* per-CPU entry (limit == cpu)      */
-        .align PAGE_SIZE, 0
-
 /*
  * Mapping of first 2 megabytes of memory. This is mapped with 4kB mappings
  * to avoid type conflicts with fixed-range MTRRs covering the lowest megabyte
diff --git a/xen/arch/x86/desc.c b/xen/arch/x86/desc.c
new file mode 100644
index 0000000000..c5dad90cdd
--- /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) */
+};
+
+/*
+ * Used by each CPU as it starts up, to enter C with a suitable %cs.
+ * References boot_cpu_gdt_table for a short period, until the CPUs switch
+ * onto their per-CPU GDTs.
+ */
+struct desc_ptr boot_gdtr = {
+    .limit = LAST_RESERVED_GDT_BYTE,
+    .base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 0be9348d29..0ebdcd05a9 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -103,10 +103,10 @@
 #define SYS_DESC_trap_gate    15
 
 typedef union {
+    uint64_t raw;
     struct {
         uint32_t a, b;
     };
-    uint64_t raw;
 } seg_desc_t;
 
 typedef union {
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h

[Roger Pau Monné]

On Mon, Aug 05, 2019 at 01:42:58PM +0100, Andrew Cooper wrote:
> There is nothing interesting for assembly code in msr.h
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

If those are the only assembly files including msr.h, could you also
get rid of the assembly guard in msr.h?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h

[Jan Beulich]

On 06.08.2019 16:39, Roger Pau Monné  wrote:
> On Mon, Aug 05, 2019 at 01:42:58PM +0100, Andrew Cooper wrote:
>> There is nothing interesting for assembly code in msr.h
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

Re: [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names

[Roger Pau Monné]

On Mon, Aug 05, 2019 at 01:43:00PM +0100, Andrew Cooper wrote:
> The current names, boot_cpu_{,compat_}gdt_table, have a table suffix which is
> redundant with the T of GDT, and the cpu infix doesn't provide any meaningful
> context.  Drop them both.
> 
> Likewise, shorten the {,compat_}gdt{,_l1e} variables.
> 
> Finally, rename gdt_descr to boot_gdtr to more clearly identify its purpose.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

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

[Roger Pau Monné]

On Mon, Aug 05, 2019 at 01:43:01PM +0100, Andrew Cooper wrote:
> ... where we can at least get the compiler to fill in the surrounding space
> without having to do it manually.  This also reults in the symbols having
> proper type/size information in the debug symbols.
> 
> Reorder 'raw' in the seg_desc_t union to allow for easier initialisation.
> 
> Leave a comment explaining the various restrictions we have on altering the
> GDT layout.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h

[Andrew Cooper]

On 06/08/2019 15:39, Roger Pau Monné wrote:
> On Mon, Aug 05, 2019 at 01:42:58PM +0100, Andrew Cooper wrote:
>> There is nothing interesting for assembly code in msr.h
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> If those are the only assembly files including msr.h, could you also
> get rid of the assembly guard in msr.h?

The build seems happy with the guards removed.  I'll fold those two hunks.

~Andrew

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

Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot()

[Jan Beulich]

On 05.08.2019 14:42, Andrew Cooper wrote:
> Split up the long asm block by commenting the logical subsections.
> 
> The movabs for obtaining __start_xen can be a rip-relative lea instead.  This
> has the added advantage that objdump can now cross reference it during
> disassembly.

I'm surprised this works, but I take it that you've tested it: At
the time the asm() executes, I thought we're still in (what EFI
calls) physical mode, i.e. %rip holding a value less than 4Gb.
Accessing memory using %rip-relative addressing is fine, since
the Xen image is mapped in both places, but obtaining the new
linear address for %rip this way via lea should not be, as this
wouldn't move us to the XEN_VIRT_{START,END} range. I'm curious
to learn which part of my understanding is wrong here.

> The stack handing is confusing to follow.  %rsp is set up by reading
> stack_start which is a pointer to cpu0_stack, then constructing an lret frame
> under %rsp (to avoid clobbering whatever is adjacent to cpu0_stack), and uses
> the Pascal form of lret to move %rsp to the base of cpu0_stack.
> 
> Remove stack_start from the mix and use a single lea to load cpu0_stack base
> directly,

I disagree with this change, at least as long as
xen/arch/x86/boot/x86_64.S also reads from stack_start, rather than
accessing cpu0_stack directly. The code here is intended to mirror
what's being done on the non-EFI path. And it was always my
understanding that it's done this way such that one would need to go
hunt for uses if one wanted to change what (right now) stack_start
points at during pre-SMP boot. Otherwise stack_start wouldn't need
an initializer anymore, and hence could move to .bss.

> and use the more common push/push/lretq sequence for reloading %cs.

I don't see what's wrong with what you call "Pascal form" of lret
(C's __stdcall uses this as well, for example). I don't heavily
mind this transformation, but (I'm sorry to say that) it looks to
me as if this was a change for the sake of changing the code, not
for making it any "better" (for whatever definition of "better").

Jan

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

Re: [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names

[Jan Beulich]

On 06.08.2019 16:51, Roger Pau Monné  wrote:
> On Mon, Aug 05, 2019 at 01:43:00PM +0100, Andrew Cooper wrote:
>> The current names, boot_cpu_{,compat_}gdt_table, have a table suffix which is
>> redundant with the T of GDT, and the cpu infix doesn't provide any meaningful
>> context.  Drop them both.
>>
>> Likewise, shorten the {,compat_}gdt{,_l1e} variables.
>>
>> Finally, rename gdt_descr to boot_gdtr to more clearly identify its purpose.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

(nit: ideally with the missing p inserted in the title)

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

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

[Jan Beulich]

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.

> --- /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. 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. 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.

Jan

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

Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot()

[Andrew Cooper]

On 06/08/2019 16:20, Jan Beulich wrote:
> On 05.08.2019 14:42, Andrew Cooper wrote:
>> Split up the long asm block by commenting the logical subsections.
>>
>> The movabs for obtaining __start_xen can be a rip-relative lea
>> instead.  This
>> has the added advantage that objdump can now cross reference it during
>> disassembly.
>
> I'm surprised this works, but I take it that you've tested it:

So I did specifically test it, but it now occurs to me that the test I
did was via the MB2 64-bit EFI path, which isn't this path.  /sigh

> At the time the asm() executes, I thought we're still in (what EFI
> calls) physical mode, i.e. %rip holding a value less than 4Gb.

In which case, what is the point of using a file format which does
identify the virtual address it would prefer to run at...

(This is a rhetorical question.  The answer is "because EFI seems to
always do the unhelpful thing, given the choice".)

> Accessing memory using %rip-relative addressing is fine, since
> the Xen image is mapped in both places, but obtaining the new
> linear address for %rip this way via lea should not be, as this
> wouldn't move us to the XEN_VIRT_{START,END} range. I'm curious
> to learn which part of my understanding is wrong here.
>
>> The stack handing is confusing to follow.  %rsp is set up by reading
>> stack_start which is a pointer to cpu0_stack, then constructing an
>> lret frame
>> under %rsp (to avoid clobbering whatever is adjacent to cpu0_stack),
>> and uses
>> the Pascal form of lret to move %rsp to the base of cpu0_stack.
>>
>> Remove stack_start from the mix and use a single lea to load
>> cpu0_stack base
>> directly,
>
> I disagree with this change, at least as long as
> xen/arch/x86/boot/x86_64.S also reads from stack_start, rather than
> accessing cpu0_stack directly.

That doesn't mean that a) its conceptually the right thing to do ...

> The code here is intended to mirror
> what's being done on the non-EFI path.  And it was always my
> understanding that it's done this way such that one would need to go
> hunt for uses if one wanted to change what (right now) stack_start
> points at during pre-SMP boot. Otherwise stack_start wouldn't need
> an initializer anymore, and hence could move to .bss.

... or b) that I have any intention of letting stack_start survive. 
Specifically, it is an unnecessary point of serialisation for APs, which
needs to disappear.

cpu0_stack is where cpu0 should have its stack, and this path is
exclusive to cpu0.

>
>> and use the more common push/push/lretq sequence for reloading %cs.
>
> I don't see what's wrong with what you call "Pascal form" of lret
> (C's __stdcall uses this as well, for example).

I'm afraid that this statement clearly highlights the problem I'm trying
to solve.

> I don't heavily
> mind this transformation, but (I'm sorry to say that) it looks to
> me as if this was a change for the sake of changing the code, not
> for making it any "better" (for whatever definition of "better").

It really doesn't matter if you can follow the code, or whether I can
follow it when I've double checked the instruction behaviour because,
while I'm aware this form exists, frankly I'm a little rusty on Pascal
it having ceased to be a dominant programming language before I was born...

The easier code is to follow, the easier time other people will have to
debug and develop on it, and the healthier the Xen community will be as
result.

~Andrew

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

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

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot()

[Jan Beulich]

On 07.08.2019 12:33, Andrew Cooper wrote:
> On 06/08/2019 16:20, Jan Beulich wrote:
>> On 05.08.2019 14:42, Andrew Cooper wrote:
>>> Split up the long asm block by commenting the logical subsections.
>>>
>>> The movabs for obtaining __start_xen can be a rip-relative lea
>>> instead.  This
>>> has the added advantage that objdump can now cross reference it during
>>> disassembly.
>>
>> I'm surprised this works, but I take it that you've tested it:
> 
> So I did specifically test it, but it now occurs to me that the test I
> did was via the MB2 64-bit EFI path, which isn't this path.  /sigh
> 
>> At the time the asm() executes, I thought we're still in (what EFI
>> calls) physical mode, i.e. %rip holding a value less than 4Gb.
> 
> In which case, what is the point of using a file format which does
> identify the virtual address it would prefer to run at...
> 
> (This is a rhetorical question.  The answer is "because EFI seems to
> always do the unhelpful thing, given the choice".)

Not a rhetorical question at all. As said - the pre-OS environment
is a physical one, hence relocating binaries to their preferred
addresses may (and with the addresses we use definitely is) not
possible. Hence them relocating images by default.

>> Accessing memory using %rip-relative addressing is fine, since
>> the Xen image is mapped in both places, but obtaining the new
>> linear address for %rip this way via lea should not be, as this
>> wouldn't move us to the XEN_VIRT_{START,END} range. I'm curious
>> to learn which part of my understanding is wrong here.
>>
>>> The stack handing is confusing to follow.  %rsp is set up by reading
>>> stack_start which is a pointer to cpu0_stack, then constructing an
>>> lret frame
>>> under %rsp (to avoid clobbering whatever is adjacent to cpu0_stack),
>>> and uses
>>> the Pascal form of lret to move %rsp to the base of cpu0_stack.
>>>
>>> Remove stack_start from the mix and use a single lea to load
>>> cpu0_stack base
>>> directly,
>>
>> I disagree with this change, at least as long as
>> xen/arch/x86/boot/x86_64.S also reads from stack_start, rather than
>> accessing cpu0_stack directly.
> 
> That doesn't mean that a) its conceptually the right thing to do ...
> 
>> The code here is intended to mirror
>> what's being done on the non-EFI path.  And it was always my
>> understanding that it's done this way such that one would need to go
>> hunt for uses if one wanted to change what (right now) stack_start
>> points at during pre-SMP boot. Otherwise stack_start wouldn't need
>> an initializer anymore, and hence could move to .bss.
> 
> ... or b) that I have any intention of letting stack_start survive.
> Specifically, it is an unnecessary point of serialisation for APs, which
> needs to disappear.
> 
> cpu0_stack is where cpu0 should have its stack, and this path is
> exclusive to cpu0.

And I'd be okay (but not enthusiastic) to see the other CPU0 use
disappear at the same time (same series at least, not necessarily
same patch).

>>> and use the more common push/push/lretq sequence for reloading %cs.
>>
>> I don't see what's wrong with what you call "Pascal form" of lret
>> (C's __stdcall uses this as well, for example).
> 
> I'm afraid that this statement clearly highlights the problem I'm trying
> to solve.

???

>> I don't heavily
>> mind this transformation, but (I'm sorry to say that) it looks to
>> me as if this was a change for the sake of changing the code, not
>> for making it any "better" (for whatever definition of "better").
> 
> It really doesn't matter if you can follow the code, or whether I can
> follow it when I've double checked the instruction behaviour because,
> while I'm aware this form exists, frankly I'm a little rusty on Pascal
> it having ceased to be a dominant programming language before I was born...

Still, I don't see what Pascal has got to do with it other than it
being (having been) one of the users of it. I don't think insn use
in our code base should be influenced by what languages or other
environments use them. If they're suitable for the purpose, they're
fine to use.

Jan

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

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

[Jan Beulich]

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.

Jan

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

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

[Andrew Cooper]

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