Re: [PATCH v1 1/1] s390/protvirt: fix compilation issue

[Claudio Imbrenda]

On Thu, 23 Apr 2020 14:54:22 +0200
Vasily Gorbik <gor@xxxxxxxxxxxxx> wrote:

> On Thu, Apr 23, 2020 at 02:01:14PM +0200, Claudio Imbrenda wrote:
> > The kernel fails to compile with
> > CONFIG_PROTECTED_VIRTUALIZATION_GUEST set but CONFIG_KVM unset.
> > 
> > This patch fixes the issue by making the needed variable always
> > available.  
> 
> This statement confuses me a bit.
> 
> It's worth to mention that both arch/s390/boot/uv.c (for the
> decompressor) and arch/s390/kernel/uv.c (for the main kernel) are only
> built when either CONFIG_PROTECTED_VIRTUALIZATION_GUEST or
> CONFIG_KVM is enabled.
> Both arch/s390/boot/Makefile and arch/s390/kernel/Makefile contain:
> obj-$(findstring y, $(CONFIG_PROTECTED_VIRTUALIZATION_GUEST)
> $(CONFIG_PGSTE))   += uv.o
> 
> So this makes the variable available when
> CONFIG_PROTECTED_VIRTUALIZATION_GUEST or CONFIG_KVM (expressed via
> CONFIG_PGSTE) is enabled. Hence no need for extra conditions for
> variable declaration.

yes, "always available" when the file is compiled at all, obviously.

in fact, there are some probably useless ifdefs in that file too, but
this patch should only address the bug

> > Fixes: a0f60f8431999bf5 ("s390/protvirt: Add sysfs firmware
> > interface for Ultravisor information") Reported-by: kbuild test
> > robot <lkp@xxxxxxxxx> Reported-by: Philipp Rudo
> > <prudo@xxxxxxxxxxxxx> Suggested-by: Philipp Rudo
> > <prudo@xxxxxxxxxxxxx> CC: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> > Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > ---
> >  arch/s390/boot/uv.c   | 2 --
> >  arch/s390/kernel/uv.c | 3 ++-
> >  2 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> > index 8fde561f1d07..f887a479cdc7 100644
> > --- a/arch/s390/boot/uv.c
> > +++ b/arch/s390/boot/uv.c
> > @@ -7,9 +7,7 @@
> >  #ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> >  int __bootdata_preserved(prot_virt_guest);
> >  #endif
> > -#if IS_ENABLED(CONFIG_KVM)
> >  struct uv_info __bootdata_preserved(uv_info);
> > -#endif
> >  
> >  void uv_query_info(void)
> >  {
> > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> > index c86d654351d1..4c0677fc8904 100644
> > --- a/arch/s390/kernel/uv.c
> > +++ b/arch/s390/kernel/uv.c
> > @@ -23,10 +23,11 @@
> >  int __bootdata_preserved(prot_virt_guest);
> >  #endif
> >  
> > +struct uv_info __bootdata_preserved(uv_info);
> > +
> >  #if IS_ENABLED(CONFIG_KVM)
> >  int prot_virt_host;
> >  EXPORT_SYMBOL(prot_virt_host);
> > -struct uv_info __bootdata_preserved(uv_info);
> >  EXPORT_SYMBOL(uv_info);  
> 
> hm, EXPORT_SYMBOL(uv_info) is not needed without CONFIG_KVM and this
> saves 1 symbol export, but I'd still made EXPORT_SYMBOL follow the
> declaration immediately. Documentation/process/coding-style.rst
> mentions that only for function declarations though.

I was wondering the same regarding the export.

I don't have any strong opinions, so if anyone has a preference, speak
up and I can fix it.

> Reviewed-by: Vasily Gorbik <gor@xxxxxxxxxxxxx>