diff mbox series

RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM

Message ID 20221010094029.1579672-1-ajones@ventanamicro.com (mailing list archive)
State Superseded
Headers show
Series RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM | expand

Commit Message

Andrew Jones Oct. 10, 2022, 9:40 a.m. UTC
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(+)

Comments

Palmer Dabbelt Oct. 10, 2022, 6:36 p.m. UTC | #1
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.
Conor Dooley Oct. 10, 2022, 6:47 p.m. UTC | #2
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/
Anup Patel Oct. 11, 2022, 5:05 a.m. UTC | #3
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
Andrew Jones Oct. 12, 2022, 5:07 p.m. UTC | #4
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
Andrew Jones Oct. 13, 2022, 1:44 p.m. UTC | #5
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 mbox series

Patch

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;
 	}