* Re: [PATCH] RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM
2022-10-10 18:36 ` Palmer Dabbelt
@ 2022-10-10 18:47 ` Conor Dooley
2022-10-11 5:05 ` Anup Patel
2022-10-12 17:07 ` Andrew Jones
2 siblings, 0 replies; 6+ messages in thread
From: Conor Dooley @ 2022-10-10 18:47 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: ajones, kvm-riscv, linux-riscv, anup, atishp, conor.dooley,
vernon2gm, lkp
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/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM
2022-10-10 18:36 ` Palmer Dabbelt
2022-10-10 18:47 ` Conor Dooley
@ 2022-10-11 5:05 ` Anup Patel
2022-10-12 17:07 ` Andrew Jones
2 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2022-10-11 5:05 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: ajones, kvm-riscv, linux-riscv, anup, atishp, conor.dooley,
vernon2gm, lkp
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM
2022-10-10 18:36 ` Palmer Dabbelt
2022-10-10 18:47 ` Conor Dooley
2022-10-11 5:05 ` Anup Patel
@ 2022-10-12 17:07 ` Andrew Jones
2022-10-13 13:44 ` Andrew Jones
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Jones @ 2022-10-12 17:07 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: kvm-riscv, linux-riscv, anup, atishp, conor.dooley, vernon2gm,
lkp
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] RISC-V: KVM: Fix compilation without RISCV_ISA_ZICBOM
2022-10-12 17:07 ` Andrew Jones
@ 2022-10-13 13:44 ` Andrew Jones
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Jones @ 2022-10-13 13:44 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: kvm-riscv, linux-riscv, anup, atishp, conor.dooley, vernon2gm,
lkp
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 6+ messages in thread