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.
> Fixes: a0f60f8431999bf5 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
> Reported-by: kbuild test robot <[email protected]>
> Reported-by: Philipp Rudo <[email protected]>
> Suggested-by: Philipp Rudo <[email protected]>
> CC: Vasily Gorbik <[email protected]>
> Signed-off-by: Claudio Imbrenda <[email protected]>
> ---
> 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.
Reviewed-by: Vasily Gorbik <[email protected]>
On Thu, 23 Apr 2020 14:54:22 +0200
Vasily Gorbik <[email protected]> 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 <[email protected]> Reported-by: Philipp Rudo
> > <[email protected]> Suggested-by: Philipp Rudo
> > <[email protected]> CC: Vasily Gorbik <[email protected]>
> > Signed-off-by: Claudio Imbrenda <[email protected]>
> > ---
> > 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 <[email protected]>
On 23.04.20 14:54, Vasily Gorbik 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.
>
>> Fixes: a0f60f8431999bf5 ("s390/protvirt: Add sysfs firmware interface for Ultravisor information")
>> Reported-by: kbuild test robot <[email protected]>
>> Reported-by: Philipp Rudo <[email protected]>
>> Suggested-by: Philipp Rudo <[email protected]>
>> CC: Vasily Gorbik <[email protected]>
>> Signed-off-by: Claudio Imbrenda <[email protected]>
>> ---
>> 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.
>
> Reviewed-by: Vasily Gorbik <[email protected]>
Acked-by: Christian Borntraeger <[email protected]>
Vasily, I guess you have a pull request soon? Do you want to pick this?
On Fri, Apr 24, 2020 at 03:17:11PM +0200, Christian Borntraeger wrote:
>
> Acked-by: Christian Borntraeger <[email protected]>
>
> Vasily, I guess you have a pull request soon? Do you want to pick this?
yes, I'll pick this up.