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

From: Boris Ostrovsky
Date: Fri Jul 11 2014 - 16:31:08 EST


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/