From: Jan Beulich <jbeulich@suse.com> To: Andrew Cooper <andrew.cooper3@citrix.com>, "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org> Cc: "Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com> Subject: Re: [Xen-devel] [PATCH v2 2/2] x86/desc: Build boot_{, compat_}gdt[] in C Date: Fri, 9 Aug 2019 14:43:23 +0200 Message-ID: <f7bbe866-aee9-fed5-0789-4e6018e6c83b@suse.com> (raw) In-Reply-To: <7a920e20-c6f9-4276-ef30-679b77c074ac@citrix.com> On 09.08.2019 14:19, Andrew Cooper wrote: > On 09/08/2019 11:40, Jan Beulich wrote: >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> Introduce SEL2GDT(). Correct GDT indices in public header. > > "Correct" here is ambiguous because it implies there is a breakage. > > You appear to have reversed FLAT_RING3_CS64 and DS32 (retaining the > original comments) and changed the comments for FLAT_RING3_SS{32,64}. Well - the comments were what was wrong after all. > Except that now they are out of their logical order (CS 32 then 64, DS > 32 then 64, SS 32 then 64). > > What is the reasoning for the new order? It isn't sorted by index. It is, as much as it looks to have been the original author's intent. I.e. I didn't re-order things across the nul selector between the two groups.I can pull FLAT_RING3_SS* up if that's what you want. I'm also happy with any other order that you may prefer - just let me know which one. All I care about is for the comments to be in sync with the actual values. >> --- /dev/null >> +++ b/xen/arch/x86/desc.c >> @@ -0,0 +1,109 @@ >> + >> +#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 }, > > It would be better if the = { } were vertically aligned. It makes > reading them easier. > > Also, now that SEL2GDT() is present, we need a BUILD_BUG_ON() to check > that the size doesn't vary from one page. At the moment, changing a > selector to use 0xfxxx will cause this to grow beyond 1 page without any > compiler diagnostic. I'd go with > > static void __init __maybe_unused > build_assertions(void) > > { > BUILD_BUG_ON(sizeof(boot_gdt) != PAGE_SIZE); > BUILD_BUG_ON(sizeof(boot_compat_gdt) != PAGE_SIZE); > } Will do, albeit for the build assertions this isn't really the right place imo, because this isn't the place where we depend on them being just single pages. I'll put it there nevertheless, but I'll add a comment for why they're there. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply index Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-09 10:34 [Xen-devel] [PATCH v2 0/2] x86/boot: cleanup 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 [this message] 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 ` [Xen-devel] [PATCH v2 0/2] x86/boot: cleanup Jan Beulich 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 publicly 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=f7bbe866-aee9-fed5-0789-4e6018e6c83b@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 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.git