| Message ID | 20221010094029.1579672-1-ajones@ventanamicro.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM | expand |
On Mon, 10 Oct 2022 02:40:29 PDT (-0700), ajones@ventanamicro.com wrote: > Fix undefined reference of riscv_cbom_block_size when compiling KVM > without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient > guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile > dma-noncoherent.c (which is the file where riscv_cbom_block_size and > its initializer live). > > Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > arch/riscv/kvm/vcpu.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > index a032c4f0d600..e4453caba728 100644 > --- a/arch/riscv/kvm/vcpu.c > +++ b/arch/riscv/kvm/vcpu.c > @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > case KVM_REG_RISCV_CONFIG_REG(isa): > reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; > break; > +#ifdef CONFIG_RISCV_ISA_ZICBOM > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > return -EINVAL; > reg_val = riscv_cbom_block_size; > break; > +#endif > default: > return -EINVAL; > } Thanks, looks like this is tripping up the kernelci builds too: https://linux.kernelci.org/build/id/6343b37c6fd2dcbb5dcab5f3/logs/ . This symbol is actually a bit odd, as we don't actually just use it under Zicbom because of the T-Head stuff. Looks like that's generically broken, though, so IMO we need something like this diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c index 6cb7d96ad9c7..fba86fa3a628 100644 --- a/arch/riscv/mm/cacheflush.c +++ b/arch/riscv/mm/cacheflush.c @@ -5,6 +5,9 @@ #include <asm/cacheflush.h> +unsigned int riscv_cbom_block_size; +EXPORT_SYMBOL(riscv_cbom_block_size); + #ifdef CONFIG_SMP #include <asm/sbi.h> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index e3f9bdf47c5f..33d21ace0971 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -12,7 +12,6 @@ #include <linux/of_device.h> #include <asm/cacheflush.h> -unsigned int riscv_cbom_block_size; static bool noncoherent_supported; void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, which should be Fixes: 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing"). We might also want the ifdef for KVM, as that would allow us to build KVM that doesn't support these VCPU configurations, but that's sort of a different question.
On Mon, Oct 10, 2022 at 11:36:24AM -0700, Palmer Dabbelt wrote: > On Mon, 10 Oct 2022 02:40:29 PDT (-0700), ajones@ventanamicro.com wrote: > > Fix undefined reference of riscv_cbom_block_size when compiling KVM > > without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient > > guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile > > dma-noncoherent.c (which is the file where riscv_cbom_block_size and > > its initializer live). > > > > Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > arch/riscv/kvm/vcpu.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > > index a032c4f0d600..e4453caba728 100644 > > --- a/arch/riscv/kvm/vcpu.c > > +++ b/arch/riscv/kvm/vcpu.c > > @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > > case KVM_REG_RISCV_CONFIG_REG(isa): > > reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; > > break; > > +#ifdef CONFIG_RISCV_ISA_ZICBOM > > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > > return -EINVAL; > > reg_val = riscv_cbom_block_size; > > break; > > +#endif > > default: > > return -EINVAL; > > } > > Thanks, looks like this is tripping up the kernelci builds too: > https://linux.kernelci.org/build/id/6343b37c6fd2dcbb5dcab5f3/logs/ . > This symbol is actually a bit odd, as we don't actually just use it > under Zicbom because of the T-Head stuff. Looks like that's generically > broken, though, so IMO we need something like this > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c > index 6cb7d96ad9c7..fba86fa3a628 100644 > --- a/arch/riscv/mm/cacheflush.c > +++ b/arch/riscv/mm/cacheflush.c > @@ -5,6 +5,9 @@ > > #include <asm/cacheflush.h> > > +unsigned int riscv_cbom_block_size; > +EXPORT_SYMBOL(riscv_cbom_block_size); > + > #ifdef CONFIG_SMP > > #include <asm/sbi.h> > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index e3f9bdf47c5f..33d21ace0971 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,6 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -unsigned int riscv_cbom_block_size; > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > > which should be > > Fixes: 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing"). > > We might also want the ifdef for KVM, as that would allow us to build KVM that > doesn't support these VCPU configurations, but that's sort of a different > question. I assume (or hope?) this approach has the advantage of also fixing: https://lore.kernel.org/all/202210102012.iShn6U6Q-lkp@intel.com/
On Tue, Oct 11, 2022 at 12:06 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Mon, 10 Oct 2022 02:40:29 PDT (-0700), ajones@ventanamicro.com wrote: > > Fix undefined reference of riscv_cbom_block_size when compiling KVM > > without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient > > guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile > > dma-noncoherent.c (which is the file where riscv_cbom_block_size and > > its initializer live). > > > > Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > arch/riscv/kvm/vcpu.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > > index a032c4f0d600..e4453caba728 100644 > > --- a/arch/riscv/kvm/vcpu.c > > +++ b/arch/riscv/kvm/vcpu.c > > @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > > case KVM_REG_RISCV_CONFIG_REG(isa): > > reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; > > break; > > +#ifdef CONFIG_RISCV_ISA_ZICBOM > > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > > return -EINVAL; > > reg_val = riscv_cbom_block_size; > > break; > > +#endif > > default: > > return -EINVAL; > > } > > Thanks, looks like this is tripping up the kernelci builds too: > https://linux.kernelci.org/build/id/6343b37c6fd2dcbb5dcab5f3/logs/ . > This symbol is actually a bit odd, as we don't actually just use it > under Zicbom because of the T-Head stuff. Looks like that's generically > broken, though, so IMO we need something like this > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c > index 6cb7d96ad9c7..fba86fa3a628 100644 > --- a/arch/riscv/mm/cacheflush.c > +++ b/arch/riscv/mm/cacheflush.c > @@ -5,6 +5,9 @@ > > #include <asm/cacheflush.h> > > +unsigned int riscv_cbom_block_size; > +EXPORT_SYMBOL(riscv_cbom_block_size); > + > #ifdef CONFIG_SMP > > #include <asm/sbi.h> > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index e3f9bdf47c5f..33d21ace0971 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,6 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -unsigned int riscv_cbom_block_size; > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > > which should be > > Fixes: 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing"). I agree with your change. May be also move riscv_init_cbom_blocksize() to arch/riscv/mm/cacheflush.c ? > > We might also want the ifdef for KVM, as that would allow us to build KVM that > doesn't support these VCPU configurations, but that's sort of a different > question. Yes, I will be sending a PR including the KVM fix this week itself. > > -- > kvm-riscv mailing list > kvm-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kvm-riscv Regards, Anup
On Mon, Oct 10, 2022 at 11:36:24AM -0700, Palmer Dabbelt wrote: > On Mon, 10 Oct 2022 02:40:29 PDT (-0700), ajones@ventanamicro.com wrote: > > Fix undefined reference of riscv_cbom_block_size when compiling KVM > > without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient > > guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile > > dma-noncoherent.c (which is the file where riscv_cbom_block_size and > > its initializer live). > > > > Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > --- > > arch/riscv/kvm/vcpu.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > > index a032c4f0d600..e4453caba728 100644 > > --- a/arch/riscv/kvm/vcpu.c > > +++ b/arch/riscv/kvm/vcpu.c > > @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > > case KVM_REG_RISCV_CONFIG_REG(isa): > > reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; > > break; > > +#ifdef CONFIG_RISCV_ISA_ZICBOM > > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > > return -EINVAL; > > reg_val = riscv_cbom_block_size; > > break; > > +#endif > > default: > > return -EINVAL; > > } > > Thanks, looks like this is tripping up the kernelci builds too: > https://linux.kernelci.org/build/id/6343b37c6fd2dcbb5dcab5f3/logs/ . > This symbol is actually a bit odd, as we don't actually just use it > under Zicbom because of the T-Head stuff. Looks like that's generically > broken, though, so IMO we need something like this > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c > index 6cb7d96ad9c7..fba86fa3a628 100644 > --- a/arch/riscv/mm/cacheflush.c > +++ b/arch/riscv/mm/cacheflush.c > @@ -5,6 +5,9 @@ > > #include <asm/cacheflush.h> > > +unsigned int riscv_cbom_block_size; > +EXPORT_SYMBOL(riscv_cbom_block_size); > + > #ifdef CONFIG_SMP > > #include <asm/sbi.h> > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index e3f9bdf47c5f..33d21ace0971 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -12,7 +12,6 @@ > #include <linux/of_device.h> > #include <asm/cacheflush.h> > > -unsigned int riscv_cbom_block_size; > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > > which should be > > Fixes: 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing"). Hi Palmer, Should I post your suggestion as a v2 or were you planning to post it? I'm happy to do so, but I'd prefer to also move riscv_init_cbom_blocksize() and drop its #ifdef CONFIG_RISCV_ISA_ZICBOM guard. The reason I'd prefer we always try to parse the block size is that we always parse the ISA string, and, when zicbom is present, we set it in the ISA extension bitmap, even when CONFIG_RISCV_ISA_ZICBOM isn't enabled. This is a reasonable behavior if CONFIG_RISCV_ISA_ZICBOM is meant to decide whether or not the kernel uses the feature for itself, as its Kconfig text implies, rather than to mean we should pretend the feature isn't present, whether it is or not. So, if we're not trying to completely avoid the feature by disabling CONFIG_RISCV_ISA_ZICBOM, then KVM can still inform the guest about it. When KVM does that it also needs the block size. Thanks, drew
On Wed, Oct 12, 2022 at 07:07:49PM +0200, Andrew Jones wrote: > On Mon, Oct 10, 2022 at 11:36:24AM -0700, Palmer Dabbelt wrote: > > On Mon, 10 Oct 2022 02:40:29 PDT (-0700), ajones@ventanamicro.com wrote: > > > Fix undefined reference of riscv_cbom_block_size when compiling KVM > > > without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient > > > guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile > > > dma-noncoherent.c (which is the file where riscv_cbom_block_size and > > > its initializer live). > > > > > > Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > arch/riscv/kvm/vcpu.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c > > > index a032c4f0d600..e4453caba728 100644 > > > --- a/arch/riscv/kvm/vcpu.c > > > +++ b/arch/riscv/kvm/vcpu.c > > > @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, > > > case KVM_REG_RISCV_CONFIG_REG(isa): > > > reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; > > > break; > > > +#ifdef CONFIG_RISCV_ISA_ZICBOM > > > case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): > > > if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) > > > return -EINVAL; > > > reg_val = riscv_cbom_block_size; > > > break; > > > +#endif > > > default: > > > return -EINVAL; > > > } > > > > Thanks, looks like this is tripping up the kernelci builds too: > > https://linux.kernelci.org/build/id/6343b37c6fd2dcbb5dcab5f3/logs/ . > > This symbol is actually a bit odd, as we don't actually just use it > > under Zicbom because of the T-Head stuff. Looks like that's generically > > broken, though, so IMO we need something like this > > > > diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c > > index 6cb7d96ad9c7..fba86fa3a628 100644 > > --- a/arch/riscv/mm/cacheflush.c > > +++ b/arch/riscv/mm/cacheflush.c > > @@ -5,6 +5,9 @@ > > > > #include <asm/cacheflush.h> > > > > +unsigned int riscv_cbom_block_size; > > +EXPORT_SYMBOL(riscv_cbom_block_size); > > + > > #ifdef CONFIG_SMP > > > > #include <asm/sbi.h> > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > > index e3f9bdf47c5f..33d21ace0971 100644 > > --- a/arch/riscv/mm/dma-noncoherent.c > > +++ b/arch/riscv/mm/dma-noncoherent.c > > @@ -12,7 +12,6 @@ > > #include <linux/of_device.h> > > #include <asm/cacheflush.h> > > > > -unsigned int riscv_cbom_block_size; > > static bool noncoherent_supported; > > > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > > > > which should be > > > > Fixes: 8f7e001e0325 ("RISC-V: Clean up the Zicbom block size probing"). > > Hi Palmer, > > Should I post your suggestion as a v2 or were you planning to post it? > I'm happy to do so, but I'd prefer to also move > riscv_init_cbom_blocksize() and drop its #ifdef CONFIG_RISCV_ISA_ZICBOM > guard. The reason I'd prefer we always try to parse the block size is > that we always parse the ISA string, and, when zicbom is present, we set > it in the ISA extension bitmap, even when CONFIG_RISCV_ISA_ZICBOM isn't > enabled. This is a reasonable behavior if CONFIG_RISCV_ISA_ZICBOM is meant > to decide whether or not the kernel uses the feature for itself, as its > Kconfig text implies, rather than to mean we should pretend the feature > isn't present, whether it is or not. So, if we're not trying to completely > avoid the feature by disabling CONFIG_RISCV_ISA_ZICBOM, then KVM can still > inform the guest about it. When KVM does that it also needs the block > size. I went ahead and posted a v2 since it may be easier to communicate the idea by patch. Thanks, drew
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c index a032c4f0d600..e4453caba728 100644 --- a/arch/riscv/kvm/vcpu.c +++ b/arch/riscv/kvm/vcpu.c @@ -265,11 +265,13 @@ static int kvm_riscv_vcpu_get_reg_config(struct kvm_vcpu *vcpu, case KVM_REG_RISCV_CONFIG_REG(isa): reg_val = vcpu->arch.isa[0] & KVM_RISCV_BASE_ISA_MASK; break; +#ifdef CONFIG_RISCV_ISA_ZICBOM case KVM_REG_RISCV_CONFIG_REG(zicbom_block_size): if (!riscv_isa_extension_available(vcpu->arch.isa, ZICBOM)) return -EINVAL; reg_val = riscv_cbom_block_size; break; +#endif default: return -EINVAL; }
Fix undefined reference of riscv_cbom_block_size when compiling KVM without RISCV_ISA_ZICBOM. Note, RISCV_ISA_ZICBOM is a sufficient guard as it selects RISCV_DMA_NONCOHERENT, which is needed to compile dma-noncoherent.c (which is the file where riscv_cbom_block_size and its initializer live). Fixes: afd5dde9a186 ("RISC-V: KVM: Provide UAPI for Zicbom block size") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- arch/riscv/kvm/vcpu.c | 2 ++ 1 file changed, 2 insertions(+)