Re: [PATCH 2/2] arch/x86/xen: Silence compiler warnings

[Boris Ostrovsky]

On 07/11/2014 04:10 PM, Daniel Kiper wrote:
> On Fri, Jul 11, 2014 at 04:03:46PM -0400, Boris Ostrovsky wrote:
> > On 07/11/2014 03:54 PM, Daniel Kiper wrote:
> > > Compiler complains in the following way when x86 32-bit kernel
> > > with Xen support is build:
> > >
> > >    CC      arch/x86/xen/enlighten.o
> > > arch/x86/xen/enlighten.c: In function âxen_start_kernelâ:
> > > arch/x86/xen/enlighten.c:1726:3: warning: right shift count >= width of type [enabled by default]
> > >
> > > Such line contains following EFI initialization code:
> > >
> > > boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
> > >
> > > There is no issue if x86 64-bit kernel is build. However, 32-bit case
> > > generate warning (even if that code will not be executed because Xen
> > > does not work on 32-bit EFI platforms) due to __pa() returning unsigned long
> > > type which has 32-bits width. So move whole EFI initialization stuff
> > > to separate function and build its body conditionally to avoid above
> > > mentioned warning on x86 32-bit architecture.
> > >
> > > Signed-off-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
> > > ---
> > >   arch/x86/xen/enlighten.c |   35 ++++++++++++++++++++++-------------
> > >   1 file changed, 22 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > > index bc89647..6abec74 100644
> > > --- a/arch/x86/xen/enlighten.c
> > > +++ b/arch/x86/xen/enlighten.c
> > > @@ -1516,12 +1516,32 @@ static void __init xen_pvh_early_guest_init(void)
> > >   #endif
> > >   }
> > > +static void __init xen_efi_init(void)
> > > +{
> > > +#ifdef CONFIG_XEN_EFI
> > > +	efi_system_table_t *efi_systab_xen;
> > > +
> > > +	efi_systab_xen = xen_efi_probe();
> > > +
> > > +	if (efi_systab_xen == NULL)
> > > +		return;
> > > +
> > > +	strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> > > +			sizeof(boot_params.efi_info.efi_loader_signature));
> > > +	boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
> > > +	boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
> > > +
> > > +	set_bit(EFI_BOOT, &efi.flags);
> > > +	set_bit(EFI_PARAVIRT, &efi.flags);
> > > +	set_bit(EFI_64BIT, &efi.flags);
> > > +#endif
> > > +}
> > > +
> > >   /* First C function to be called on Xen boot */
> > >   asmlinkage __visible void __init xen_start_kernel(void)
> > >   {
> > >   	struct physdev_set_iopl set_iopl;
> > >   	int rc;
> > > -	efi_system_table_t *efi_systab_xen;
> > >   	if (!xen_start_info)
> > >   		return;
> > > @@ -1717,18 +1737,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
> > >   	xen_setup_runstate_info(0);
> > > -	efi_systab_xen = xen_efi_probe();
> > > -
> > > -	if (efi_systab_xen) {
> > > -		strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
> > > -				sizeof(boot_params.efi_info.efi_loader_signature));
> > > -		boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
> > > -		boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
> > > -
> > > -		set_bit(EFI_BOOT, &efi.flags);
> > > -		set_bit(EFI_PARAVIRT, &efi.flags);
> > > -		set_bit(EFI_64BIT, &efi.flags);
> > > -	}
> > > +	xen_efi_init();
> > I'd put ifdef CONFIG_XEN_EFI around the call instead of having it
> > inside the routine.
> Well, I thought about that a bit and I prefer function like Konrad.
> Could you agree with him which solution do you (as maintainers) prefer?

I am not arguing against having a separate routine. All I am saying is 
that calling xen_efi_init() when CONFIG_XEN_EFI is not defined doesn't 
look logical. It will also add an unnecessary call (although compiler 
may optimize it out).

-boris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/