| Message ID | 20221010013329.199167-1-vernon2gm@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: KVM: fixup undefined reference to riscv_cbom_block_size | expand |
On Mon, Oct 10, 2022 at 09:33:29AM +0800, Vernon Yang wrote: > When some RISC-V compilers do not support the Zicbom extension, > the build system auto disable the CONFIG_RISCV_ISA_ZICBOM, so the > source code of the relevant function is not compiled, resulting > in the definition of the riscv_cbom_block_size variable cannot > be found Hmm, my understanding was that riscv_cbom_block_size was not supposed to depend on CONFIG_RISCV_ISA_ZICBOM because the thead is able to use it even if the toolchain does not support it. The code in cacheflush.h looks like: extern unsigned int riscv_cbom_block_size; #ifdef CONFIG_RISCV_ISA_ZICBOM void riscv_init_cbom_blocksize(void); #else static inline void riscv_init_cbom_blocksize(void) { } #endif #ifdef CONFIG_RISCV_DMA_NONCOHERENT void riscv_noncoherent_supported(void); #endif It's early and I only had a quick look but I think that this is not defined because RISCV_DMA_NONCOHERENT is not defined, not because of RISCV_ISA_ZICBOM. I'm not the KVM maintainer, but I dislike #ifdefery in c files, so it'd be nice I think to sort this out in the header and not have to worry about guarding the variable. That's my 0.02 €.. Thanks, Conor. > So add conditional compilation to fix it > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vernon Yang <vernon2gm@gmail.com> > --- > arch/riscv/kvm/vcpu.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > index a032c4f0d600..08a6c3cb695d 100644 > --- a/arch/riscv/kvm/vcpu.c > +++ b/arch/riscv/kvm/vcpu.c > @@ -256,7 +256,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | > KVM_REG_SIZE_MASK | > KVM_REG_RISCV_CONFIG); > - unsigned long reg_val; > + unsigned long reg_val = 0; > > if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long)) > return -EINVAL; > @@ -268,7 +268,9 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > return -EINVAL; > +#ifdef CONFIG_RISCV_ISA_ZICBOM > reg_val = riscv_cbom_block_size; > +#endif > break; > default: > return -EINVAL; > -- > 2.25.1 > >
On Mon, Oct 10, 2022 at 09:33:29AM +0800, Vernon Yang wrote: > When some RISC-V compilers do not support the Zicbom extension, > the build system auto disable the CONFIG_RISCV_ISA_ZICBOM, so the > source code of the relevant function is not compiled, resulting > in the definition of the riscv_cbom_block_size variable cannot > be found > > So add conditional compilation to fix it > > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Vernon Yang <vernon2gm@gmail.com> > --- > arch/riscv/kvm/vcpu.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > index a032c4f0d600..08a6c3cb695d 100644 > --- a/arch/riscv/kvm/vcpu.c > +++ b/arch/riscv/kvm/vcpu.c > @@ -256,7 +256,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | > KVM_REG_SIZE_MASK | > KVM_REG_RISCV_CONFIG); > - unsigned long reg_val; > + unsigned long reg_val = 0; > > if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long)) > return -EINVAL; > @@ -268,7 +268,9 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > return -EINVAL; > +#ifdef CONFIG_RISCV_ISA_ZICBOM > reg_val = riscv_cbom_block_size; > +#endif > break; Thanks Vernon, I sent a fixup to Anup last week for this, which is similar to your patch, but #ifdef's out the entire case. Thanks, drew > default: > return -EINVAL; > -- > 2.25.1 >
On Mon, Oct 10, 2022 at 07:42:04AM +0100, Conor Dooley wrote: > On Mon, Oct 10, 2022 at 09:33:29AM +0800, Vernon Yang wrote: > > When some RISC-V compilers do not support the Zicbom extension, > > the build system auto disable the CONFIG_RISCV_ISA_ZICBOM, so the > > source code of the relevant function is not compiled, resulting > > in the definition of the riscv_cbom_block_size variable cannot > > be found > > Hmm, my understanding was that riscv_cbom_block_size was not supposed to > depend on CONFIG_RISCV_ISA_ZICBOM because the thead is able to use it > even if the toolchain does not support it. > > The code in cacheflush.h looks like: > extern unsigned int riscv_cbom_block_size; > #ifdef CONFIG_RISCV_ISA_ZICBOM > void riscv_init_cbom_blocksize(void); > #else > static inline void riscv_init_cbom_blocksize(void) { } > #endif > > #ifdef CONFIG_RISCV_DMA_NONCOHERENT > void riscv_noncoherent_supported(void); > #endif > > It's early and I only had a quick look but I think that this is not > defined because RISCV_DMA_NONCOHERENT is not defined, not because of > RISCV_ISA_ZICBOM. thead is able to use riscv_cbom_block_size because it does its own initialization of it and selects RISCV_DMA_NONCOHERENT to get access to it. KVM depends on the initializer in dma-noncoherent.c, which is guarded by RISCV_ISA_ZICBOM and does not select RISCV_DMA_NONCOHERENT, but RISCV_ISA_ZICBOM does. I think guarding use of riscv_cbom_block_size with RISCV_ISA_ZICBOM in KVM makes sense. > I'm not the KVM maintainer, but I dislike #ifdefery > in c files, so it'd be nice I think to sort this out in the header and > not have to worry about guarding the variable. I also dislike #ifdefery, but unless we move riscv_cbom_block_size to an unconditionally built file like cacheflush.c (as Anup once did), then we don't have much choice. Thanks, drew
On Mon, Oct 10, 2022 at 08:59:49AM +0200, Andrew Jones wrote: > On Mon, Oct 10, 2022 at 07:42:04AM +0100, Conor Dooley wrote: > > On Mon, Oct 10, 2022 at 09:33:29AM +0800, Vernon Yang wrote: > > > When some RISC-V compilers do not support the Zicbom extension, > > > the build system auto disable the CONFIG_RISCV_ISA_ZICBOM, so the > > > source code of the relevant function is not compiled, resulting > > > in the definition of the riscv_cbom_block_size variable cannot > > > be found > > > > Hmm, my understanding was that riscv_cbom_block_size was not supposed to > > depend on CONFIG_RISCV_ISA_ZICBOM because the thead is able to use it > > even if the toolchain does not support it. > > > > The code in cacheflush.h looks like: > > extern unsigned int riscv_cbom_block_size; > > #ifdef CONFIG_RISCV_ISA_ZICBOM > > void riscv_init_cbom_blocksize(void); > > #else > > static inline void riscv_init_cbom_blocksize(void) { } > > #endif > > > > #ifdef CONFIG_RISCV_DMA_NONCOHERENT > > void riscv_noncoherent_supported(void); > > #endif > > > > It's early and I only had a quick look but I think that this is not > > defined because RISCV_DMA_NONCOHERENT is not defined, not because of > > RISCV_ISA_ZICBOM. > > thead is able to use riscv_cbom_block_size because it does its own > initialization of it and selects RISCV_DMA_NONCOHERENT to get access > to it. KVM depends on the initializer in dma-noncoherent.c, which is > guarded by RISCV_ISA_ZICBOM and does not select RISCV_DMA_NONCOHERENT, > but RISCV_ISA_ZICBOM does. I think guarding use of riscv_cbom_block_size > with RISCV_ISA_ZICBOM in KVM makes sense. Aight chief, you know the code better than I do :) > > I'm not the KVM maintainer, but I dislike #ifdefery > > in c files, so it'd be nice I think to sort this out in the header and > > not have to worry about guarding the variable. > > I also dislike #ifdefery, but unless we move riscv_cbom_block_size to > an unconditionally built file like cacheflush.c (as Anup once did), then > we don't have much choice. Fair enough. Maybe once fixes, for-next & some of Heiko's cleanups have aligned it'll make sense to do such a change. Sorry for the noise then, Conor.
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index a032c4f0d600..08a6c3cb695d 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -256,7 +256,7 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, unsigned long reg_num = reg->id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_RISCV_CONFIG); - unsigned long reg_val; + unsigned long reg_val = 0; if (KVM_REG_SIZE(reg->id) != sizeof(unsigned long)) return -EINVAL; @@ -268,7 +268,9 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) return -EINVAL; +#ifdef CONFIG_RISCV_ISA_ZICBOM reg_val = riscv_cbom_block_size; +#endif break; default: return -EINVAL;
When some RISC-V compilers do not support the Zicbom extension, the build system auto disable the CONFIG_RISCV_ISA_ZICBOM, so the source code of the relevant function is not compiled, resulting in the definition of the riscv_cbom_block_size variable cannot be found So add conditional compilation to fix it Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Vernon Yang <vernon2gm@gmail.com> --- arch/riscv/kvm/vcpu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)