[PATCH v1 0/2] small kern_hyp_va() cleanups

[PATCH v1 0/2] small kern_hyp_va() cleanups

[Joey Gouly]

Hello,

I wanted to add some comments to __kern_hyp_va(), since I was recently trying
to understand it, and I think it could be helpful for someone looking at it in
the future.

The second patch removes the assembly macro version, since it is unused. Maybe
(out-of-tree) pkvm uses it or it's just worth keeping around in case someone
needs it in the future. So if that patch isn't applied it's not too important.

Thanks,
Joey

Joey Gouly (2):
  KVM: arm64: add comments to __kern_hyp_va
  KVM: arm64: removed unused kern_hyp_va asm macro

 arch/arm64/include/asm/kvm_mmu.h | 41 ++++++++++++--------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/2] KVM: arm64: add comments to __kern_hyp_va

[Joey Gouly]

Document this function a little, to make it easier to understand.

The assembly comments were copied from the kern_hyp_va asm macro.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_mmu.h | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index e3e793d0ec30..d248a7ae627a 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -127,14 +127,24 @@ void kvm_apply_hyp_relocations(void);
 
 #define __hyp_pa(x) (((phys_addr_t)(x)) + hyp_physvirt_offset)
 
+/*
+ * Convert a kernel VA into a HYP VA.
+ *
+ * Can be called from hyp or non-hyp context.
+ */
 static __always_inline unsigned long __kern_hyp_va(unsigned long v)
 {
+/*
+ * This #ifndef is an optimisation for when this is called from VHE hyp
+ * context.  When called from a VHE non-hyp context, kvm_update_va_mask() will
+ * replace the instructions with `nop`s.
+ */
 #ifndef __KVM_VHE_HYPERVISOR__
-	asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n"
-				    "ror %0, %0, #1\n"
-				    "add %0, %0, #0\n"
-				    "add %0, %0, #0, lsl 12\n"
-				    "ror %0, %0, #63\n",
+	asm volatile(ALTERNATIVE_CB("and %0, %0, #1\n"         /* mask with va_mask */
+				    "ror %0, %0, #1\n"         /* rotate to the first tag bit */
+				    "add %0, %0, #0\n"         /* insert the low 12 bits of the tag */
+				    "add %0, %0, #0, lsl 12\n" /* insert the top 12 bits of the tag */
+				    "ror %0, %0, #63\n",       /* rotate back */
 				    ARM64_ALWAYS_SYSTEM,
 				    kvm_update_va_mask)
 		     : "+r" (v));
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/2] KVM: arm64: removed unused kern_hyp_va asm macro

[Joey Gouly]

The last usage of this macro was removed in:
    commit 5dc33bd199ca ("KVM: arm64: nVHE: Pass pointers consistently to hyp-init")

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_mmu.h | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index d248a7ae627a..961d4431654b 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -53,27 +53,6 @@
 
 #include <asm/alternative.h>
 
-/*
- * Convert a kernel VA into a HYP VA.
- * reg: VA to be converted.
- *
- * The actual code generation takes place in kvm_update_va_mask, and
- * the instructions below are only there to reserve the space and
- * perform the register allocation (kvm_update_va_mask uses the
- * specific registers encoded in the instructions).
- */
-.macro kern_hyp_va	reg
-#ifndef __KVM_VHE_HYPERVISOR__
-alternative_cb ARM64_ALWAYS_SYSTEM, kvm_update_va_mask
-	and     \reg, \reg, #1		/* mask with va_mask */
-	ror	\reg, \reg, #1		/* rotate to the first tag bit */
-	add	\reg, \reg, #0		/* insert the low 12 bits of the tag */
-	add	\reg, \reg, #0, lsl 12	/* insert the top 12 bits of the tag */
-	ror	\reg, \reg, #63		/* rotate back */
-alternative_cb_end
-#endif
-.endm
-
 /*
  * Convert a hypervisor VA to a PA
  * reg: hypervisor address to be converted in place
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/2] small kern_hyp_va() cleanups

[Oliver Upton]

Hi Joey,

Thanks for having a look.

On Thu, Feb 08, 2024 at 10:54:20AM +0000, Joey Gouly wrote:
> I wanted to add some comments to __kern_hyp_va(), since I was recently trying
> to understand it, and I think it could be helpful for someone looking at it in
> the future.

That part is definitely a bit screwy :)

> The second patch removes the assembly macro version, since it is unused. Maybe
> (out-of-tree) pkvm uses it or it's just worth keeping around in case someone
> needs it in the future. So if that patch isn't applied it's not too important.

Keeping unused code hanging around is rarely useful. While it is nice to
give consideration to the pKVM folks (looks like they don't need it), it
is easy enough to re-introduce later on.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/2] small kern_hyp_va() cleanups

[Marc Zyngier]

On Thu, 08 Feb 2024 10:54:20 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> Hello,
> 
> I wanted to add some comments to __kern_hyp_va(), since I was recently trying
> to understand it, and I think it could be helpful for someone looking at it in
> the future.
> 
> The second patch removes the assembly macro version, since it is unused. Maybe
> (out-of-tree) pkvm uses it or it's just worth keeping around in case someone
> needs it in the future. So if that patch isn't applied it's not too important.

I checked with Fuad, and he confirmed that there is no assembly code
using kern_hyp_va() left in pKVM either, so this guys is definitely a
goner.

For the series,

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/2] KVM: arm64: removed unused kern_hyp_va asm macro

[Marc Zyngier]

On Thu, 08 Feb 2024 10:54:22 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> The last usage of this macro was removed in:
>     commit 5dc33bd199ca ("KVM: arm64: nVHE: Pass pointers consistently to hyp-init")
> 
> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_mmu.h | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index d248a7ae627a..961d4431654b 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -53,27 +53,6 @@
>  
>  #include <asm/alternative.h>
>  
> -/*
> - * Convert a kernel VA into a HYP VA.
> - * reg: VA to be converted.
> - *
> - * The actual code generation takes place in kvm_update_va_mask, and
> - * the instructions below are only there to reserve the space and
> - * perform the register allocation (kvm_update_va_mask uses the
> - * specific registers encoded in the instructions).

Consider moving this comment to the C version instead of deleting it
completely (the part about the register allocation is pretty crucial).

Oliver can probably fix this when applying it, and my previous Ack
still stands.

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/2] KVM: arm64: removed unused kern_hyp_va asm macro

[Oliver Upton]

On Thu, Feb 08, 2024 at 12:42:25PM +0000, Marc Zyngier wrote:
> On Thu, 08 Feb 2024 10:54:22 +0000,
> Joey Gouly <joey.gouly@arm.com> wrote:
> > 
> > The last usage of this macro was removed in:
> >     commit 5dc33bd199ca ("KVM: arm64: nVHE: Pass pointers consistently to hyp-init")
> > 
> > Signed-off-by: Joey Gouly <joey.gouly@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Oliver Upton <oliver.upton@linux.dev>
> > ---
> >  arch/arm64/include/asm/kvm_mmu.h | 21 ---------------------
> >  1 file changed, 21 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > index d248a7ae627a..961d4431654b 100644
> > --- a/arch/arm64/include/asm/kvm_mmu.h
> > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > @@ -53,27 +53,6 @@
> >  
> >  #include <asm/alternative.h>
> >  
> > -/*
> > - * Convert a kernel VA into a HYP VA.
> > - * reg: VA to be converted.
> > - *
> > - * The actual code generation takes place in kvm_update_va_mask, and
> > - * the instructions below are only there to reserve the space and
> > - * perform the register allocation (kvm_update_va_mask uses the
> > - * specific registers encoded in the instructions).
> 
> Consider moving this comment to the C version instead of deleting it
> completely (the part about the register allocation is pretty crucial).
> 
> Oliver can probably fix this when applying it, and my previous Ack
> still stands.

Yeah -- this comment is still rather useful. I'll fix it up when I apply
the series.

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/2] small kern_hyp_va() cleanups

[Oliver Upton]

On Thu, 8 Feb 2024 10:54:20 +0000, Joey Gouly wrote:
> I wanted to add some comments to __kern_hyp_va(), since I was recently trying
> to understand it, and I think it could be helpful for someone looking at it in
> the future.
> 
> The second patch removes the assembly macro version, since it is unused. Maybe
> (out-of-tree) pkvm uses it or it's just worth keeping around in case someone
> needs it in the future. So if that patch isn't applied it's not too important.
> 
> [...]

Applied to kvmarm/next, thanks!

[1/2] KVM: arm64: add comments to __kern_hyp_va
      https://git.kernel.org/kvmarm/kvmarm/c/d198e2668e24
[2/2] KVM: arm64: removed unused kern_hyp_va asm macro
      https://git.kernel.org/kvmarm/kvmarm/c/a02395d0f3bf

--
Best,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel