[PATCH v2 0/3] KVM: arm64: Minor improvements to RAZ register handling

[PATCH v2 0/3] KVM: arm64: Minor improvements to RAZ register handling

[Alexandru Elisei]

What sparked these small patches is the series that fixed the PMU reset
values and their visibility from userspace, more specifically the
discussion around the patch that removed the PMSWINC_EL0 shadow register
[1].

The patches are straightforward cleanups without any changes in
functionality.

Tested on a rockpro64, by running kvm-unit-tests under qemu.

Changes in v2:

* Added patch #3 ("KVM: arm64: Replace get_raz_id_reg() with get_raz_reg()"), as
  suggested by Drew.
* Rebased on top of v5.15-rc5.
* Gathered Reviewed-by, thanks!
* A few small commit message changes to patch #1.

[1] https://www.spinics.net/lists/kvm-arm/msg47976.html

Alexandru Elisei (3):
  KVM: arm64: Return early from read_id_reg() if register is RAZ
  KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0
  KVM: arm64: Replace get_raz_id_reg() with get_raz_reg()

 arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

-- 
2.33.0


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

[PATCH v2 1/3] KVM: arm64: Return early from read_id_reg() if register is RAZ

[Alexandru Elisei]

If read_id_reg() is called for an ID register which is Read-As-Zero (RAZ),
it initializes the return value to zero, then goes through a list of
registers which require special handling before returning the final value.

By not returning as soon as it checks that the register should be RAZ, the
function creates the opportunity for bugs, if, for example, a patch changes
a register to RAZ (like has happened with PMSWINC_EL0 in commit
11663111cd49), but doesn't remove the special handling from read_id_reg();
or if a register is RAZ in certain situations, but readable in others.

Return early to make it impossible for a RAZ register to be anything other
than zero.

Reviewed-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..4adda8bf3168 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1064,7 +1064,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)
 {
 	u32 id = reg_to_encoding(r);
-	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
+	u64 val;
+
+	if (raz)
+		return 0;
+
+	val = read_sanitised_ftr_reg(id);
 
 	switch (id) {
 	case SYS_ID_AA64PFR0_EL1:
-- 
2.33.0


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

[PATCH v2 2/3] KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0

[Alexandru Elisei]

PMSWINC_EL0 is a write-only register and was initially part of the VCPU
register state, but was later removed in commit 7a3ba3095a32 ("KVM:
arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
register was kept accessible from userspace as Read-As-Zero (RAZ).

The read function that is used to handle userspace reads of this
register is get_raz_id_reg(), which, while technically correct, as it
returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
register as the function name suggests.

Add a new function, get_raz_reg(), to use it as the accessor for
PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
of registers.

No functional change intended.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 4adda8bf3168..1be827740f87 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1285,6 +1285,15 @@ static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return __set_id_reg(vcpu, rd, uaddr, true);
 }
 
+static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+		       const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	const u64 id = sys_reg_to_index(rd);
+	const u64 val = 0;
+
+	return reg_to_user(uaddr, &val, id);
+}
+
 static int set_wi_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      const struct kvm_one_reg *reg, void __user *uaddr)
 {
@@ -1647,7 +1656,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	 * previously (and pointlessly) advertised in the past...
 	 */
 	{ PMU_SYS_REG(SYS_PMSWINC_EL0),
-	  .get_user = get_raz_id_reg, .set_user = set_wi_reg,
+	  .get_user = get_raz_reg, .set_user = set_wi_reg,
 	  .access = access_pmswinc, .reset = NULL },
 	{ PMU_SYS_REG(SYS_PMSELR_EL0),
 	  .access = access_pmselr, .reset = reset_pmselr, .reg = PMSELR_EL0 },
-- 
2.33.0


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

Re: [PATCH v2 2/3] KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0

[Andrew Jones]

On Mon, Oct 11, 2021 at 11:58:39AM +0100, Alexandru Elisei wrote:
> PMSWINC_EL0 is a write-only register and was initially part of the VCPU
> register state, but was later removed in commit 7a3ba3095a32 ("KVM:
> arm64: Remove PMSWINC_EL0 shadow register"). To prevent regressions, the
> register was kept accessible from userspace as Read-As-Zero (RAZ).
> 
> The read function that is used to handle userspace reads of this
> register is get_raz_id_reg(), which, while technically correct, as it
> returns 0, it is not semantically correct, as PMSWINC_EL0 is not an ID
> register as the function name suggests.
> 
> Add a new function, get_raz_reg(), to use it as the accessor for
> PMSWINC_EL0, as to not conflate get_raz_id_reg() to handle other types
> of registers.
> 
> No functional change intended.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>


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

[PATCH v2 3/3] KVM: arm64: Replace get_raz_id_reg() with get_raz_reg()

[Alexandru Elisei]

Reading a RAZ ID register isn't different from reading any other RAZ
register, so get rid of get_raz_id_reg() and replace it with get_raz_reg(),
which does the same thing, but does it without going through two layers of
indirection.

No functional change.

Suggested-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/sys_regs.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1be827740f87..3aff06aafd0c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1273,12 +1273,6 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	return __set_id_reg(vcpu, rd, uaddr, raz);
 }
 
-static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
-			  const struct kvm_one_reg *reg, void __user *uaddr)
-{
-	return __get_id_reg(vcpu, rd, uaddr, true);
-}
-
 static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 			  const struct kvm_one_reg *reg, void __user *uaddr)
 {
@@ -1402,7 +1396,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
 #define ID_UNALLOCATED(crm, op2) {			\
 	Op0(3), Op1(0), CRn(0), CRm(crm), Op2(op2),	\
 	.access = access_raz_id_reg,			\
-	.get_user = get_raz_id_reg,			\
+	.get_user = get_raz_reg,			\
 	.set_user = set_raz_id_reg,			\
 }
 
@@ -1414,7 +1408,7 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
 #define ID_HIDDEN(name) {			\
 	SYS_DESC(SYS_##name),			\
 	.access = access_raz_id_reg,		\
-	.get_user = get_raz_id_reg,		\
+	.get_user = get_raz_reg,		\
 	.set_user = set_raz_id_reg,		\
 }
 
-- 
2.33.0


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

Re: [PATCH v2 3/3] KVM: arm64: Replace get_raz_id_reg() with get_raz_reg()

[Andrew Jones]

On Mon, Oct 11, 2021 at 11:58:40AM +0100, Alexandru Elisei wrote:
> Reading a RAZ ID register isn't different from reading any other RAZ
> register, so get rid of get_raz_id_reg() and replace it with get_raz_reg(),
> which does the same thing, but does it without going through two layers of
> indirection.
> 
> No functional change.
> 
> Suggested-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>

Reviewed-by: Andrew Jones <drjones@redhat.com>


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

Re: [PATCH v2 0/3] KVM: arm64: Minor improvements to RAZ register handling

[Marc Zyngier]

On Mon, 11 Oct 2021 11:58:37 +0100, Alexandru Elisei wrote:
> What sparked these small patches is the series that fixed the PMU reset
> values and their visibility from userspace, more specifically the
> discussion around the patch that removed the PMSWINC_EL0 shadow register
> [1].
> 
> The patches are straightforward cleanups without any changes in
> functionality.
> 
> [...]

Applied to next, thanks!

[1/3] KVM: arm64: Return early from read_id_reg() if register is RAZ
      commit: 00d5101b254b77c35a8d55fe46331b19192866f3
[2/3] KVM: arm64: Use get_raz_reg() for userspace reads of PMSWINC_EL0
      commit: 5a4309762356f05df4c92629e5df15ab75c42c0d
[3/3] KVM: arm64: Replace get_raz_id_reg() with get_raz_reg()
      commit: ebf6aa8c047352fe43b1e20e580f12d5564da28e

Cheers,

	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