Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 0/2] x86/boot: cleanup
Date: Fri, 9 Aug 2019 12:41:42 +0200
Message-ID: <af4765d0-5010-42fe-c78b-d5b8a2a65e79@suse.com> (raw)
In-Reply-To: <a9651ce7-bdd2-ac6d-6670-0abbe4442b14@suse.com>

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On 09.08.2019 12:34, Jan Beulich wrote:
> This is my slight adjustment of the original patch 4 in Andrew's
> series, plus a (new) prereq adjustment. I think the result is
> cleaner overall.
> 
> 1: x86: define a few selector values
> 2: x86/desc: Build boot_{,compat_}gdt[] in C

And I realize I should have attached the patches here once again.

Jan

[-- Attachment #2: x86-selectors.patch --]
[-- Type: text/plain, Size: 3004 bytes --]

x86: define a few selector values

TSS, LDT, and per-CPU entries all can benefit a little from also having
their selector values defined.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -761,7 +761,7 @@ void load_system_tables(void)
 	per_cpu(full_gdt_loaded, cpu) = false;
 	lgdt(&gdtr);
 	lidt(&idtr);
-	ltr(TSS_ENTRY << 3);
+	ltr(TSS_SELECTOR);
 	lldt(0);
 
 	enable_each_ist(idt_tables[cpu]);
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1572,7 +1572,7 @@ bool svm_load_segs(unsigned int ldt_ents
 
         _set_tssldt_desc(desc, ldt_base, ldt_ents * 8 - 1, SYS_DESC_ldt);
 
-        vmcb->ldtr.sel = LDT_ENTRY << 3;
+        vmcb->ldtr.sel = LDT_SELECTOR;
         vmcb->ldtr.attr = SYS_DESC_ldt | (_SEGMENT_P >> 8);
         vmcb->ldtr.limit = ldt_ents * 8 - 1;
         vmcb->ldtr.base = ldt_base;
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1128,7 +1128,7 @@ static int construct_vmcs(struct vcpu *v
     __vmwrite(HOST_GS_SELECTOR, 0);
     __vmwrite(HOST_FS_BASE, 0);
     __vmwrite(HOST_GS_BASE, 0);
-    __vmwrite(HOST_TR_SELECTOR, TSS_ENTRY << 3);
+    __vmwrite(HOST_TR_SELECTOR, TSS_SELECTOR);
 
     /* Host control registers. */
     v->arch.hvm.vmx.host_cr0 = read_cr0() & ~X86_CR0_TS;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1917,7 +1917,7 @@ void load_TR(void)
     /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
     asm volatile (
         "sgdt %0; lgdt %2; ltr %w1; lgdt %0"
-        : "=m" (old_gdt) : "rm" (TSS_ENTRY << 3), "m" (tss_gdt) : "memory" );
+        : "=m" (old_gdt) : "rm" (TSS_SELECTOR), "m" (tss_gdt) : "memory" );
 }
 
 static unsigned int calc_ler_msr(void)
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -251,7 +251,7 @@ void do_double_fault(struct cpu_user_reg
 
     console_force_unlock();
 
-    asm ( "lsll %1, %0" : "=r" (cpu) : "rm" (PER_CPU_GDT_ENTRY << 3) );
+    asm ( "lsll %1, %0" : "=r" (cpu) : "rm" (PER_CPU_GDT_SELECTOR) );
 
     /* Find information saved during fault and dump it to the console. */
     printk("*** DOUBLE FAULT ***\n");
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -36,6 +36,10 @@
 #define LDT_ENTRY (TSS_ENTRY + 2)
 #define PER_CPU_GDT_ENTRY (LDT_ENTRY + 2)
 
+#define TSS_SELECTOR         (TSS_ENTRY << 3)
+#define LDT_SELECTOR         (LDT_ENTRY << 3)
+#define PER_CPU_GDT_SELECTOR (PER_CPU_GDT_ENTRY << 3)
+
 #ifndef __ASSEMBLY__
 
 #define GUEST_KERNEL_RPL(d) (is_pv_32bit_domain(d) ? 1 : 3)
--- a/xen/include/asm-x86/ldt.h
+++ b/xen/include/asm-x86/ldt.h
@@ -16,7 +16,7 @@ static inline void load_LDT(struct vcpu
         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);
+        lldt(LDT_SELECTOR);
     }
 }
 

[-- Attachment #3: x86-move-GDTs-to-C.patch --]
[-- Type: text/plain, Size: 8740 bytes --]

x86/desc: Build boot_{,compat_}gdt[] in C

... where we can at least get the compiler to fill in the surrounding space
without having to do it manually.  This also results 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>

Introduce SEL2GDT(). Correct GDT indices in public header.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Address my own v1 review comments. Re-base.
---
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.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_PV) += compat.o x86_64/comp
 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
--- 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>
--- 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 0x00af9b000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
-        .quad 0x00cf93000000ffff     /* 0xe010 ring 0 data                */
-        .quad 0x0000000000000000     /* reserved                          */
-        .quad 0x00cffb000000ffff     /* 0xe023 ring 3 code, compatibility */
-        .quad 0x00cff3000000ffff     /* 0xe02b ring 3 data                */
-        .quad 0x00affb000000ffff     /* 0xe033 ring 3 code, 64-bit mode   */
-        .quad 0x00cf9b000000ffff     /* 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 0x00af9b000000ffff     /* 0xe008 ring 0 code, 64-bit mode   */
-        .quad 0x00cf93000000ffff     /* 0xe010 ring 0 data                */
-        .quad 0x00cfbb000000ffff     /* 0xe019 ring 1 code, compatibility */
-        .quad 0x00cfb3000000ffff     /* 0xe021 ring 1 data                */
-        .quad 0x00cffb000000ffff     /* 0xe02b ring 3 code, compatibility */
-        .quad 0x00cff3000000ffff     /* 0xe033 ring 3 data                */
-        .quad 0x00cf9b000000ffff     /* 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
--- /dev/null
+++ b/xen/arch/x86/desc.c
@@ -0,0 +1,109 @@
+#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.
+ */
+
+#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 },
+};
+
+__section(".data.page_aligned") __aligned(PAGE_SIZE)
+seg_desc_t boot_compat_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 },
+
+    /* 0xe019 - Ring 1 code, compatibility */
+    [SEL2GDT(FLAT_COMPAT_RING1_CS)] = { 0x00cfbb000000ffff },
+
+    /* 0xe021 - Ring 1 data */
+    [SEL2GDT(FLAT_COMPAT_RING1_DS)] = { 0x00cfb3000000ffff },
+
+    /* 0xe02b - Ring 3 code, compatibility */
+    [SEL2GDT(FLAT_COMPAT_RING3_CS)] = { 0x00cffb000000ffff },
+
+    /* 0xe033 - Ring 3 data */
+    [SEL2GDT(FLAT_COMPAT_RING3_DS)] = { 0x00cff3000000ffff },
+
+    /* 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 },
+};
+
+/*
+ * 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:
+ */
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -107,10 +107,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 {
--- a/xen/include/public/arch-x86/xen-x86_64.h
+++ b/xen/include/public/arch-x86/xen-x86_64.h
@@ -44,11 +44,11 @@
  */
 
 #define FLAT_RING3_CS32 0xe023  /* GDT index 260 */
-#define FLAT_RING3_CS64 0xe033  /* GDT index 261 */
-#define FLAT_RING3_DS32 0xe02b  /* GDT index 262 */
+#define FLAT_RING3_DS32 0xe02b  /* GDT index 261 */
+#define FLAT_RING3_CS64 0xe033  /* GDT index 262 */
 #define FLAT_RING3_DS64 0x0000  /* NULL selector */
-#define FLAT_RING3_SS32 0xe02b  /* GDT index 262 */
-#define FLAT_RING3_SS64 0xe02b  /* GDT index 262 */
+#define FLAT_RING3_SS32 0xe02b  /* GDT index 261 */
+#define FLAT_RING3_SS64 0xe02b  /* GDT index 261 */
 
 #define FLAT_KERNEL_DS64 FLAT_RING3_DS64
 #define FLAT_KERNEL_DS32 FLAT_RING3_DS32

[-- Attachment #4: Type: text/plain, Size: 157 bytes --]

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

  parent reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 10:34 Jan Beulich
2019-08-09 10:38 ` [Xen-devel] [PATCH v2 1/2] x86: define a few selector values Jan Beulich
2019-08-09 11:50   ` Andrew Cooper
2019-08-09 12:35     ` Jan Beulich
2019-08-23  2:38   ` Tian, Kevin
2019-08-09 10:40 ` [Xen-devel] [PATCH v2 2/2] x86/desc: Build boot_{, compat_}gdt[] in C Jan Beulich
2019-08-09 12:19   ` Andrew Cooper
2019-08-09 12:43     ` Jan Beulich
2019-08-09 13:07       ` Andrew Cooper
2019-08-09 13:18         ` Jan Beulich
2019-08-09 15:25           ` Andrew Cooper
2019-08-12  7:32   ` Jan Beulich
2019-08-12 10:36     ` Andrew Cooper
2019-08-09 10:41 ` Jan Beulich [this message]
2019-08-09 12:39 ` [Xen-devel] [PATCH 3/2] x86/desc: Drop __HYPERVISOR_CS32 Andrew Cooper
2019-08-09 12:50   ` Jan Beulich
2019-08-09 15:36     ` Andrew Cooper
2019-08-09 15:52       ` Jan Beulich

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=af4765d0-5010-42fe-c78b-d5b8a2a65e79@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox