Re: [Xen-devel] [PATCH 1/4] xen/PVH: Replace GDT_ENTRY with explicit constant

[Roger Pau MonnÃ]

On Mon, Apr 30, 2018 at 02:07:43PM -0400, Boris Ostrovsky wrote:
> On 04/30/2018 12:57 PM, Roger Pau Monné wrote:
> > On Mon, Apr 30, 2018 at 12:23:36PM -0400, Boris Ostrovsky wrote:
> >> Latest binutils release (2.29.1) will no longer allow proper computation
> >> of GDT entries on 32-bits, with warning:
> >>
> >> arch/x86/xen/xen-pvh.S: Assembler messages:
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (40 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:150: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (40 is not between 0 and 31)
> >> arch/x86/xen/xen-pvh.S:152: Warning: shift count out of range (32 is not between 0 and 31)
> >>
> >> Use explicit value of the entry instead of using GDT_ENTRY() macro.
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> ---
> >>  arch/x86/xen/xen-pvh.S | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/x86/xen/xen-pvh.S b/arch/x86/xen/xen-pvh.S
> >> index e1a5fbe..934f7d4 100644
> >> --- a/arch/x86/xen/xen-pvh.S
> >> +++ b/arch/x86/xen/xen-pvh.S
> >> @@ -145,11 +145,11 @@ gdt_start:
> >>  	.quad 0x0000000000000000            /* NULL descriptor */
> >>  	.quad 0x0000000000000000            /* reserved */
> >>  #ifdef CONFIG_X86_64
> >> -	.quad GDT_ENTRY(0xa09a, 0, 0xfffff) /* __KERNEL_CS */
> >> +	.quad 0x00af9a000000ffff            /* __BOOT_CS */
> >>  #else
> >> -	.quad GDT_ENTRY(0xc09a, 0, 0xfffff) /* __KERNEL_CS */
> >> +	.quad 0x00cf9a000000ffff            /* __BOOT_CS */
> > Maybe it would be cleaner to use something like:
> 
> I actually considered all of these and ended up with a raw number
> because it seems to be a convention in kernel (and Xen too, apparently) 
> to use raw values in .S files.
> 
> Kernel is using now GDT_ENTRY_INIT() which is a C macro. There is one
> other location where GDT_INIT() is used (arch/x86/boot/pm.c) and,
> incidentally, it also generates this warning IIRC.
> 
> I really don't want to move definition to C code just to use a macro ---
> I don't think C code needs to be exposed to this GDT.
> 
> 
> >
> > .word 0xffff /* limit */
> > .word 0      /* base */
> > .byte 0      /* base */
> > .byte 0x9a   /* access */
> > #ifdef CONFIG_X86_64
> > .byte 0xaf   /* flags plus limit */
> > #else
> > .byte 0xcf   /* flags plus limit */
> > #endif
> > .byte 0      /* base */
> 
> 
> I, in fact, started with something like this. But if you repeat this 4
> times you will probably see why I decided against it ;-)

Heh, right. Maybe a .macro to generate those? Or this is all too much
for just a couple of GDT entries anyway...

For long mode however you could use simpler values, AFAICT the code
segment in long mode could be simplified to:

0x00209a0000000000

Because the base/limit have no effect.

In any case I'm not specially inclined either way, and maybe using
similar values for 32 and 64bit modes makes this easier to understand
(and decode if needed).

Roger.