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

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

[Daniel Kiper]

On Fri, Jul 11, 2014 at 08:14:51PM -0400, Konrad Rzeszutek Wilk wrote:
>
> On Jul 11, 2014 7:45 PM, Daniel Kiper <daniel.kiper@oracle.com> 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@oracle.com>
> > > >>>---
> > > >>>  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.

What is wrong with them?

> 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?

OK, this (putting declaration/definition in *.h file) makes sens if you
declare/define functions which must be called from different places.
However, xen_efi_init() is called only once in arch/x86/xen/enlighten.c.
Of course, I could define this function here in similar way like it is done
in above headers but it take a bit more place. However, if you wish why not.

Daniel

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

[Konrad Rzeszutek Wilk]

. snip ..
> > > 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.
> 
> What is wrong with them?

#ifdef should not be in C files. It is making the code a bit of a mess.
> 
> > 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?
> 
> OK, this (putting declaration/definition in *.h file) makes sens if you
> declare/define functions which must be called from different places.

Right.
> However, xen_efi_init() is called only once in arch/x86/xen/enlighten.c.

And the vga (see arch/x86/xen/vga.c) is also called only once.

> Of course, I could define this function here in similar way like it is done
> in above headers but it take a bit more place. However, if you wish why not.

I was thinking something along this (not compile tested):

>From 436461a33cf93eed2cd96774bfca78fb08930de1 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Fri, 11 Jul 2014 22:30:33 -0400
Subject: [PATCH] Like this.

---
 arch/x86/xen/Makefile    |    1 +
 arch/x86/xen/efi.c       |   22 ++++++++++++++++++++++
 arch/x86/xen/enlighten.c |   23 +----------------------
 arch/x86/xen/xen-ops.h   |    8 ++++++++
 4 files changed, 32 insertions(+), 22 deletions(-)
 create mode 100644 arch/x86/xen/efi.c

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index b187df5..aa045ad 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= spinlock.o
 obj-$(CONFIG_XEN_DEBUG_FS)	+= debugfs.o
 obj-$(CONFIG_XEN_DOM0)		+= apic.o vga.o
 obj-$(CONFIG_SWIOTLB_XEN)	+= pci-swiotlb-xen.o
+obj-$(CONFIG_XEN_EFI)		+= efi.c
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
new file mode 100644
index 0000000..0b751d9
--- /dev/null
+++ b/arch/x86/xen/efi.c
@@ -0,0 +1,22 @@
+/* Need some headers. */
+
+extern efi_system_table_t __init *xen_efi_probe(void);
+
+void __init xen_efi_init(void)
+{
+	efi_system_table_t *efi_systab_xen;
+
+	efi_systab_xen = xen_efi_probe();
+
+	if (!efi_systab_xen)
+		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_NO_DIRECT, &efi.flags);
+	set_bit(EFI_64BIT, &efi.flags);
+}
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index abd8013..2d71db3 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -152,15 +152,6 @@ struct shared_info *HYPERVISOR_shared_info = &xen_dummy_shared_info;
  */
 static int have_vcpu_info_placement = 1;
 
-#ifdef CONFIG_XEN_EFI
-extern efi_system_table_t __init *xen_efi_probe(void);
-#else
-static efi_system_table_t __init *xen_efi_probe(void)
-{
-	return NULL;
-}
-#endif
-
 struct tls_descs {
 	struct desc_struct desc[3];
 };
@@ -1581,7 +1572,6 @@ 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;
@@ -1777,18 +1767,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_NO_DIRECT, &efi.flags);
-		set_bit(EFI_64BIT, &efi.flags);
-	}
+	xen_efi_init();
 
 	/* Start the world */
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 12a884d..0908dec 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -127,4 +127,12 @@ __visible void xen_adjust_exception_frame(void);
 extern int xen_panic_handler_init(void);
 
 void xen_pvh_secondary_vcpu_init(int cpu);
+
+#ifdef CONFIG_X86_64
+void __init xen_efi_init(void);
+#else
+static inline void __init xen_efi_init(void)
+{
+}
+#endif
 #endif /* XEN_OPS_H */
-- 
1.7.7.6

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

[Konrad Rzeszutek Wilk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 5289 bytes --]


On Jul 11, 2014 7:45 PM, Daniel Kiper <daniel.kiper@oracle.com> 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@oracle.com> 
> > >>>--- 
> > >>>  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 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 0/2] xen: Silence compiler warnings

[Daniel Kiper]

Hi,

Here are two patches that follow earlier Xen dom0 EFI series and
fix some compiler warnings found during detailed build tests.

Both patches should be applied to:

git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next

Daniel

PS I will be on vacation from 14th Jul till 25th Jul.

 arch/x86/xen/enlighten.c |   35 ++++++++++++++++++++++-------------
 include/xen/xen-ops.h    |    2 +-
 2 files changed, 23 insertions(+), 14 deletions(-)

Daniel Kiper (2):
      xen: Silence compiler warnings
      arch/x86/xen: Silence compiler warnings

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

[Daniel Kiper]

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@oracle.com>
---
 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();
 
 	/* Start the world */
 #ifdef CONFIG_X86_32
-- 
1.7.10.4

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

[Boris Ostrovsky]

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@oracle.com>
> ---
>   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.

-boris

>   
>   	/* Start the world */
>   #ifdef CONFIG_X86_32

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

[Daniel Kiper]

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@oracle.com>
> >---
> >  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?

Daniel

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@oracle.com>
>>> ---
>>>   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

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

[Daniel Kiper]

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@oracle.com>
> >>>---
> >>>  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.

Have a nice weekend,

Daniel