Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.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 13:19:20 +0100
Message-ID: <7a920e20-c6f9-4276-ef30-679b77c074ac@citrix.com> (raw)
In-Reply-To: <e686a008-1e0f-c82e-e9ab-83e966d11243@suse.com>

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

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.

>
> --- /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);
}

~Andrew

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

  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 [this message]
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 ` [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 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=7a920e20-c6f9-4276-ef30-679b77c074ac@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=jbeulich@suse.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