* [Xen-devel] [PATCH 0/4] x86/boot: Cleanup @ 2019-08-05 12:42 Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h Andrew Cooper ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Andrew Cooper @ 2019-08-05 12:42 UTC (permalink / raw) To: Xen-devel; +Cc: 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h 2019-08-05 12:42 [Xen-devel] [PATCH 0/4] x86/boot: Cleanup Andrew Cooper @ 2019-08-05 12:42 ` Andrew Cooper 2019-08-06 14:39 ` Roger Pau Monné 2019-08-05 12:42 ` [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() Andrew Cooper ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Andrew Cooper @ 2019-08-05 12:42 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h 2019-08-05 12:42 ` [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h Andrew Cooper @ 2019-08-06 14:39 ` Roger Pau Monné 2019-08-06 14:50 ` Jan Beulich 2019-08-06 15:14 ` Andrew Cooper 0 siblings, 2 replies; 18+ messages in thread From: Roger Pau Monné @ 2019-08-06 14:39 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h 2019-08-06 14:39 ` Roger Pau Monné @ 2019-08-06 14:50 ` Jan Beulich 2019-08-06 15:14 ` Andrew Cooper 1 sibling, 0 replies; 18+ messages in thread From: Jan Beulich @ 2019-08-06 14:50 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h 2019-08-06 14:39 ` Roger Pau Monné 2019-08-06 14:50 ` Jan Beulich @ 2019-08-06 15:14 ` Andrew Cooper 1 sibling, 0 replies; 18+ messages in thread From: Andrew Cooper @ 2019-08-06 15:14 UTC (permalink / raw) To: Roger Pau Monné; +Cc: Xen-devel, Wei Liu, Jan Beulich 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() 2019-08-05 12:42 [Xen-devel] [PATCH 0/4] x86/boot: Cleanup Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h Andrew Cooper @ 2019-08-05 12:42 ` Andrew Cooper 2019-08-06 15:20 ` Jan Beulich 2019-08-05 12:43 ` [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names Andrew Cooper 2019-08-05 12:43 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C Andrew Cooper 3 siblings, 1 reply; 18+ messages in thread From: Andrew Cooper @ 2019-08-05 12:42 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() 2019-08-05 12:42 ` [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() Andrew Cooper @ 2019-08-06 15:20 ` Jan Beulich 2019-08-07 10:33 ` Andrew Cooper 0 siblings, 1 reply; 18+ messages in thread From: Jan Beulich @ 2019-08-06 15:20 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() 2019-08-06 15:20 ` Jan Beulich @ 2019-08-07 10:33 ` Andrew Cooper 2019-08-07 10:50 ` Jan Beulich 0 siblings, 1 reply; 18+ messages in thread From: Andrew Cooper @ 2019-08-07 10:33 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() 2019-08-07 10:33 ` Andrew Cooper @ 2019-08-07 10:50 ` Jan Beulich 0 siblings, 0 replies; 18+ messages in thread From: Jan Beulich @ 2019-08-07 10:50 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names 2019-08-05 12:42 [Xen-devel] [PATCH 0/4] x86/boot: Cleanup Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() Andrew Cooper @ 2019-08-05 12:43 ` Andrew Cooper 2019-08-06 14:51 ` Roger Pau Monné 2019-08-05 12:43 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C Andrew Cooper 3 siblings, 1 reply; 18+ messages in thread From: Andrew Cooper @ 2019-08-05 12:43 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names 2019-08-05 12:43 ` [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names Andrew Cooper @ 2019-08-06 14:51 ` Roger Pau Monné 2019-08-06 15:28 ` Jan Beulich 0 siblings, 1 reply; 18+ messages in thread From: Roger Pau Monné @ 2019-08-06 14:51 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names 2019-08-06 14:51 ` Roger Pau Monné @ 2019-08-06 15:28 ` Jan Beulich 0 siblings, 0 replies; 18+ messages in thread From: Jan Beulich @ 2019-08-06 15:28 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C 2019-08-05 12:42 [Xen-devel] [PATCH 0/4] x86/boot: Cleanup Andrew Cooper ` (2 preceding siblings ...) 2019-08-05 12:43 ` [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names Andrew Cooper @ 2019-08-05 12:43 ` Andrew Cooper 2019-08-06 15:04 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] " Roger Pau Monné 2019-08-06 15:48 ` Jan Beulich 3 siblings, 2 replies; 18+ messages in thread From: Andrew Cooper @ 2019-08-05 12:43 UTC (permalink / raw) To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné ... 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] in C 2019-08-05 12:43 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C Andrew Cooper @ 2019-08-06 15:04 ` Roger Pau Monné 2019-08-06 15:48 ` Jan Beulich 1 sibling, 0 replies; 18+ messages in thread From: Roger Pau Monné @ 2019-08-06 15:04 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Jan Beulich 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] in C 2019-08-05 12:43 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C Andrew Cooper 2019-08-06 15:04 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] " Roger Pau Monné @ 2019-08-06 15:48 ` Jan Beulich 2019-08-07 10:46 ` Andrew Cooper 1 sibling, 1 reply; 18+ messages in thread From: Jan Beulich @ 2019-08-06 15:48 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] in C 2019-08-06 15:48 ` Jan Beulich @ 2019-08-07 10:46 ` Andrew Cooper 2019-08-07 10:55 ` Jan Beulich 0 siblings, 1 reply; 18+ messages in thread From: Andrew Cooper @ 2019-08-07 10:46 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] in C 2019-08-07 10:46 ` Andrew Cooper @ 2019-08-07 10:55 ` Jan Beulich 2019-08-07 12:49 ` Andrew Cooper 0 siblings, 1 reply; 18+ messages in thread From: Jan Beulich @ 2019-08-07 10:55 UTC (permalink / raw) To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] in C 2019-08-07 10:55 ` Jan Beulich @ 2019-08-07 12:49 ` Andrew Cooper 0 siblings, 0 replies; 18+ messages in thread From: Andrew Cooper @ 2019-08-07 12:49 UTC (permalink / raw) To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-08-07 12:50 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-05 12:42 [Xen-devel] [PATCH 0/4] x86/boot: Cleanup Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 1/4] x86/asm: Include msr-index.h rather than msr.h Andrew Cooper 2019-08-06 14:39 ` Roger Pau Monné 2019-08-06 14:50 ` Jan Beulich 2019-08-06 15:14 ` Andrew Cooper 2019-08-05 12:42 ` [Xen-devel] [PATCH 2/4] x86/boot: Minor improvements to efi_arch_post_exit_boot() Andrew Cooper 2019-08-06 15:20 ` Jan Beulich 2019-08-07 10:33 ` Andrew Cooper 2019-08-07 10:50 ` Jan Beulich 2019-08-05 12:43 ` [Xen-devel] [PATCH 3/4] x86/desc: Shorten boot_{, comat_}gdt[] variable names Andrew Cooper 2019-08-06 14:51 ` Roger Pau Monné 2019-08-06 15:28 ` Jan Beulich 2019-08-05 12:43 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{,compat_}gdt[] in C Andrew Cooper 2019-08-06 15:04 ` [Xen-devel] [PATCH 4/4] x86/desc: Build boot_{, compat_}gdt[] " Roger Pau Monné 2019-08-06 15:48 ` Jan Beulich 2019-08-07 10:46 ` Andrew Cooper 2019-08-07 10:55 ` Jan Beulich 2019-08-07 12:49 ` Andrew Cooper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.