[PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Huacai Chen]

Only Loongson64 platform has and needs loongson_regs.h, including it
unconditionally will cause build errors.

Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3")
Cc: stable@vger.kernel.org
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 arch/mips/kvm/vz.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
index 3932f76..a474578 100644
--- a/arch/mips/kvm/vz.c
+++ b/arch/mips/kvm/vz.c
@@ -29,7 +29,9 @@
 #include <linux/kvm_host.h>
 
 #include "interrupt.h"
+#ifdef CONFIG_CPU_LOONGSON64
 #include "loongson_regs.h"
+#endif
 
 #include "trace.h"
 
-- 
2.7.0

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Greg KH]

On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote:
> Only Loongson64 platform has and needs loongson_regs.h, including it
> unconditionally will cause build errors.
> 
> Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3")
> Cc: stable@vger.kernel.org
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  arch/mips/kvm/vz.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
> index 3932f76..a474578 100644
> --- a/arch/mips/kvm/vz.c
> +++ b/arch/mips/kvm/vz.c
> @@ -29,7 +29,9 @@
>  #include <linux/kvm_host.h>
>  
>  #include "interrupt.h"
> +#ifdef CONFIG_CPU_LOONGSON64
>  #include "loongson_regs.h"
> +#endif

The fix for this should be in the .h file, no #ifdef should be needed in
a .c file.

thanks,

greg k-h

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Jiaxun Yang]

在 2020/8/8 下午11:31, Greg KH 写道:
> On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote:
>> Only Loongson64 platform has and needs loongson_regs.h, including it
>> unconditionally will cause build errors.
>>
>> Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3")
>> Cc: stable@vger.kernel.org
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> ---
>>   arch/mips/kvm/vz.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
>> index 3932f76..a474578 100644
>> --- a/arch/mips/kvm/vz.c
>> +++ b/arch/mips/kvm/vz.c
>> @@ -29,7 +29,9 @@
>>   #include <linux/kvm_host.h>
>>   
>>   #include "interrupt.h"
>> +#ifdef CONFIG_CPU_LOONGSON64
>>   #include "loongson_regs.h"
>> +#endif
> The fix for this should be in the .h file, no #ifdef should be needed in
> a .c file.

The header file can only be reached when CONFIG_CPU_LOONGSON64 is 
selected...
Otherwise the include path of this file won't be a part of CFLAGS.

Thanks.

- Jiaxun

>
> thanks,
>
> greg k-h

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Greg KH]

On Sat, Aug 08, 2020 at 11:35:54PM +0800, Jiaxun Yang wrote:
> 
> 
> 在 2020/8/8 下午11:31, Greg KH 写道:
> > On Sat, Aug 08, 2020 at 08:50:52PM +0800, Huacai Chen wrote:
> > > Only Loongson64 platform has and needs loongson_regs.h, including it
> > > unconditionally will cause build errors.
> > > 
> > > Fixes: 7f2a83f1c2a941ebfee5 ("KVM: MIPS: Add CPUCFG emulation for Loongson-3")
> > > Cc: stable@vger.kernel.org
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > > ---
> > >   arch/mips/kvm/vz.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
> > > index 3932f76..a474578 100644
> > > --- a/arch/mips/kvm/vz.c
> > > +++ b/arch/mips/kvm/vz.c
> > > @@ -29,7 +29,9 @@
> > >   #include <linux/kvm_host.h>
> > >   #include "interrupt.h"
> > > +#ifdef CONFIG_CPU_LOONGSON64
> > >   #include "loongson_regs.h"
> > > +#endif
> > The fix for this should be in the .h file, no #ifdef should be needed in
> > a .c file.
> 
> The header file can only be reached when CONFIG_CPU_LOONGSON64 is
> selected...
> Otherwise the include path of this file won't be a part of CFLAGS.

That sounds like you should fix up the path of this file in the
#include line as well :)

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Paolo Bonzini]

On 09/08/20 09:02, Greg KH wrote:
>>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
>>>> index 3932f76..a474578 100644
>>>> --- a/arch/mips/kvm/vz.c
>>>> +++ b/arch/mips/kvm/vz.c
>>>> @@ -29,7 +29,9 @@
>>>>   #include <linux/kvm_host.h>
>>>>   #include "interrupt.h"
>>>> +#ifdef CONFIG_CPU_LOONGSON64
>>>>   #include "loongson_regs.h"
>>>> +#endif
>>> The fix for this should be in the .h file, no #ifdef should be needed in
>>> a .c file.
>> The header file can only be reached when CONFIG_CPU_LOONGSON64 is
>> selected...
>> Otherwise the include path of this file won't be a part of CFLAGS.
> That sounds like you should fix up the path of this file in the
> #include line as well :)
> 

There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
more #include "loongson_regs.h" in arch/mips.  So while I agree with
Greg that this idiom is quite unusual, it seems to be the expected way
to use this header.  I queued the patch.

Paolo

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Sasha Levin]

On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote:
>On 09/08/20 09:02, Greg KH wrote:
>>>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
>>>>> index 3932f76..a474578 100644
>>>>> --- a/arch/mips/kvm/vz.c
>>>>> +++ b/arch/mips/kvm/vz.c
>>>>> @@ -29,7 +29,9 @@
>>>>>   #include <linux/kvm_host.h>
>>>>>   #include "interrupt.h"
>>>>> +#ifdef CONFIG_CPU_LOONGSON64
>>>>>   #include "loongson_regs.h"
>>>>> +#endif
>>>> The fix for this should be in the .h file, no #ifdef should be needed in
>>>> a .c file.
>>> The header file can only be reached when CONFIG_CPU_LOONGSON64 is
>>> selected...
>>> Otherwise the include path of this file won't be a part of CFLAGS.
>> That sounds like you should fix up the path of this file in the
>> #include line as well :)
>>
>
>There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
>more #include "loongson_regs.h" in arch/mips.  So while I agree with
>Greg that this idiom is quite unusual, it seems to be the expected way
>to use this header.  I queued the patch.

Where is that header coming from anyway? I can't find it in the tree,
nor anything that would generate it.

-- 
Thanks,
Sasha

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Sasha Levin]

On Sun, Aug 09, 2020 at 01:38:22PM -0400, Sasha Levin wrote:
>On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote:
>>On 09/08/20 09:02, Greg KH wrote:
>>>>>>diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
>>>>>>index 3932f76..a474578 100644
>>>>>>--- a/arch/mips/kvm/vz.c
>>>>>>+++ b/arch/mips/kvm/vz.c
>>>>>>@@ -29,7 +29,9 @@
>>>>>>  #include <linux/kvm_host.h>
>>>>>>  #include "interrupt.h"
>>>>>>+#ifdef CONFIG_CPU_LOONGSON64
>>>>>>  #include "loongson_regs.h"
>>>>>>+#endif
>>>>>The fix for this should be in the .h file, no #ifdef should be needed in
>>>>>a .c file.
>>>>The header file can only be reached when CONFIG_CPU_LOONGSON64 is
>>>>selected...
>>>>Otherwise the include path of this file won't be a part of CFLAGS.
>>>That sounds like you should fix up the path of this file in the
>>>#include line as well :)
>>>
>>
>>There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
>>more #include "loongson_regs.h" in arch/mips.  So while I agree with
>>Greg that this idiom is quite unusual, it seems to be the expected way
>>to use this header.  I queued the patch.
>
>Where is that header coming from anyway? I can't find it in the tree,
>nor anything that would generate it.

Woops, nevermind - sorry for the noise.

-- 
Thanks,
Sasha

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Greg KH]

On Sun, Aug 09, 2020 at 07:23:28PM +0200, Paolo Bonzini wrote:
> On 09/08/20 09:02, Greg KH wrote:
> >>>> diff --git a/arch/mips/kvm/vz.c b/arch/mips/kvm/vz.c
> >>>> index 3932f76..a474578 100644
> >>>> --- a/arch/mips/kvm/vz.c
> >>>> +++ b/arch/mips/kvm/vz.c
> >>>> @@ -29,7 +29,9 @@
> >>>>   #include <linux/kvm_host.h>
> >>>>   #include "interrupt.h"
> >>>> +#ifdef CONFIG_CPU_LOONGSON64
> >>>>   #include "loongson_regs.h"
> >>>> +#endif
> >>> The fix for this should be in the .h file, no #ifdef should be needed in
> >>> a .c file.
> >> The header file can only be reached when CONFIG_CPU_LOONGSON64 is
> >> selected...
> >> Otherwise the include path of this file won't be a part of CFLAGS.
> > That sounds like you should fix up the path of this file in the
> > #include line as well :)
> > 
> 
> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
> more #include "loongson_regs.h" in arch/mips.  So while I agree with
> Greg that this idiom is quite unusual, it seems to be the expected way
> to use this header.  I queued the patch.

Or you all could fix it up to work properly like all other #include
lines in the kernel source tree.  There's no reason mips should be
"special" here, right?

thanks,

greg k-h

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Paolo Bonzini]

On 10/08/20 09:44, Greg KH wrote:
>> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
>> more #include "loongson_regs.h" in arch/mips.  So while I agree with
>> Greg that this idiom is quite unusual, it seems to be the expected way
>> to use this header.  I queued the patch.
> Or you all could fix it up to work properly like all other #include
> lines in the kernel source tree.  There's no reason mips should be
> "special" here, right?

It's not just this #include, there's a couple dozen mach-* directories;
changing how they work would be up to the MIPS maintainers (CCed), and
it would certainly not be a patch that can be merged in stable@ kernels.

arch/mips/kernel/cpu-probe.c has the same

#ifdef CONFIG_CPU_LOONGSON64
#include <loongson_regs.h>

for example, so apparently they're good with this.  So if I don't pick
up the patch to fix the build it would be in all likelihood merged by
MIPS maintainers.  The only difference will be how long the build
remains broken and the fact that they need to worry about KVM despite
the presence of a specific maintainer.

KVM could of course just #include <asm/mach-loongson64/loongson_regs.h>,
which would be found unconditionally.  But there is some assembly in the
header, so even if it would compile (I didn't check) it seems wrong to
include it unconditionally and in fact it would be the only case of a
file including <asm/mach-*/*.h> even if it is not compiled for that
platform.

Another alternative would be to move CONFIG_CPU_LOONGSON64 code out of
arch/mips/kvm/vz.c and include it with obj-$(CONFIG_CPU_LOONGSON64).
I'll gladly accept a patch to do that, but I won't write it since I
don't have access to the hardware in order to test it.  For now, and for
the immediate purpose of not breaking the build, when in Rome I'll do as
the Romans do.

Paolo

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Greg KH]

On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote:
> On 10/08/20 09:44, Greg KH wrote:
> >> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
> >> more #include "loongson_regs.h" in arch/mips.  So while I agree with
> >> Greg that this idiom is quite unusual, it seems to be the expected way
> >> to use this header.  I queued the patch.
> > Or you all could fix it up to work properly like all other #include
> > lines in the kernel source tree.  There's no reason mips should be
> > "special" here, right?
> 
> It's not just this #include, there's a couple dozen mach-* directories;
> changing how they work would be up to the MIPS maintainers (CCed), and
> it would certainly not be a patch that can be merged in stable@ kernels.
> 
> arch/mips/kernel/cpu-probe.c has the same
> 
> #ifdef CONFIG_CPU_LOONGSON64
> #include <loongson_regs.h>
> 
> for example, so apparently they're good with this.  So if I don't pick
> up the patch to fix the build it would be in all likelihood merged by
> MIPS maintainers.  The only difference will be how long the build
> remains broken and the fact that they need to worry about KVM despite
> the presence of a specific maintainer.

Ok, fair enough, but in the long-run, this should probably be fixed up
"properly" if this arch is still being maintained.

thanks,

greg k-h

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Jiaxun Yang]

在 2020/8/10 下午5:03, Greg KH 写道:
> On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote:
>> On 10/08/20 09:44, Greg KH wrote:
>>>> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
>>>> more #include "loongson_regs.h" in arch/mips.  So while I agree with
>>>> Greg that this idiom is quite unusual, it seems to be the expected way
>>>> to use this header.  I queued the patch.
>>> Or you all could fix it up to work properly like all other #include
>>> lines in the kernel source tree.  There's no reason mips should be
>>> "special" here, right?
>> It's not just this #include, there's a couple dozen mach-* directories;
>> changing how they work would be up to the MIPS maintainers (CCed), and
>> it would certainly not be a patch that can be merged in stable@ kernels.
>>
>> arch/mips/kernel/cpu-probe.c has the same
>>
>> #ifdef CONFIG_CPU_LOONGSON64
>> #include <loongson_regs.h>
>>
>> for example, so apparently they're good with this.  So if I don't pick
>> up the patch to fix the build it would be in all likelihood merged by
>> MIPS maintainers.  The only difference will be how long the build
>> remains broken and the fact that they need to worry about KVM despite
>> the presence of a specific maintainer.
> Ok, fair enough, but in the long-run, this should probably be fixed up
> "properly" if this arch is still being maintained.

MIPS is using another approach to management machine specified headers.
It changes include path per-machine to get ride of the generic one.

I'm a little bit confused about the proper way to do that. I thought the 
design
overall, is just fine.

Should we try to migrate it to another style?

Thanks.

- Jiaxun
>
> thanks,
>
> greg k-h

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Thomas Bogendoerfer]

On Mon, Aug 10, 2020 at 11:03:10AM +0200, Greg KH wrote:
> On Mon, Aug 10, 2020 at 10:56:48AM +0200, Paolo Bonzini wrote:
> > On 10/08/20 09:44, Greg KH wrote:
> > >> There is more #ifdef CONFIG_CPU_LOONGSON64 in arch/mips/kvm/vz.c, and
> > >> more #include "loongson_regs.h" in arch/mips.  So while I agree with
> > >> Greg that this idiom is quite unusual, it seems to be the expected way
> > >> to use this header.  I queued the patch.
> > > Or you all could fix it up to work properly like all other #include
> > > lines in the kernel source tree.  There's no reason mips should be
> > > "special" here, right?
> > 
> > It's not just this #include, there's a couple dozen mach-* directories;
> > changing how they work would be up to the MIPS maintainers (CCed), and
> > it would certainly not be a patch that can be merged in stable@ kernels.
> > 
> > arch/mips/kernel/cpu-probe.c has the same
> > 
> > #ifdef CONFIG_CPU_LOONGSON64
> > #include <loongson_regs.h>
> > 
> > for example, so apparently they're good with this.  So if I don't pick
> > up the patch to fix the build it would be in all likelihood merged by
> > MIPS maintainers.  The only difference will be how long the build
> > remains broken and the fact that they need to worry about KVM despite
> > the presence of a specific maintainer.
> 
> Ok, fair enough, but in the long-run, this should probably be fixed up
> "properly" if this arch is still being maintained.

I have it on my todo list. My plan is to move stuff out of mach-* directories,
which aren't needed there. This should solve issues like the one here.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: VZ: Only include loongson_regs.h for CPU_LOONGSON64

[Maciej W. Rozycki]

On Mon, 10 Aug 2020, Thomas Bogendoerfer wrote:

> > > It's not just this #include, there's a couple dozen mach-* directories;
> > > changing how they work would be up to the MIPS maintainers (CCed), and
> > > it would certainly not be a patch that can be merged in stable@ kernels.
> > > 
> > > arch/mips/kernel/cpu-probe.c has the same
> > > 
> > > #ifdef CONFIG_CPU_LOONGSON64
> > > #include <loongson_regs.h>
> > > 
> > > for example, so apparently they're good with this.  So if I don't pick
> > > up the patch to fix the build it would be in all likelihood merged by
> > > MIPS maintainers.  The only difference will be how long the build
> > > remains broken and the fact that they need to worry about KVM despite
> > > the presence of a specific maintainer.
> > 
> > Ok, fair enough, but in the long-run, this should probably be fixed up
> > "properly" if this arch is still being maintained.
> 
> I have it on my todo list. My plan is to move stuff out of mach-* directories,
> which aren't needed there. This should solve issues like the one here.

 Correct, it looks like another maintainer's oversight.

 The asm/mach-<platform>/ directories are there for platform variants of 
generic stuff found in asm/mach-generic/.  So if something is not there in 
asm/mach-generic/, then it must not be in any other asm/mach-<plaftorm>/ 
subdirectory either.

 Regular platform headers need to go under asm/<plaftorm>/.  Compare 
asm/mach-dec/ vs asm/dec/, or asm/mach-sibyte/ vs asm/sibyte/, and so on.  
So this `loongson_regs.h' piece belongs to asm/loongson64/ rather than 
asm/mach-loongson64/.

  Maciej