Hi Drew,

On Wed, Oct 06, 2021 at 05:23:02PM +0200, Andrew Jones wrote:

On Wed, Oct 06, 2021 at 03:49:19PM +0100, Alexandru Elisei wrote:

quoted

Hi Drew,

Thank you for the review!

On Thu, Sep 30, 2021 at 03:29:15PM +0200, Andrew Jones wrote:

quoted

On Mon, Sep 27, 2021 at 01:49:11PM +0100, Alexandru Elisei wrote:

quoted

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 <redacted>
---
 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

What about replacing get_raz_id_reg() with this new function? Do really need
both?

I thought about that when writing this patch. I ultimately decided against it
because changing the get_user accessor to be get_raz_reg() instead of
get_raz_id_reg() would break the symmetry with set_user, which needs to stay
set_raz_id_reg(), and cannot be substituted with set_wi_reg() because that would
be a change in behaviour (set_raz_id_reg() checks that val == 0, set_wi_reg()
doesn't).

I do agree that get_raz_id_reg() does the exact same thing as get_raz_reg(), but
in a more roundabout manner. So if you still feel that I should use
get_raz_reg() instead, I'll do that for the next iteration of the series. What
do you think?

I'd prefer we avoid maintaining two implementations of the same
functionality. If we want to keep the symmetry with set_raz_id_reg,
then we could implement get_raz_id_reg as 'return get_raz_reg()'.

Agreed, I'll replace get_raz_id_reg() with get_raz_reg().

Thanks,
Alex

Thanks,
drew

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