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

[Konrad Rzeszutek Wilk]

On Jul 11, 2014 7:45 PM, Daniel Kiper <daniel.kiper@xxxxxxxxxx> wrote:
>
> On Fri, Jul 11, 2014 at 04:32:27PM -0400, Boris Ostrovsky wrote: 
> > 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 
>
> Ahh... I misunderstood you. However, your proposal, as below: 
>
> #ifdef CONFIG_XEN_EFI 
> Â xen_efi_init(); 
> #endif 
>
> does not solve the problem because this vulnerable shift will be still 
> visible for compiler during x86 32-bit kernel build. 
>
> > compiler may optimize it out). 
>
> Please loot at arch/x86/xen/enlighten.c:xen_check_mwait() and 
> arch/x86/xen/enlighten.c:xen_boot_params_init_edd() (probably 
> there are more stuff like that around). As I can see this is fairly 
> common solution and probably compiler cope with it quite well. 
>

Those are some examples of some rather bad examples.

The way that is preferred in the Linux code is to have the ifdef in headers.

See
http://lxr.free-electrons.com/source/arch/x86/include/asm/xen/swiotlb-xen.h
Or
http://lxr.free-electrons.com/source/arch/x86/include/asm/xen/pci.h

You can create a similar file there and for the 32 bit implementation just make an empty static function.

The 64 bit implementation has to be somewhere. Can it be in the Xen EFI file which is only compiled on 64 bit platforms?

> Have a nice weekend, 
>
> Daniel