Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"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 16:25:15 +0100
Message-ID: <749b514a-822b-c86f-8628-422376683267@citrix.com> (raw)
In-Reply-To: <ef79ee24-af84-aabd-95f5-a4363ec2a78d@suse.com>

On 09/08/2019 14:18, Jan Beulich wrote:
> On 09.08.2019 15:07, Andrew Cooper wrote:
>> On 09/08/2019 13:43, Jan Beulich wrote:
>>> On 09.08.2019 14:19, Andrew Cooper wrote:
>>>> On 09/08/2019 11:40, Jan Beulich wrote:
>>>>> --- /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.
>>
>> IMO this is the right place, because it is right beside where the array
>> is specifically defined to be [PAGE_SIZE / sizeof()].
>
> I was about to ask why we then need build_assertions() at all,
> until I also saw ...
>
>> What it is doing is working around what is arguably a compiler bug by
>> allowing foo[x] = { [x + 1] = ... } to work.
>
> ... this. Which made me go check, and both gcc 4.3 and gcc 9.1
> correctly complain "array index in initializer exceeds array
> bounds".
>
>> Anyway, with these assertions and the tweaked constant clenaup,
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Thanks, but as per above I'm now irritated: With the explicit
> specification of the array size, build_assertions() should either
> be dropped again, or its comment be extended to cover why it's
> needed _despite_ the specified array size (i.e. in which case
> your example above would not cause a build failure).

This comes down to a bug I encountered while doing the CPUID work, which
is specifically why we have cpuid.c's build assertions.

Given that we get failures from at least one compiler in CI, we can drop
the extra build assertions.

At some point which isn't now, I'll try to work out exactly what went
wrong in the CPUID case, but I can't reproduce it from memory.

~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
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 [this message]
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=749b514a-822b-c86f-8628-422376683267@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