[PATCH v7 00/10] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2,3}

[PATCH v7 00/10] Enable writable for idregs DFR0,PFR0, MMFR{0,1,2,3}

[Jing Zhang]

This patch series enable userspace writable for below idregs:
ID_AA64DFR0_EL1, ID_DFR0_EL1, ID_AA64PFR0_EL1, ID_AA64MMFR{0, 1, 2, 3}_EL1.
A vm ioctl is added to get feature ID register writable masks from userspace.
A selftest is added to verify that KVM handles the writings from user space
correctly.
A relevant patch from Oliver is picked from [3].
It is based on v6.5-rc4 which contains infrastructure for writable idregs.

---

* v6 -> v7
  - Rebase on v6.5-rc4.
  - Add a vm ioctl to get feature ID register writable masks from userspace.
  - Split the change for debug version in ID_{AA64}DFR0_EL1.
  - Addressed some bugs in selftest.

* v5 -> v6
  - Override the type of field AA64DFR0_EL1_DebugVer to be FTR_LOWER_SAFE by the
    discussion of Oliver and Suraj.

* v4 -> v5
  - Rebase on v6.5-rc1 which contains infrastructure for writable idregs.
  - Use guest ID registers values for the sake of emulation.
  - Added a selftest to verify idreg userspace writing.

* v3 -> v4
  - Rebase on v11 of writable idregs series at [2].

* v2 -> v3
  - Rebase on v6 of writable idregs series.
  - Enable writable for ID_AA64PFR0_EL1 and ID_AA64MMFR{0, 1, 2}_EL1.

* v1 -> v2
  - Rebase on latest patch series [1] of enabling writable ID register.

[1] https://lore.kernel.org/all/20230402183735.3011540-1-jingzhangos@google.com
[2] https://lore.kernel.org/all/20230602005118.2899664-1-jingzhangos@google.com
[3] https://lore.kernel.org/kvmarm/20230623205232.2837077-1-oliver.upton@linux.dev

[v1] https://lore.kernel.org/all/20230326011950.405749-1-jingzhangos@google.com
[v2] https://lore.kernel.org/all/20230403003723.3199828-1-jingzhangos@google.com
[v3] https://lore.kernel.org/all/20230405172146.297208-1-jingzhangos@google.com
[v4] https://lore.kernel.org/all/20230607194554.87359-1-jingzhangos@google.com
[v5] https://lore.kernel.org/all/20230710192430.1992246-1-jingzhangos@google.com
[v6] https://lore.kernel.org/all/20230718164522.3498236-1-jingzhangos@google.com

---

Jing Zhang (8):
  KVM: arm64: Allow userspace to get the writable masks for feature ID
    registers
  KVM: arm64: Document KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS
  KVM: arm64: Use guest ID register values for the sake of emulation
  KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1
  KVM: arm64: Enable writable for ID_AA64PFR0_EL1
  KVM: arm64: Refactor helper Macros for idreg desc
  KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1
  KVM: arm64: selftests: Test for setting ID register from usersapce

Oliver Upton (2):
  KVM: arm64: Reject attempts to set invalid debug arch version
  KVM: arm64: Bump up the default KVM sanitised debug version to v8p8

 Documentation/virt/kvm/api.rst                |  26 +++
 arch/arm64/include/asm/kvm_host.h             |   2 +
 arch/arm64/include/uapi/asm/kvm.h             |  25 +++
 arch/arm64/kvm/arm.c                          |   3 +
 arch/arm64/kvm/sys_regs.c                     | 191 ++++++++++++------
 include/uapi/linux/kvm.h                      |   2 +
 tools/arch/arm64/include/uapi/asm/kvm.h       |  25 +++
 tools/include/uapi/linux/kvm.h                |   2 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/set_id_regs.c       | 191 ++++++++++++++++++
 10 files changed, 408 insertions(+), 60 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c


base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Jing Zhang]

Add a VM ioctl to allow userspace to get writable masks for feature ID
registers in below system register space:
op0 = 3, op1 = {0, 1, 3}, CRn = 0, CRm = {0 - 7}, op2 = {0 - 7}
This is used to support mix-and-match userspace and kernels for writable
ID registers, where userspace may want to know upfront whether it can
actually tweak the contents of an idreg or not.

Suggested-by: Marc Zyngier <maz@kernel.org>
Suggested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  2 ++
 arch/arm64/include/uapi/asm/kvm.h | 25 +++++++++++++++
 arch/arm64/kvm/arm.c              |  3 ++
 arch/arm64/kvm/sys_regs.c         | 51 +++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          |  2 ++
 5 files changed, 83 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d3dd05bbfe23..3996a3707f4e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -1074,6 +1074,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
 			       struct kvm_arm_copy_mte_tags *copy_tags);
 int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
 				    struct kvm_arm_counter_offset *offset);
+int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm,
+					       u64 __user *masks);
 
 /* Guest/host FPSIMD coordination helpers */
 int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index f7ddd73a8c0f..2970c0d792ee 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -505,6 +505,31 @@ struct kvm_smccc_filter {
 #define KVM_HYPERCALL_EXIT_SMC		(1U << 0)
 #define KVM_HYPERCALL_EXIT_16BIT	(1U << 1)
 
+/* Get feature ID registers userspace writable mask. */
+/*
+ * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
+ * Feature Register 2"):
+ *
+ * "The Feature ID space is defined as the System register space in
+ * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
+ * op2=={0-7}."
+ *
+ * This covers all R/O registers that indicate anything useful feature
+ * wise, including the ID registers.
+ */
+#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)		\
+	({								\
+		__u64 __op1 = (op1) & 3;				\
+		__op1 -= (__op1 == 3);					\
+		(__op1 << 6 | ((crm) & 7) << 3 | (op2));		\
+	})
+
+#define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
+
+struct feature_id_writable_masks {
+	__u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
+};
+
 #endif
 
 #endif /* __ARM_KVM_H__ */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 72dc53a75d1c..c9cd14057c58 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1630,6 +1630,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 
 		return kvm_vm_set_attr(kvm, &attr);
 	}
+	case KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS: {
+		return kvm_vm_ioctl_get_feature_id_writable_masks(kvm, argp);
+	}
 	default:
 		return -EINVAL;
 	}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2ca2973abe66..d9317b640ba5 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -3560,6 +3560,57 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 	return write_demux_regids(uindices);
 }
 
+#define ARM64_FEATURE_ID_SPACE_INDEX(r)			\
+	ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),	\
+		sys_reg_Op1(r),				\
+		sys_reg_CRn(r),				\
+		sys_reg_CRm(r),				\
+		sys_reg_Op2(r))
+
+static bool is_feature_id_reg(u32 encoding)
+{
+	return (sys_reg_Op0(encoding) == 3 &&
+		(sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
+		sys_reg_CRn(encoding) == 0 &&
+		sys_reg_CRm(encoding) <= 7);
+}
+
+int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm, u64 __user *masks)
+{
+	/* Wipe the whole thing first */
+	for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
+		if (put_user(0, masks + i))
+			return -EFAULT;
+
+	for (int i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
+		const struct sys_reg_desc *reg = &sys_reg_descs[i];
+		u32 encoding = reg_to_encoding(reg);
+		u64 val;
+
+		if (!is_feature_id_reg(encoding) || !reg->set_user)
+			continue;
+
+		/*
+		 * For ID registers, we return the writable mask. Other feature
+		 * registers return a full 64bit mask. That's not necessary
+		 * compliant with a given revision of the architecture, but the
+		 * RES0/RES1 definitions allow us to do that.
+		 */
+		if (is_id_reg(encoding)) {
+			if (!reg->val)
+				continue;
+			val = reg->val;
+		} else {
+			val = ~0UL;
+		}
+
+		if (put_user(val, (masks + ARM64_FEATURE_ID_SPACE_INDEX(encoding))))
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
 int __init kvm_sys_reg_table_init(void)
 {
 	struct sys_reg_params params;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f089ab290978..86ffdf134eb8 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1555,6 +1555,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_ARM_MTE_COPY_TAGS	  _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
 /* Available with KVM_CAP_COUNTER_OFFSET */
 #define KVM_ARM_SET_COUNTER_OFFSET _IOW(KVMIO,  0xb5, struct kvm_arm_counter_offset)
+#define KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS \
+			_IOR(KVMIO,  0xb6, struct feature_id_writable_masks)
 
 /* ioctl for vm fd */
 #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
-- 
2.41.0.585.gd2178a4bd4-goog


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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Oliver Upton]

Hi Jing,

On Tue, Aug 01, 2023 at 08:19:57AM -0700, Jing Zhang wrote:
> Add a VM ioctl to allow userspace to get writable masks for feature ID
> registers in below system register space:
> op0 = 3, op1 = {0, 1, 3}, CRn = 0, CRm = {0 - 7}, op2 = {0 - 7}
> This is used to support mix-and-match userspace and kernels for writable
> ID registers, where userspace may want to know upfront whether it can
> actually tweak the contents of an idreg or not.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 ++
>  arch/arm64/include/uapi/asm/kvm.h | 25 +++++++++++++++
>  arch/arm64/kvm/arm.c              |  3 ++
>  arch/arm64/kvm/sys_regs.c         | 51 +++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          |  2 ++
>  5 files changed, 83 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d3dd05bbfe23..3996a3707f4e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1074,6 +1074,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>  			       struct kvm_arm_copy_mte_tags *copy_tags);
>  int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
>  				    struct kvm_arm_counter_offset *offset);
> +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm,
> +					       u64 __user *masks);
>  
>  /* Guest/host FPSIMD coordination helpers */
>  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index f7ddd73a8c0f..2970c0d792ee 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -505,6 +505,31 @@ struct kvm_smccc_filter {
>  #define KVM_HYPERCALL_EXIT_SMC		(1U << 0)
>  #define KVM_HYPERCALL_EXIT_16BIT	(1U << 1)
>  
> +/* Get feature ID registers userspace writable mask. */
> +/*
> + * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
> + * Feature Register 2"):
> + *
> + * "The Feature ID space is defined as the System register space in
> + * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
> + * op2=={0-7}."
> + *
> + * This covers all R/O registers that indicate anything useful feature
> + * wise, including the ID registers.
> + */
> +#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)		\
> +	({								\
> +		__u64 __op1 = (op1) & 3;				\
> +		__op1 -= (__op1 == 3);					\
> +		(__op1 << 6 | ((crm) & 7) << 3 | (op2));		\
> +	})
> +
> +#define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
> +
> +struct feature_id_writable_masks {
> +	__u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> +};

This UAPI is rather difficult to extend in the future. We may need to
support describing the masks of multiple ranges of registers in the
future. I was thinking something along the lines of:

	enum reg_mask_range_idx {
		FEATURE_ID,
	};

	struct reg_mask_range {
		__u64 idx;
		__u64 *masks;
		__u64 rsvd[6];
	};

>  #endif
>  
>  #endif /* __ARM_KVM_H__ */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 72dc53a75d1c..c9cd14057c58 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1630,6 +1630,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  
>  		return kvm_vm_set_attr(kvm, &attr);
>  	}
> +	case KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS: {
> +		return kvm_vm_ioctl_get_feature_id_writable_masks(kvm, argp);
> +	}
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2ca2973abe66..d9317b640ba5 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -3560,6 +3560,57 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  	return write_demux_regids(uindices);
>  }
>  
> +#define ARM64_FEATURE_ID_SPACE_INDEX(r)			\
> +	ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),	\
> +		sys_reg_Op1(r),				\
> +		sys_reg_CRn(r),				\
> +		sys_reg_CRm(r),				\
> +		sys_reg_Op2(r))
> +
> +static bool is_feature_id_reg(u32 encoding)
> +{
> +	return (sys_reg_Op0(encoding) == 3 &&
> +		(sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
> +		sys_reg_CRn(encoding) == 0 &&
> +		sys_reg_CRm(encoding) <= 7);
> +}
> +
> +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm, u64 __user *masks)
> +{

Use the correct type for the user pointer (it's a struct pointer).

> +	/* Wipe the whole thing first */
> +	for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
> +		if (put_user(0, masks + i))
> +			return -EFAULT;

Why not:

	if (clear_user(masks, ARM64_FEATURE_ID_SPACE_SIZE * sizeof(__u64)))
		return -EFAULT;

Of course, this may need to be adapted if the UAPI struct changes.

-- 
Thanks,
Oliver

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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Jing Zhang]

Hi Oliver,

On Tue, Aug 1, 2023 at 5:04 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Jing,
>
> On Tue, Aug 01, 2023 at 08:19:57AM -0700, Jing Zhang wrote:
> > Add a VM ioctl to allow userspace to get writable masks for feature ID
> > registers in below system register space:
> > op0 = 3, op1 = {0, 1, 3}, CRn = 0, CRm = {0 - 7}, op2 = {0 - 7}
> > This is used to support mix-and-match userspace and kernels for writable
> > ID registers, where userspace may want to know upfront whether it can
> > actually tweak the contents of an idreg or not.
> >
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  2 ++
> >  arch/arm64/include/uapi/asm/kvm.h | 25 +++++++++++++++
> >  arch/arm64/kvm/arm.c              |  3 ++
> >  arch/arm64/kvm/sys_regs.c         | 51 +++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h          |  2 ++
> >  5 files changed, 83 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index d3dd05bbfe23..3996a3707f4e 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1074,6 +1074,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> >                              struct kvm_arm_copy_mte_tags *copy_tags);
> >  int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
> >                                   struct kvm_arm_counter_offset *offset);
> > +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm,
> > +                                            u64 __user *masks);
> >
> >  /* Guest/host FPSIMD coordination helpers */
> >  int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index f7ddd73a8c0f..2970c0d792ee 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -505,6 +505,31 @@ struct kvm_smccc_filter {
> >  #define KVM_HYPERCALL_EXIT_SMC               (1U << 0)
> >  #define KVM_HYPERCALL_EXIT_16BIT     (1U << 1)
> >
> > +/* Get feature ID registers userspace writable mask. */
> > +/*
> > + * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
> > + * Feature Register 2"):
> > + *
> > + * "The Feature ID space is defined as the System register space in
> > + * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
> > + * op2=={0-7}."
> > + *
> > + * This covers all R/O registers that indicate anything useful feature
> > + * wise, including the ID registers.
> > + */
> > +#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)          \
> > +     ({                                                              \
> > +             __u64 __op1 = (op1) & 3;                                \
> > +             __op1 -= (__op1 == 3);                                  \
> > +             (__op1 << 6 | ((crm) & 7) << 3 | (op2));                \
> > +     })
> > +
> > +#define ARM64_FEATURE_ID_SPACE_SIZE  (3 * 8 * 8)
> > +
> > +struct feature_id_writable_masks {
> > +     __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> > +};
>
> This UAPI is rather difficult to extend in the future. We may need to
> support describing the masks of multiple ranges of registers in the
> future. I was thinking something along the lines of:
>
>         enum reg_mask_range_idx {
>                 FEATURE_ID,
>         };
>
>         struct reg_mask_range {
>                 __u64 idx;
>                 __u64 *masks;
>                 __u64 rsvd[6];
>         };
>
Since have the way to map sysregs encoding to the index in the mask
array, we can extend the UAPI by just adding a size field in struct
feature_id_writable_masks like below:
struct feature_id_writable_masks {
         __u64 size;
         __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
};
The 'size' field can be used as input for the size of 'mask' array and
output for the number of masks actually read in.
This way, we can freely add more ranges without breaking anything in userspace.
WDYT?
> >  #endif
> >
> >  #endif /* __ARM_KVM_H__ */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 72dc53a75d1c..c9cd14057c58 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1630,6 +1630,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >
> >               return kvm_vm_set_attr(kvm, &attr);
> >       }
> > +     case KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS: {
> > +             return kvm_vm_ioctl_get_feature_id_writable_masks(kvm, argp);
> > +     }
> >       default:
> >               return -EINVAL;
> >       }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 2ca2973abe66..d9317b640ba5 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -3560,6 +3560,57 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >       return write_demux_regids(uindices);
> >  }
> >
> > +#define ARM64_FEATURE_ID_SPACE_INDEX(r)                      \
> > +     ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),      \
> > +             sys_reg_Op1(r),                         \
> > +             sys_reg_CRn(r),                         \
> > +             sys_reg_CRm(r),                         \
> > +             sys_reg_Op2(r))
> > +
> > +static bool is_feature_id_reg(u32 encoding)
> > +{
> > +     return (sys_reg_Op0(encoding) == 3 &&
> > +             (sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
> > +             sys_reg_CRn(encoding) == 0 &&
> > +             sys_reg_CRm(encoding) <= 7);
> > +}
> > +
> > +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm, u64 __user *masks)
> > +{
>
> Use the correct type for the user pointer (it's a struct pointer).
Will fix.
>
> > +     /* Wipe the whole thing first */
> > +     for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
> > +             if (put_user(0, masks + i))
> > +                     return -EFAULT;
>
> Why not:
>
>         if (clear_user(masks, ARM64_FEATURE_ID_SPACE_SIZE * sizeof(__u64)))
>                 return -EFAULT;
Sure, will do.
>
> Of course, this may need to be adapted if the UAPI struct changes.
>
> --
> Thanks,
> Oliver
Thanks,
Jing

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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Oliver Upton]

On Wed, Aug 02, 2023 at 08:55:43AM -0700, Jing Zhang wrote:
> > > +#define ARM64_FEATURE_ID_SPACE_SIZE  (3 * 8 * 8)
> > > +
> > > +struct feature_id_writable_masks {
> > > +     __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> > > +};
> >
> > This UAPI is rather difficult to extend in the future. We may need to
> > support describing the masks of multiple ranges of registers in the
> > future. I was thinking something along the lines of:
> >
> >         enum reg_mask_range_idx {
> >                 FEATURE_ID,
> >         };
> >
> >         struct reg_mask_range {
> >                 __u64 idx;
> >                 __u64 *masks;
> >                 __u64 rsvd[6];
> >         };
> >
> Since have the way to map sysregs encoding to the index in the mask
> array, we can extend the UAPI by just adding a size field in struct
> feature_id_writable_masks like below:
> struct feature_id_writable_masks {
>          __u64 size;
>          __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> };
> The 'size' field can be used as input for the size of 'mask' array and
> output for the number of masks actually read in.
> This way, we can freely add more ranges without breaking anything in userspace.
> WDYT?

Sorry, 'index' is a bit overloaded in this context. The point I was
trying to get across is that we might want to describe a completely
different range of registers than the feature ID registers in the
future. Nonetheless, we shouldn't even presume the shape of future
extensions to the ioctl.

	struct reg_mask_range {
		__u64 addr;	/* pointer to mask array */
		__u64 rsvd[7];
	};

Then in KVM we should require ::rsvd be zero and fail the ioctl
otherwise.

-- 
Thanks,
Oliver

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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Jing Zhang]

On Wed, Aug 2, 2023 at 10:04 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Aug 02, 2023 at 08:55:43AM -0700, Jing Zhang wrote:
> > > > +#define ARM64_FEATURE_ID_SPACE_SIZE  (3 * 8 * 8)
> > > > +
> > > > +struct feature_id_writable_masks {
> > > > +     __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> > > > +};
> > >
> > > This UAPI is rather difficult to extend in the future. We may need to
> > > support describing the masks of multiple ranges of registers in the
> > > future. I was thinking something along the lines of:
> > >
> > >         enum reg_mask_range_idx {
> > >                 FEATURE_ID,
> > >         };
> > >
> > >         struct reg_mask_range {
> > >                 __u64 idx;
> > >                 __u64 *masks;
> > >                 __u64 rsvd[6];
> > >         };
> > >
> > Since have the way to map sysregs encoding to the index in the mask
> > array, we can extend the UAPI by just adding a size field in struct
> > feature_id_writable_masks like below:
> > struct feature_id_writable_masks {
> >          __u64 size;
> >          __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> > };
> > The 'size' field can be used as input for the size of 'mask' array and
> > output for the number of masks actually read in.
> > This way, we can freely add more ranges without breaking anything in userspace.
> > WDYT?
>
> Sorry, 'index' is a bit overloaded in this context. The point I was
> trying to get across is that we might want to describe a completely
> different range of registers than the feature ID registers in the
> future. Nonetheless, we shouldn't even presume the shape of future
> extensions to the ioctl.
>
>         struct reg_mask_range {
>                 __u64 addr;     /* pointer to mask array */
>                 __u64 rsvd[7];
>         };
>
> Then in KVM we should require ::rsvd be zero and fail the ioctl
> otherwise.
Got it. Will add the ::rsvd for future expansion.

Thanks,
Jing
>
> --
> Thanks,
> Oliver

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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Cornelia Huck]

On Wed, Aug 02 2023, Oliver Upton <oliver.upton@linux.dev> wrote:

> On Wed, Aug 02, 2023 at 08:55:43AM -0700, Jing Zhang wrote:
>> > > +#define ARM64_FEATURE_ID_SPACE_SIZE  (3 * 8 * 8)
>> > > +
>> > > +struct feature_id_writable_masks {
>> > > +     __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
>> > > +};
>> >
>> > This UAPI is rather difficult to extend in the future. We may need to
>> > support describing the masks of multiple ranges of registers in the
>> > future. I was thinking something along the lines of:
>> >
>> >         enum reg_mask_range_idx {
>> >                 FEATURE_ID,
>> >         };
>> >
>> >         struct reg_mask_range {
>> >                 __u64 idx;
>> >                 __u64 *masks;
>> >                 __u64 rsvd[6];
>> >         };
>> >
>> Since have the way to map sysregs encoding to the index in the mask
>> array, we can extend the UAPI by just adding a size field in struct
>> feature_id_writable_masks like below:
>> struct feature_id_writable_masks {
>>          __u64 size;
>>          __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
>> };
>> The 'size' field can be used as input for the size of 'mask' array and
>> output for the number of masks actually read in.
>> This way, we can freely add more ranges without breaking anything in userspace.
>> WDYT?
>
> Sorry, 'index' is a bit overloaded in this context. The point I was
> trying to get across is that we might want to describe a completely
> different range of registers than the feature ID registers in the
> future. Nonetheless, we shouldn't even presume the shape of future
> extensions to the ioctl.
>
> 	struct reg_mask_range {
> 		__u64 addr;	/* pointer to mask array */
> 		__u64 rsvd[7];
> 	};
>
> Then in KVM we should require ::rsvd be zero and fail the ioctl
> otherwise.

[I assume rsvd == reserved? I think I have tried to divine further
meaning into this for far too long...]

Is the idea here for userspace the request a mask array for FEATURE_ID
and future ranges separately instead of getting all id-type regs in one
go?


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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Oliver Upton]

On Thu, Aug 03, 2023 at 03:20:41PM +0200, Cornelia Huck wrote:
> On Wed, Aug 02 2023, Oliver Upton <oliver.upton@linux.dev> wrote:
> > Sorry, 'index' is a bit overloaded in this context. The point I was
> > trying to get across is that we might want to describe a completely
> > different range of registers than the feature ID registers in the
> > future. Nonetheless, we shouldn't even presume the shape of future
> > extensions to the ioctl.
> >
> > 	struct reg_mask_range {
> > 		__u64 addr;	/* pointer to mask array */
> > 		__u64 rsvd[7];
> > 	};
> >
> > Then in KVM we should require ::rsvd be zero and fail the ioctl
> > otherwise.
> 
> [I assume rsvd == reserved? I think I have tried to divine further
> meaning into this for far too long...]

Indeed.

> Is the idea here for userspace the request a mask array for FEATURE_ID
> and future ranges separately instead of getting all id-type regs in one
> go?

I rambled a bit, but the overall suggestion is that we leave room in the
UAPI for future extension. Asserting that the reserved portions of the
structure must be zero is the easiest way to accomplish that. The
complete feature ID register space is known, but maybe there are other
ranges of registers (possibly unrelated to ID) that we'd like to
similarly describe with masks.

-- 
Thanks,
Oliver

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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Shaoqin Huang]

Hi Jing,

On 8/1/23 23:19, Jing Zhang wrote:
> Add a VM ioctl to allow userspace to get writable masks for feature ID
> registers in below system register space:
> op0 = 3, op1 = {0, 1, 3}, CRn = 0, CRm = {0 - 7}, op2 = {0 - 7}
> This is used to support mix-and-match userspace and kernels for writable
> ID registers, where userspace may want to know upfront whether it can
> actually tweak the contents of an idreg or not.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>   arch/arm64/include/asm/kvm_host.h |  2 ++
>   arch/arm64/include/uapi/asm/kvm.h | 25 +++++++++++++++
>   arch/arm64/kvm/arm.c              |  3 ++
>   arch/arm64/kvm/sys_regs.c         | 51 +++++++++++++++++++++++++++++++
>   include/uapi/linux/kvm.h          |  2 ++
>   5 files changed, 83 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d3dd05bbfe23..3996a3707f4e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1074,6 +1074,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
>   			       struct kvm_arm_copy_mte_tags *copy_tags);
>   int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
>   				    struct kvm_arm_counter_offset *offset);
> +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm,
> +					       u64 __user *masks);
>   
>   /* Guest/host FPSIMD coordination helpers */
>   int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index f7ddd73a8c0f..2970c0d792ee 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -505,6 +505,31 @@ struct kvm_smccc_filter {
>   #define KVM_HYPERCALL_EXIT_SMC		(1U << 0)
>   #define KVM_HYPERCALL_EXIT_16BIT	(1U << 1)
>   
> +/* Get feature ID registers userspace writable mask. */
> +/*
> + * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
> + * Feature Register 2"):
> + *
> + * "The Feature ID space is defined as the System register space in
> + * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
> + * op2=={0-7}."
> + *
> + * This covers all R/O registers that indicate anything useful feature
> + * wise, including the ID registers.
> + */
> +#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)		\
> +	({								\
> +		__u64 __op1 = (op1) & 3;				\
> +		__op1 -= (__op1 == 3);					\
> +		(__op1 << 6 | ((crm) & 7) << 3 | (op2));		\
> +	})
> +
> +#define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
> +
> +struct feature_id_writable_masks {
> +	__u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> +};
> +
>   #endif
>   
>   #endif /* __ARM_KVM_H__ */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 72dc53a75d1c..c9cd14057c58 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1630,6 +1630,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>   
>   		return kvm_vm_set_attr(kvm, &attr);
>   	}
> +	case KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS: {
> +		return kvm_vm_ioctl_get_feature_id_writable_masks(kvm, argp);
> +	}
>   	default:
>   		return -EINVAL;
>   	}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2ca2973abe66..d9317b640ba5 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -3560,6 +3560,57 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>   	return write_demux_regids(uindices);
>   }
>   
> +#define ARM64_FEATURE_ID_SPACE_INDEX(r)			\
> +	ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),	\
> +		sys_reg_Op1(r),				\
> +		sys_reg_CRn(r),				\
> +		sys_reg_CRm(r),				\
> +		sys_reg_Op2(r))
> +
> +static bool is_feature_id_reg(u32 encoding)
> +{
> +	return (sys_reg_Op0(encoding) == 3 &&
> +		(sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
> +		sys_reg_CRn(encoding) == 0 &&
> +		sys_reg_CRm(encoding) <= 7);
> +}

A fool question.

In the code base, there is a function is_id_reg() to check if it's a 
id_reg, what's the difference between the is_feature_id_reg() and the 
is_id_reg()?

/*
  * Return true if the register's (Op0, Op1, CRn, CRm, Op2) is
  * (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
  */
static inline bool is_id_reg(u32 id)
{
	return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
		sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
		sys_reg_CRm(id) < 8);
}

Thanks,
Shaoqin

> +
> +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm, u64 __user *masks)
> +{
> +	/* Wipe the whole thing first */
> +	for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
> +		if (put_user(0, masks + i))
> +			return -EFAULT;
> +
> +	for (int i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> +		const struct sys_reg_desc *reg = &sys_reg_descs[i];
> +		u32 encoding = reg_to_encoding(reg);
> +		u64 val;
> +
> +		if (!is_feature_id_reg(encoding) || !reg->set_user)
> +			continue;
> +
> +		/*
> +		 * For ID registers, we return the writable mask. Other feature
> +		 * registers return a full 64bit mask. That's not necessary
> +		 * compliant with a given revision of the architecture, but the
> +		 * RES0/RES1 definitions allow us to do that.
> +		 */
> +		if (is_id_reg(encoding)) {
> +			if (!reg->val)
> +				continue;
> +			val = reg->val;
> +		} else {
> +			val = ~0UL;
> +		}
> +
> +		if (put_user(val, (masks + ARM64_FEATURE_ID_SPACE_INDEX(encoding))))
> +			return -EFAULT;
> +	}
> +
> +	return 0;
> +}
> +
>   int __init kvm_sys_reg_table_init(void)
>   {
>   	struct sys_reg_params params;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f089ab290978..86ffdf134eb8 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1555,6 +1555,8 @@ struct kvm_s390_ucas_mapping {
>   #define KVM_ARM_MTE_COPY_TAGS	  _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
>   /* Available with KVM_CAP_COUNTER_OFFSET */
>   #define KVM_ARM_SET_COUNTER_OFFSET _IOW(KVMIO,  0xb5, struct kvm_arm_counter_offset)
> +#define KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS \
> +			_IOR(KVMIO,  0xb6, struct feature_id_writable_masks)
>   
>   /* ioctl for vm fd */
>   #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)

-- 
Shaoqin


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

Re: [PATCH v7 01/10] KVM: arm64: Allow userspace to get the writable masks for feature ID registers

[Jing Zhang]

Hi Shaoqin,

On Wed, Aug 9, 2023 at 9:27 PM Shaoqin Huang <shahuang@redhat.com> wrote:
>
> Hi Jing,
>
> On 8/1/23 23:19, Jing Zhang wrote:
> > Add a VM ioctl to allow userspace to get writable masks for feature ID
> > registers in below system register space:
> > op0 = 3, op1 = {0, 1, 3}, CRn = 0, CRm = {0 - 7}, op2 = {0 - 7}
> > This is used to support mix-and-match userspace and kernels for writable
> > ID registers, where userspace may want to know upfront whether it can
> > actually tweak the contents of an idreg or not.
> >
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Suggested-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >   arch/arm64/include/asm/kvm_host.h |  2 ++
> >   arch/arm64/include/uapi/asm/kvm.h | 25 +++++++++++++++
> >   arch/arm64/kvm/arm.c              |  3 ++
> >   arch/arm64/kvm/sys_regs.c         | 51 +++++++++++++++++++++++++++++++
> >   include/uapi/linux/kvm.h          |  2 ++
> >   5 files changed, 83 insertions(+)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index d3dd05bbfe23..3996a3707f4e 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -1074,6 +1074,8 @@ int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm,
> >                              struct kvm_arm_copy_mte_tags *copy_tags);
> >   int kvm_vm_ioctl_set_counter_offset(struct kvm *kvm,
> >                                   struct kvm_arm_counter_offset *offset);
> > +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm,
> > +                                            u64 __user *masks);
> >
> >   /* Guest/host FPSIMD coordination helpers */
> >   int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index f7ddd73a8c0f..2970c0d792ee 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -505,6 +505,31 @@ struct kvm_smccc_filter {
> >   #define KVM_HYPERCALL_EXIT_SMC              (1U << 0)
> >   #define KVM_HYPERCALL_EXIT_16BIT    (1U << 1)
> >
> > +/* Get feature ID registers userspace writable mask. */
> > +/*
> > + * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
> > + * Feature Register 2"):
> > + *
> > + * "The Feature ID space is defined as the System register space in
> > + * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
> > + * op2=={0-7}."
> > + *
> > + * This covers all R/O registers that indicate anything useful feature
> > + * wise, including the ID registers.
> > + */
> > +#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)          \
> > +     ({                                                              \
> > +             __u64 __op1 = (op1) & 3;                                \
> > +             __op1 -= (__op1 == 3);                                  \
> > +             (__op1 << 6 | ((crm) & 7) << 3 | (op2));                \
> > +     })
> > +
> > +#define ARM64_FEATURE_ID_SPACE_SIZE  (3 * 8 * 8)
> > +
> > +struct feature_id_writable_masks {
> > +     __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
> > +};
> > +
> >   #endif
> >
> >   #endif /* __ARM_KVM_H__ */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 72dc53a75d1c..c9cd14057c58 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -1630,6 +1630,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> >
> >               return kvm_vm_set_attr(kvm, &attr);
> >       }
> > +     case KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS: {
> > +             return kvm_vm_ioctl_get_feature_id_writable_masks(kvm, argp);
> > +     }
> >       default:
> >               return -EINVAL;
> >       }
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 2ca2973abe66..d9317b640ba5 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -3560,6 +3560,57 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >       return write_demux_regids(uindices);
> >   }
> >
> > +#define ARM64_FEATURE_ID_SPACE_INDEX(r)                      \
> > +     ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(r),      \
> > +             sys_reg_Op1(r),                         \
> > +             sys_reg_CRn(r),                         \
> > +             sys_reg_CRm(r),                         \
> > +             sys_reg_Op2(r))
> > +
> > +static bool is_feature_id_reg(u32 encoding)
> > +{
> > +     return (sys_reg_Op0(encoding) == 3 &&
> > +             (sys_reg_Op1(encoding) < 2 || sys_reg_Op1(encoding) == 3) &&
> > +             sys_reg_CRn(encoding) == 0 &&
> > +             sys_reg_CRm(encoding) <= 7);
> > +}
>
> A fool question.
>
> In the code base, there is a function is_id_reg() to check if it's a
> id_reg, what's the difference between the is_feature_id_reg() and the
> is_id_reg()?
>
> /*
>   * Return true if the register's (Op0, Op1, CRn, CRm, Op2) is
>   * (3, 0, 0, crm, op2), where 1<=crm<8, 0<=op2<8.
>   */
> static inline bool is_id_reg(u32 id)
> {
>         return (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
>                 sys_reg_CRn(id) == 0 && sys_reg_CRm(id) >= 1 &&
>                 sys_reg_CRm(id) < 8);
> }
>
> Thanks,
> Shaoqin
>

Basically, is_feature_id_reg() is used to check whether the reg is in
the feature ID system register space. You can refer to page 6788 at
DDI0487J.a, D19.2.66 for the feature ID space range as below:
The Feature ID space is defined as the System register space in
AArch64 with op0==3, op1=={0,
1, 3}, CRn==0, CRm=={0-7}, op2=={0-7}.

But is_id_reg() is used to check whether the reg is one of the ID_*
regs which have been defined in the previous feature ID space range.
You can refer to the table D18-2 at page D18-6308 for the encoding.

Agreed that the names are somewhat confusing.

> > +
> > +int kvm_vm_ioctl_get_feature_id_writable_masks(struct kvm *kvm, u64 __user *masks)
> > +{
> > +     /* Wipe the whole thing first */
> > +     for (int i = 0; i < ARM64_FEATURE_ID_SPACE_SIZE; i++)
> > +             if (put_user(0, masks + i))
> > +                     return -EFAULT;
> > +
> > +     for (int i = 0; i < ARRAY_SIZE(sys_reg_descs); i++) {
> > +             const struct sys_reg_desc *reg = &sys_reg_descs[i];
> > +             u32 encoding = reg_to_encoding(reg);
> > +             u64 val;
> > +
> > +             if (!is_feature_id_reg(encoding) || !reg->set_user)
> > +                     continue;
> > +
> > +             /*
> > +              * For ID registers, we return the writable mask. Other feature
> > +              * registers return a full 64bit mask. That's not necessary
> > +              * compliant with a given revision of the architecture, but the
> > +              * RES0/RES1 definitions allow us to do that.
> > +              */
> > +             if (is_id_reg(encoding)) {
> > +                     if (!reg->val)
> > +                             continue;
> > +                     val = reg->val;
> > +             } else {
> > +                     val = ~0UL;
> > +             }
> > +
> > +             if (put_user(val, (masks + ARM64_FEATURE_ID_SPACE_INDEX(encoding))))
> > +                     return -EFAULT;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >   int __init kvm_sys_reg_table_init(void)
> >   {
> >       struct sys_reg_params params;
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index f089ab290978..86ffdf134eb8 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -1555,6 +1555,8 @@ struct kvm_s390_ucas_mapping {
> >   #define KVM_ARM_MTE_COPY_TAGS         _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
> >   /* Available with KVM_CAP_COUNTER_OFFSET */
> >   #define KVM_ARM_SET_COUNTER_OFFSET _IOW(KVMIO,  0xb5, struct kvm_arm_counter_offset)
> > +#define KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS \
> > +                     _IOR(KVMIO,  0xb6, struct feature_id_writable_masks)
> >
> >   /* ioctl for vm fd */
> >   #define KVM_CREATE_DEVICE     _IOWR(KVMIO,  0xe0, struct kvm_create_device)
>
> --
> Shaoqin
>

Jing

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

[PATCH v7 02/10] KVM: arm64: Document KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS

[Jing Zhang]

Add some basic documentation on how to get feature ID register writable
masks from userspace.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 Documentation/virt/kvm/api.rst | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index c0ddd3035462..e6cda4169764 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6068,6 +6068,32 @@ writes to the CNTVCT_EL0 and CNTPCT_EL0 registers using the SET_ONE_REG
 interface. No error will be returned, but the resulting offset will not be
 applied.
 
+4.139 KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS
+-------------------------------------------
+
+:Capability: none
+:Architectures: arm64
+:Type: vm ioctl
+:Parameters: struct feature_id_writable_masks (out)
+:Returns: 0 on success, < 0 on error
+
+
+::
+
+        #define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
+
+        struct feature_id_writable_masks {
+                __u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
+        };
+
+This ioctl would copy the writable masks for feature ID registers to userspace.
+The Feature ID space is defined as the System register space in AArch64 with
+op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7}, op2=={0-7}.
+To get the index in ``mask`` array for a specified feature ID register, use the
+macro ``ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)``.
+This allows the userspace to know upfront whether it can actually tweak the
+contents of a feature ID register or not.
+
 5. The kvm_run structure
 ========================
 
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 03/10] KVM: arm64: Use guest ID register values for the sake of emulation

[Jing Zhang]

Since KVM now supports per-VM ID registers, use per-VM ID register
values for the sake of emulation for DBGDIDR and LORegion.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d9317b640ba5..6eab45ce05d9 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -379,7 +379,7 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
+	u64 val = IDREG(vcpu->kvm, SYS_ID_AA64MMFR1_EL1);
 	u32 sr = reg_to_encoding(r);
 
 	if (!(val & (0xfUL << ID_AA64MMFR1_EL1_LO_SHIFT))) {
@@ -2429,8 +2429,8 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
 	if (p->is_write) {
 		return ignore_write(vcpu, p);
 	} else {
-		u64 dfr = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
-		u64 pfr = read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1);
+		u64 dfr = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
+		u64 pfr = IDREG(vcpu->kvm, SYS_ID_AA64PFR0_EL1);
 		u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL1_EL3_SHIFT);
 
 		p->regval = ((((dfr >> ID_AA64DFR0_EL1_WRPs_SHIFT) & 0xf) << 28) |
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 04/10] KVM: arm64: Reject attempts to set invalid debug arch version

[Jing Zhang]

From: Oliver Upton <oliver.upton@linux.dev>

The debug architecture is mandatory in ARMv8, so KVM should not allow
userspace to configure a vCPU with less than that. Of course, this isn't
handled elegantly by the generic ID register plumbing, as the respective
ID register fields have a nonzero starting value.

Add an explicit check for debug versions less than v8 of the
architecture.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6eab45ce05d9..7fcbc317f100 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1216,8 +1216,14 @@ static s64 kvm_arm64_ftr_safe_value(u32 id, const struct arm64_ftr_bits *ftrp,
 	/* Some features have different safe value type in KVM than host features */
 	switch (id) {
 	case SYS_ID_AA64DFR0_EL1:
-		if (kvm_ftr.shift == ID_AA64DFR0_EL1_PMUVer_SHIFT)
+		switch (kvm_ftr.shift) {
+		case ID_AA64DFR0_EL1_PMUVer_SHIFT:
 			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		case ID_AA64DFR0_EL1_DebugVer_SHIFT:
+			kvm_ftr.type = FTR_LOWER_SAFE;
+			break;
+		}
 		break;
 	case SYS_ID_DFR0_EL1:
 		if (kvm_ftr.shift == ID_DFR0_EL1_PerfMon_SHIFT)
@@ -1469,14 +1475,22 @@ static u64 read_sanitised_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 	return val;
 }
 
+#define ID_REG_LIMIT_FIELD_ENUM(val, reg, field, limit)			       \
+({									       \
+	u64 __f_val = FIELD_GET(reg##_##field##_MASK, val);		       \
+	(val) &= ~reg##_##field##_MASK;					       \
+	(val) |= FIELD_PREP(reg##_##field##_MASK,			       \
+			min(__f_val, (u64)reg##_##field##_##limit));	       \
+	(val);								       \
+})
+
 static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 					  const struct sys_reg_desc *rd)
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
 	/* Limit debug to ARMv8.0 */
-	val &= ~ID_AA64DFR0_EL1_DebugVer_MASK;
-	val |= SYS_FIELD_PREP_ENUM(ID_AA64DFR0_EL1, DebugVer, IMP);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1496,6 +1510,7 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 			       const struct sys_reg_desc *rd,
 			       u64 val)
 {
+	u8 debugver = SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, val);
 	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, val);
 
 	/*
@@ -1515,6 +1530,13 @@ static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	if (pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
 		val &= ~ID_AA64DFR0_EL1_PMUVer_MASK;
 
+	/*
+	 * ID_AA64DFR0_EL1.DebugVer is one of those awkward fields with a
+	 * nonzero minimum safe value.
+	 */
+	if (debugver < ID_AA64DFR0_EL1_DebugVer_IMP)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
@@ -1536,6 +1558,7 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 			   u64 val)
 {
 	u8 perfmon = SYS_FIELD_GET(ID_DFR0_EL1, PerfMon, val);
+	u8 copdbg = SYS_FIELD_GET(ID_DFR0_EL1, CopDbg, val);
 
 	if (perfmon == ID_DFR0_EL1_PerfMon_IMPDEF) {
 		val &= ~ID_DFR0_EL1_PerfMon_MASK;
@@ -1551,6 +1574,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (perfmon != 0 && perfmon < ID_DFR0_EL1_PerfMon_PMUv3)
 		return -EINVAL;
 
+	if (copdbg < ID_DFR0_EL1_CopDbg_Armv8)
+		return -EINVAL;
+
 	return set_id_reg(vcpu, rd, val);
 }
 
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 05/10] KVM: arm64: Enable writable for ID_AA64DFR0_EL1 and ID_DFR0_EL1

[Jing Zhang]

All valid fields in ID_AA64DFR0_EL1 and ID_DFR0_EL1 are writable
from usrespace with this change.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 7fcbc317f100..2183cd3af472 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2006,7 +2006,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .set_user = set_id_dfr0_el1,
 	  .visibility = aa32_id_visibility,
 	  .reset = read_sanitised_id_dfr0_el1,
-	  .val = ID_DFR0_EL1_PerfMon_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR1_EL1),
@@ -2055,7 +2055,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .get_user = get_id_reg,
 	  .set_user = set_id_aa64dfr0_el1,
 	  .reset = read_sanitised_id_aa64dfr0_el1,
-	  .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
 	ID_UNALLOCATED(5,3),
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 06/10] KVM: arm64: Bump up the default KVM sanitised debug version to v8p8

[Jing Zhang]

From: Oliver Upton <oliver.upton@linux.dev>

Since ID_AA64DFR0_EL1 and ID_DFR0_EL1 are now writable from userspace,
it is safe to bump up the default KVM sanitised debug version to v8p8.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2183cd3af472..5a886ccb33fa 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1489,8 +1489,7 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
-	/* Limit debug to ARMv8.0 */
-	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1550,6 +1549,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (kvm_vcpu_has_pmu(vcpu))
 		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
 
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
+
 	return val;
 }
 
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 07/10] KVM: arm64: Enable writable for ID_AA64PFR0_EL1

[Jing Zhang]

All valid fields in ID_AA64PFR0_EL1 are writable from usrespace
with this change.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5a886ccb33fa..0a406058abb9 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2041,7 +2041,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	  .get_user = get_id_reg,
 	  .set_user = set_id_reg,
 	  .reset = read_sanitised_id_aa64pfr0_el1,
-	  .val = ID_AA64PFR0_EL1_CSV2_MASK | ID_AA64PFR0_EL1_CSV3_MASK, },
+	  .val = GENMASK(63, 0), },
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 08/10] KVM: arm64: Refactor helper Macros for idreg desc

[Jing Zhang]

Add some helpers to ease the declaration for idreg desc.
These Macros will be heavily used for future commits enabling writable
for idregs.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 79 ++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 46 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0a406058abb9..9ca23cfec9e5 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1844,27 +1844,37 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
  * from userspace.
  */
 
-/* sys_reg_desc initialiser for known cpufeature ID registers */
-#define ID_SANITISED(name) {			\
-	SYS_DESC(SYS_##name),			\
-	.access	= access_id_reg,		\
-	.get_user = get_id_reg,			\
-	.set_user = set_id_reg,			\
-	.visibility = id_visibility,		\
-	.reset = kvm_read_sanitised_id_reg,	\
-	.val = 0,				\
+#define ID_DESC(name, _set_user, _visibility, _reset, mask) {	\
+	SYS_DESC(SYS_##name),					\
+	.access	= access_id_reg,				\
+	.get_user = get_id_reg,					\
+	.set_user = _set_user,					\
+	.visibility = _visibility,				\
+	.reset = _reset,					\
+	.val = mask,						\
 }
 
 /* sys_reg_desc initialiser for known cpufeature ID registers */
-#define AA32_ID_SANITISED(name) {		\
-	SYS_DESC(SYS_##name),			\
-	.access	= access_id_reg,		\
-	.get_user = get_id_reg,			\
-	.set_user = set_id_reg,			\
-	.visibility = aa32_id_visibility,	\
-	.reset = kvm_read_sanitised_id_reg,	\
-	.val = 0,				\
-}
+#define _ID_SANITISED(name, _set_user, _reset) \
+	ID_DESC(name, _set_user, id_visibility, _reset, 0)
+#define ID_SANITISED(name) \
+	_ID_SANITISED(name, set_id_reg, kvm_read_sanitised_id_reg)
+
+#define _ID_SANITISED_W(name, _set_user, _reset) \
+	ID_DESC(name, _set_user, id_visibility, _reset, GENMASK(63, 0))
+#define ID_SANITISED_W(name) \
+	_ID_SANITISED_W(name, set_id_reg, kvm_read_sanitised_id_reg)
+
+/* sys_reg_desc initialiser for known cpufeature ID registers */
+#define _AA32_ID_SANITISED(name, _set_user, _reset) \
+	ID_DESC(name, _set_user, aa32_id_visibility, _reset, 0)
+#define AA32_ID_SANITISED(name) \
+	_AA32_ID_SANITISED(name, set_id_reg, kvm_read_sanitised_id_reg)
+
+#define _AA32_ID_SANITISED_W(name, _set_user, _reset) \
+	ID_DESC(name, _set_user, aa32_id_visibility, _reset, GENMASK(63, 0))
+#define AA32_ID_SANITISED_W(name) \
+	_AA32_ID_SANITISED_W(name, set_id_reg, kvm_read_sanitised_id_reg)
 
 /*
  * sys_reg_desc initialiser for architecturally unallocated cpufeature ID
@@ -1886,15 +1896,8 @@ static unsigned int elx2_visibility(const struct kvm_vcpu *vcpu,
  * For now, these are exposed just like unallocated ID regs: they appear
  * RAZ for the guest.
  */
-#define ID_HIDDEN(name) {			\
-	SYS_DESC(SYS_##name),			\
-	.access = access_id_reg,		\
-	.get_user = get_id_reg,			\
-	.set_user = set_id_reg,			\
-	.visibility = raz_visibility,		\
-	.reset = kvm_read_sanitised_id_reg,	\
-	.val = 0,				\
-}
+#define ID_HIDDEN(name) \
+	ID_DESC(name, set_id_reg, raz_visibility, kvm_read_sanitised_id_reg, 0)
 
 static bool access_sp_el1(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
@@ -2001,13 +2004,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	/* CRm=1 */
 	AA32_ID_SANITISED(ID_PFR0_EL1),
 	AA32_ID_SANITISED(ID_PFR1_EL1),
-	{ SYS_DESC(SYS_ID_DFR0_EL1),
-	  .access = access_id_reg,
-	  .get_user = get_id_reg,
-	  .set_user = set_id_dfr0_el1,
-	  .visibility = aa32_id_visibility,
-	  .reset = read_sanitised_id_dfr0_el1,
-	  .val = GENMASK(63, 0), },
+	_AA32_ID_SANITISED_W(ID_DFR0_EL1, set_id_dfr0_el1, read_sanitised_id_dfr0_el1),
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR1_EL1),
@@ -2036,12 +2033,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	/* AArch64 ID registers */
 	/* CRm=4 */
-	{ SYS_DESC(SYS_ID_AA64PFR0_EL1),
-	  .access = access_id_reg,
-	  .get_user = get_id_reg,
-	  .set_user = set_id_reg,
-	  .reset = read_sanitised_id_aa64pfr0_el1,
-	  .val = GENMASK(63, 0), },
+	_ID_SANITISED_W(ID_AA64PFR0_EL1, set_id_reg, read_sanitised_id_aa64pfr0_el1),
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
@@ -2051,12 +2043,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_UNALLOCATED(4,7),
 
 	/* CRm=5 */
-	{ SYS_DESC(SYS_ID_AA64DFR0_EL1),
-	  .access = access_id_reg,
-	  .get_user = get_id_reg,
-	  .set_user = set_id_aa64dfr0_el1,
-	  .reset = read_sanitised_id_aa64dfr0_el1,
-	  .val = GENMASK(63, 0), },
+	_ID_SANITISED_W(ID_AA64DFR0_EL1, set_id_aa64dfr0_el1, read_sanitised_id_aa64dfr0_el1),
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
 	ID_UNALLOCATED(5,3),
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 09/10] KVM: arm64: Enable writable for ID_AA64MMFR{0, 1, 2, 3}_EL1

[Jing Zhang]

Enable writable from userspace for ID_AA64MMFR{0, 1, 2, 3}_EL1.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 arch/arm64/kvm/sys_regs.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 9ca23cfec9e5..67b50a088eac 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1346,9 +1346,6 @@ static u64 __kvm_read_sanitised_id_reg(const struct kvm_vcpu *vcpu,
 			val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_WFxT);
 		val &= ~ARM64_FEATURE_MASK(ID_AA64ISAR2_EL1_MOPS);
 		break;
-	case SYS_ID_AA64MMFR2_EL1:
-		val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
-		break;
 	case SYS_ID_MMFR4_EL1:
 		val &= ~ARM64_FEATURE_MASK(ID_MMFR4_EL1_CCIDX);
 		break;
@@ -1581,6 +1578,15 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	return set_id_reg(vcpu, rd, val);
 }
 
+static u64 read_sanitised_id_aa64mmfr2_el1(struct kvm_vcpu *vcpu,
+					   const struct sys_reg_desc *rd)
+{
+	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR2_EL1);
+
+	val &= ~ID_AA64MMFR2_EL1_CCIDX_MASK;
+	return val;
+}
+
 /*
  * cpufeature ID register user accessors
  *
@@ -2063,10 +2069,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_UNALLOCATED(6,7),
 
 	/* CRm=7 */
-	ID_SANITISED(ID_AA64MMFR0_EL1),
-	ID_SANITISED(ID_AA64MMFR1_EL1),
-	ID_SANITISED(ID_AA64MMFR2_EL1),
-	ID_SANITISED(ID_AA64MMFR3_EL1),
+	ID_SANITISED_W(ID_AA64MMFR0_EL1),
+	ID_SANITISED_W(ID_AA64MMFR1_EL1),
+	_ID_SANITISED_W(ID_AA64MMFR2_EL1, set_id_reg, read_sanitised_id_aa64mmfr2_el1),
+	ID_SANITISED_W(ID_AA64MMFR3_EL1),
 	ID_UNALLOCATED(7,4),
 	ID_UNALLOCATED(7,5),
 	ID_UNALLOCATED(7,6),
-- 
2.41.0.585.gd2178a4bd4-goog


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

[PATCH v7 10/10] KVM: arm64: selftests: Test for setting ID register from usersapce

[Jing Zhang]

Add tests to verify setting ID registers from userapce is handled
correctly by KVM. Also add a test case to use ioctl
KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS to get writable masks.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
---
 tools/arch/arm64/include/uapi/asm/kvm.h       |  25 +++
 tools/include/uapi/linux/kvm.h                |   2 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/set_id_regs.c       | 191 ++++++++++++++++++
 4 files changed, 219 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c

diff --git a/tools/arch/arm64/include/uapi/asm/kvm.h b/tools/arch/arm64/include/uapi/asm/kvm.h
index f7ddd73a8c0f..2970c0d792ee 100644
--- a/tools/arch/arm64/include/uapi/asm/kvm.h
+++ b/tools/arch/arm64/include/uapi/asm/kvm.h
@@ -505,6 +505,31 @@ struct kvm_smccc_filter {
 #define KVM_HYPERCALL_EXIT_SMC		(1U << 0)
 #define KVM_HYPERCALL_EXIT_16BIT	(1U << 1)
 
+/* Get feature ID registers userspace writable mask. */
+/*
+ * From DDI0487J.a, D19.2.66 ("ID_AA64MMFR2_EL1, AArch64 Memory Model
+ * Feature Register 2"):
+ *
+ * "The Feature ID space is defined as the System register space in
+ * AArch64 with op0==3, op1=={0, 1, 3}, CRn==0, CRm=={0-7},
+ * op2=={0-7}."
+ *
+ * This covers all R/O registers that indicate anything useful feature
+ * wise, including the ID registers.
+ */
+#define ARM64_FEATURE_ID_SPACE_IDX(op0, op1, crn, crm, op2)		\
+	({								\
+		__u64 __op1 = (op1) & 3;				\
+		__op1 -= (__op1 == 3);					\
+		(__op1 << 6 | ((crm) & 7) << 3 | (op2));		\
+	})
+
+#define ARM64_FEATURE_ID_SPACE_SIZE	(3 * 8 * 8)
+
+struct feature_id_writable_masks {
+	__u64 mask[ARM64_FEATURE_ID_SPACE_SIZE];
+};
+
 #endif
 
 #endif /* __ARM_KVM_H__ */
diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index f089ab290978..9abe69cf4001 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -1555,6 +1555,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_ARM_MTE_COPY_TAGS	  _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
 /* Available with KVM_CAP_COUNTER_OFFSET */
 #define KVM_ARM_SET_COUNTER_OFFSET _IOW(KVMIO,  0xb5, struct kvm_arm_counter_offset)
+#define KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS \
+			_IOR(KVMIO,  0xb6, struct kvm_arm_feature_id_masks)
 
 /* ioctl for vm fd */
 #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c692cc86e7da..87ceadc1292a 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -144,6 +144,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
 TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
+TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/smccc_filter
 TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
new file mode 100644
index 000000000000..9c8f439ac7b3
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * set_id_regs - Test for setting ID register from usersapce.
+ *
+ * Copyright (c) 2023 Google LLC.
+ *
+ *
+ * Test that KVM supports setting ID registers from userspace and handles the
+ * feature set correctly.
+ */
+
+#include <stdint.h>
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+#include <linux/bitfield.h>
+
+#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffsl(_mask) - 1))
+#define field_prep(_mask, _val) (((_val) << (ffsl(_mask) - 1)) & (_mask))
+
+struct reg_feature {
+	uint64_t reg;
+	uint64_t ftr_mask;
+};
+
+static void guest_code(void)
+{
+	for (;;)
+		GUEST_SYNC(0);
+}
+
+static struct reg_feature lower_safe_reg_ftrs[] = {
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_WRPS) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL3) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_FGT) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR1_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR1_PAN) },
+	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR2_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR2_FWB) },
+};
+
+static void test_user_set_lower_safe(struct kvm_vcpu *vcpu)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
+		uint64_t val, new_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+
+		/* Set a safe value for the feature */
+		if (ftr > 0)
+			ftr--;
+
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+
+		vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &new_val);
+		ASSERT_EQ(new_val, val);
+	}
+}
+
+static void test_user_set_fail(struct kvm_vcpu *vcpu)
+{
+	int i, r;
+
+	for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
+		uint64_t val, old_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+
+		/* Set a invalid value (too big) for the feature */
+		if (ftr >= GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0))
+			continue;
+		ftr++;
+
+		old_val = val;
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+
+		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		TEST_ASSERT(r < 0 && errno == EINVAL,
+			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+	}
+}
+
+static struct reg_feature exact_reg_ftrs[] = {
+	/* Items will be added when there is appropriate field of type
+	 * FTR_EXACT enabled writing from userspace later.
+	 */
+};
+
+static void test_user_set_exact(struct kvm_vcpu *vcpu)
+{
+	int i, r;
+
+	for (i = 0; i < ARRAY_SIZE(exact_reg_ftrs); i++) {
+		struct reg_feature *reg_ftr = exact_reg_ftrs + i;
+		uint64_t val, old_val, ftr;
+
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ftr = field_get(reg_ftr->ftr_mask, val);
+		old_val = val;
+
+		/* Exact match */
+		vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+
+		/* Smaller value */
+		if (ftr > 0) {
+			ftr--;
+			val &= ~reg_ftr->ftr_mask;
+			val |= field_prep(reg_ftr->ftr_mask, ftr);
+			r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+			TEST_ASSERT(r < 0 && errno == EINVAL,
+				    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+			vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+			ASSERT_EQ(val, old_val);
+			ftr++;
+		}
+
+		/* Bigger value */
+		ftr++;
+		val &= ~reg_ftr->ftr_mask;
+		val |= field_prep(reg_ftr->ftr_mask, ftr);
+		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
+		TEST_ASSERT(r < 0 && errno == EINVAL,
+			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
+		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
+		ASSERT_EQ(val, old_val);
+	}
+}
+
+static uint32_t writable_regs[] = {
+	SYS_ID_DFR0_EL1,
+	SYS_ID_AA64DFR0_EL1,
+	SYS_ID_AA64PFR0_EL1,
+	SYS_ID_AA64MMFR0_EL1,
+	SYS_ID_AA64MMFR1_EL1,
+	SYS_ID_AA64MMFR2_EL1,
+};
+
+void test_user_get_writable_masks(struct kvm_vm *vm)
+{
+	struct feature_id_writable_masks masks;
+
+	vm_ioctl(vm, KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS, &masks);
+
+	for (int i = 0; i < ARRAY_SIZE(writable_regs); i++) {
+		uint32_t reg = writable_regs[i];
+		int idx = ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(reg),
+				sys_reg_Op1(reg), sys_reg_CRn(reg),
+				sys_reg_CRm(reg), sys_reg_Op2(reg));
+
+		ASSERT_EQ(masks.mask[idx], GENMASK_ULL(63, 0));
+	}
+}
+
+int main(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	ksft_print_header();
+	ksft_set_plan(4);
+
+	test_user_get_writable_masks(vm);
+	ksft_test_result_pass("test_user_get_writable_masks\n");
+
+	test_user_set_exact(vcpu);
+	ksft_test_result_pass("test_user_set_exact\n");
+
+	test_user_set_fail(vcpu);
+	ksft_test_result_pass("test_user_set_fail\n");
+
+	test_user_set_lower_safe(vcpu);
+	ksft_test_result_pass("test_user_set_lower_safe\n");
+
+	kvm_vm_free(vm);
+
+	ksft_finished();
+}
-- 
2.41.0.585.gd2178a4bd4-goog


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

Re: [PATCH v7 10/10] KVM: arm64: selftests: Test for setting ID register from usersapce

[Oliver Upton]

Hi Jing,

On Tue, Aug 01, 2023 at 08:20:06AM -0700, Jing Zhang wrote:
> Add tests to verify setting ID registers from userapce is handled
> correctly by KVM. Also add a test case to use ioctl
> KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS to get writable masks.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  tools/arch/arm64/include/uapi/asm/kvm.h       |  25 +++
>  tools/include/uapi/linux/kvm.h                |   2 +

Why is this diff needed? I thought we wound up using the latest headers
from the kernel.

>  tools/testing/selftests/kvm/Makefile          |   1 +

Need to add your file to .gitignore too.

>  .../selftests/kvm/aarch64/set_id_regs.c       | 191 ++++++++++++++++++
>  4 files changed, 219 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c

[...]

> diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> new file mode 100644
> index 000000000000..9c8f439ac7b3
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * set_id_regs - Test for setting ID register from usersapce.
> + *
> + * Copyright (c) 2023 Google LLC.
> + *
> + *
> + * Test that KVM supports setting ID registers from userspace and handles the
> + * feature set correctly.
> + */
> +
> +#include <stdint.h>
> +#include "kvm_util.h"
> +#include "processor.h"
> +#include "test_util.h"
> +#include <linux/bitfield.h>
> +
> +#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffsl(_mask) - 1))
> +#define field_prep(_mask, _val) (((_val) << (ffsl(_mask) - 1)) & (_mask))
> +

Shadowing the naming of the kernel's own FIELD_{GET,PREP}() is a bit
awkward. I'm guessing that you're working around @_mask not being a
compile-time constant?

> +struct reg_feature {
> +	uint64_t reg;
> +	uint64_t ftr_mask;
> +};
> +
> +static void guest_code(void)
> +{
> +	for (;;)
> +		GUEST_SYNC(0);
> +}

The test should check that the written values are visible both from the
guest as well as userspace.

> +static struct reg_feature lower_safe_reg_ftrs[] = {
> +	{ KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_WRPS) },
> +	{ KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL3) },
> +	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_FGT) },
> +	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR1_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR1_PAN) },
> +	{ KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR2_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR2_FWB) },
> +};

My preference would be to organize the field descriptors by register
rather than the policy. This matches what the kernel does in cpufeature.c
quite closely and allows us to easily reason about which fields are/aren't
tested.

> +static void test_user_set_lower_safe(struct kvm_vcpu *vcpu)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> +		struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> +		uint64_t val, new_val, ftr;
> +
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ftr = field_get(reg_ftr->ftr_mask, val);
> +
> +		/* Set a safe value for the feature */
> +		if (ftr > 0)
> +			ftr--;
> +
> +		val &= ~reg_ftr->ftr_mask;
> +		val |= field_prep(reg_ftr->ftr_mask, ftr);
> +
> +		vcpu_set_reg(vcpu, reg_ftr->reg, val);
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &new_val);
> +		ASSERT_EQ(new_val, val);
> +	}
> +}
> +
> +static void test_user_set_fail(struct kvm_vcpu *vcpu)
> +{
> +	int i, r;
> +
> +	for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> +		struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> +		uint64_t val, old_val, ftr;
> +
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ftr = field_get(reg_ftr->ftr_mask, val);
> +
> +		/* Set a invalid value (too big) for the feature */
> +		if (ftr >= GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0))
> +			continue;

This assumes that the fields in the register are unsigned, but there are
several which are not.

> +		ftr++;
> +
> +		old_val = val;
> +		val &= ~reg_ftr->ftr_mask;
> +		val |= field_prep(reg_ftr->ftr_mask, ftr);
> +
> +		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> +		TEST_ASSERT(r < 0 && errno == EINVAL,
> +			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> +
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ASSERT_EQ(val, old_val);
> +	}
> +}
> +
> +static struct reg_feature exact_reg_ftrs[] = {
> +	/* Items will be added when there is appropriate field of type
> +	 * FTR_EXACT enabled writing from userspace later.
> +	 */
> +};
> +
> +static void test_user_set_exact(struct kvm_vcpu *vcpu)
> +{
> +	int i, r;
> +
> +	for (i = 0; i < ARRAY_SIZE(exact_reg_ftrs); i++) {
> +		struct reg_feature *reg_ftr = exact_reg_ftrs + i;
> +		uint64_t val, old_val, ftr;
> +
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ftr = field_get(reg_ftr->ftr_mask, val);
> +		old_val = val;
> +
> +		/* Exact match */
> +		vcpu_set_reg(vcpu, reg_ftr->reg, val);
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ASSERT_EQ(val, old_val);
> +
> +		/* Smaller value */
> +		if (ftr > 0) {
> +			ftr--;
> +			val &= ~reg_ftr->ftr_mask;
> +			val |= field_prep(reg_ftr->ftr_mask, ftr);
> +			r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> +			TEST_ASSERT(r < 0 && errno == EINVAL,
> +				    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> +			vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +			ASSERT_EQ(val, old_val);
> +			ftr++;
> +		}
> +
> +		/* Bigger value */
> +		ftr++;
> +		val &= ~reg_ftr->ftr_mask;
> +		val |= field_prep(reg_ftr->ftr_mask, ftr);
> +		r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> +		TEST_ASSERT(r < 0 && errno == EINVAL,
> +			    "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> +		vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> +		ASSERT_EQ(val, old_val);
> +	}
> +}

Don't add dead code, this can be added when we actually test FTR_EXACT
fields. Are there not any in the registers exposed by this series?

> +static uint32_t writable_regs[] = {
> +	SYS_ID_DFR0_EL1,
> +	SYS_ID_AA64DFR0_EL1,
> +	SYS_ID_AA64PFR0_EL1,
> +	SYS_ID_AA64MMFR0_EL1,
> +	SYS_ID_AA64MMFR1_EL1,
> +	SYS_ID_AA64MMFR2_EL1,
> +};
> +
> +void test_user_get_writable_masks(struct kvm_vm *vm)
> +{
> +	struct feature_id_writable_masks masks;
> +
> +	vm_ioctl(vm, KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS, &masks);
> +
> +	for (int i = 0; i < ARRAY_SIZE(writable_regs); i++) {
> +		uint32_t reg = writable_regs[i];
> +		int idx = ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(reg),
> +				sys_reg_Op1(reg), sys_reg_CRn(reg),
> +				sys_reg_CRm(reg), sys_reg_Op2(reg));
> +
> +		ASSERT_EQ(masks.mask[idx], GENMASK_ULL(63, 0));
> +	}
> +}

The more robust test would be to check that every field this test knows
is writable is actually advertised as such in the ioctl. So you could
fetch this array at the start of the entire test and pass it through to
the routines that do granular checks against the fields of every
register.

It'd also be good to see basic sanity tests on the ioctl (i.e. call
fails if ::rsvd is nonzero), since KVM has screwed that up on several
occasions in past.

> +int main(void)
> +{
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_vm *vm;
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +
> +	ksft_print_header();
> +	ksft_set_plan(4);
> +
> +	test_user_get_writable_masks(vm);
> +	ksft_test_result_pass("test_user_get_writable_masks\n");
> +
> +	test_user_set_exact(vcpu);
> +	ksft_test_result_pass("test_user_set_exact\n");
> +
> +	test_user_set_fail(vcpu);
> +	ksft_test_result_pass("test_user_set_fail\n");
> +
> +	test_user_set_lower_safe(vcpu);
> +	ksft_test_result_pass("test_user_set_lower_safe\n");
> +
> +	kvm_vm_free(vm);
> +
> +	ksft_finished();
> +}
> -- 
> 2.41.0.585.gd2178a4bd4-goog
> 

-- 
Thanks,
Oliver

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

Re: [PATCH v7 10/10] KVM: arm64: selftests: Test for setting ID register from usersapce

[Jing Zhang]

Hi Oliver,

On Wed, Aug 2, 2023 at 10:28 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Jing,
>
> On Tue, Aug 01, 2023 at 08:20:06AM -0700, Jing Zhang wrote:
> > Add tests to verify setting ID registers from userapce is handled
> > correctly by KVM. Also add a test case to use ioctl
> > KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS to get writable masks.
> >
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > ---
> >  tools/arch/arm64/include/uapi/asm/kvm.h       |  25 +++
> >  tools/include/uapi/linux/kvm.h                |   2 +
>
> Why is this diff needed? I thought we wound up using the latest headers
> from the kernel.

You are right. These changes are not needed. Will drop them.

>
> >  tools/testing/selftests/kvm/Makefile          |   1 +
>
> Need to add your file to .gitignore too.

Will do.

>
> >  .../selftests/kvm/aarch64/set_id_regs.c       | 191 ++++++++++++++++++
> >  4 files changed, 219 insertions(+)
> >  create mode 100644 tools/testing/selftests/kvm/aarch64/set_id_regs.c
>
> [...]
>
> > diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> > new file mode 100644
> > index 000000000000..9c8f439ac7b3
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> > @@ -0,0 +1,191 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * set_id_regs - Test for setting ID register from usersapce.
> > + *
> > + * Copyright (c) 2023 Google LLC.
> > + *
> > + *
> > + * Test that KVM supports setting ID registers from userspace and handles the
> > + * feature set correctly.
> > + */
> > +
> > +#include <stdint.h>
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "test_util.h"
> > +#include <linux/bitfield.h>
> > +
> > +#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffsl(_mask) - 1))
> > +#define field_prep(_mask, _val) (((_val) << (ffsl(_mask) - 1)) & (_mask))
> > +
>
> Shadowing the naming of the kernel's own FIELD_{GET,PREP}() is a bit
> awkward. I'm guessing that you're working around @_mask not being a
> compile-time constant?

Yes, they are used to work around @_mask not being a compile-time constant.
As we discussed offline, I'll port automatic system registers
definition generation in selftests and use those definition instead.

>
> > +struct reg_feature {
> > +     uint64_t reg;
> > +     uint64_t ftr_mask;
> > +};
> > +
> > +static void guest_code(void)
> > +{
> > +     for (;;)
> > +             GUEST_SYNC(0);
> > +}
>
> The test should check that the written values are visible both from the
> guest as well as userspace.

Sure. Will add checks in guest too.

>
> > +static struct reg_feature lower_safe_reg_ftrs[] = {
> > +     { KVM_ARM64_SYS_REG(SYS_ID_AA64DFR0_EL1), ARM64_FEATURE_MASK(ID_AA64DFR0_WRPS) },
> > +     { KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), ARM64_FEATURE_MASK(ID_AA64PFR0_EL3) },
> > +     { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR0_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR0_FGT) },
> > +     { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR1_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR1_PAN) },
> > +     { KVM_ARM64_SYS_REG(SYS_ID_AA64MMFR2_EL1), ARM64_FEATURE_MASK(ID_AA64MMFR2_FWB) },
> > +};
>
> My preference would be to organize the field descriptors by register
> rather than the policy. This matches what the kernel does in cpufeature.c
> quite closely and allows us to easily reason about which fields are/aren't
> tested.

Sure. Will do.

>
> > +static void test_user_set_lower_safe(struct kvm_vcpu *vcpu)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> > +             struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> > +             uint64_t val, new_val, ftr;
> > +
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ftr = field_get(reg_ftr->ftr_mask, val);
> > +
> > +             /* Set a safe value for the feature */
> > +             if (ftr > 0)
> > +                     ftr--;
> > +
> > +             val &= ~reg_ftr->ftr_mask;
> > +             val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +
> > +             vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &new_val);
> > +             ASSERT_EQ(new_val, val);
> > +     }
> > +}
> > +
> > +static void test_user_set_fail(struct kvm_vcpu *vcpu)
> > +{
> > +     int i, r;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(lower_safe_reg_ftrs); i++) {
> > +             struct reg_feature *reg_ftr = lower_safe_reg_ftrs + i;
> > +             uint64_t val, old_val, ftr;
> > +
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ftr = field_get(reg_ftr->ftr_mask, val);
> > +
> > +             /* Set a invalid value (too big) for the feature */
> > +             if (ftr >= GENMASK_ULL(ARM64_FEATURE_FIELD_BITS - 1, 0))
> > +                     continue;
>
> This assumes that the fields in the register are unsigned, but there are
> several which are not.

This is based on the reg/fields which are defined previously. Those
are carefully chosen without unsigned ones.

>
> > +             ftr++;
> > +
> > +             old_val = val;
> > +             val &= ~reg_ftr->ftr_mask;
> > +             val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +
> > +             r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > +             TEST_ASSERT(r < 0 && errno == EINVAL,
> > +                         "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > +
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ASSERT_EQ(val, old_val);
> > +     }
> > +}
> > +
> > +static struct reg_feature exact_reg_ftrs[] = {
> > +     /* Items will be added when there is appropriate field of type
> > +      * FTR_EXACT enabled writing from userspace later.
> > +      */
> > +};
> > +
> > +static void test_user_set_exact(struct kvm_vcpu *vcpu)
> > +{
> > +     int i, r;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(exact_reg_ftrs); i++) {
> > +             struct reg_feature *reg_ftr = exact_reg_ftrs + i;
> > +             uint64_t val, old_val, ftr;
> > +
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ftr = field_get(reg_ftr->ftr_mask, val);
> > +             old_val = val;
> > +
> > +             /* Exact match */
> > +             vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ASSERT_EQ(val, old_val);
> > +
> > +             /* Smaller value */
> > +             if (ftr > 0) {
> > +                     ftr--;
> > +                     val &= ~reg_ftr->ftr_mask;
> > +                     val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +                     r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > +                     TEST_ASSERT(r < 0 && errno == EINVAL,
> > +                                 "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > +                     vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +                     ASSERT_EQ(val, old_val);
> > +                     ftr++;
> > +             }
> > +
> > +             /* Bigger value */
> > +             ftr++;
> > +             val &= ~reg_ftr->ftr_mask;
> > +             val |= field_prep(reg_ftr->ftr_mask, ftr);
> > +             r = __vcpu_set_reg(vcpu, reg_ftr->reg, val);
> > +             TEST_ASSERT(r < 0 && errno == EINVAL,
> > +                         "Unexpected KVM_SET_ONE_REG error: r=%d, errno=%d", r, errno);
> > +             vcpu_get_reg(vcpu, reg_ftr->reg, &val);
> > +             ASSERT_EQ(val, old_val);
> > +     }
> > +}
>
> Don't add dead code, this can be added when we actually test FTR_EXACT
> fields. Are there not any in the registers exposed by this series?

Nope, there is no in the registers exposed by this series.
Anyway, I'll drop this code in this series.

>
> > +static uint32_t writable_regs[] = {
> > +     SYS_ID_DFR0_EL1,
> > +     SYS_ID_AA64DFR0_EL1,
> > +     SYS_ID_AA64PFR0_EL1,
> > +     SYS_ID_AA64MMFR0_EL1,
> > +     SYS_ID_AA64MMFR1_EL1,
> > +     SYS_ID_AA64MMFR2_EL1,
> > +};
> > +
> > +void test_user_get_writable_masks(struct kvm_vm *vm)
> > +{
> > +     struct feature_id_writable_masks masks;
> > +
> > +     vm_ioctl(vm, KVM_ARM_GET_FEATURE_ID_WRITABLE_MASKS, &masks);
> > +
> > +     for (int i = 0; i < ARRAY_SIZE(writable_regs); i++) {
> > +             uint32_t reg = writable_regs[i];
> > +             int idx = ARM64_FEATURE_ID_SPACE_IDX(sys_reg_Op0(reg),
> > +                             sys_reg_Op1(reg), sys_reg_CRn(reg),
> > +                             sys_reg_CRm(reg), sys_reg_Op2(reg));
> > +
> > +             ASSERT_EQ(masks.mask[idx], GENMASK_ULL(63, 0));
> > +     }
> > +}
>
> The more robust test would be to check that every field this test knows
> is writable is actually advertised as such in the ioctl. So you could
> fetch this array at the start of the entire test and pass it through to
> the routines that do granular checks against the fields of every
> register.

Makes sense. Will do.

>
> It'd also be good to see basic sanity tests on the ioctl (i.e. call
> fails if ::rsvd is nonzero), since KVM has screwed that up on several
> occasions in past.

Sure. Will add some basic sanity tests on the ioctl.

>
> > +int main(void)
> > +{
> > +     struct kvm_vcpu *vcpu;
> > +     struct kvm_vm *vm;
> > +
> > +     vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > +
> > +     ksft_print_header();
> > +     ksft_set_plan(4);
> > +
> > +     test_user_get_writable_masks(vm);
> > +     ksft_test_result_pass("test_user_get_writable_masks\n");
> > +
> > +     test_user_set_exact(vcpu);
> > +     ksft_test_result_pass("test_user_set_exact\n");
> > +
> > +     test_user_set_fail(vcpu);
> > +     ksft_test_result_pass("test_user_set_fail\n");
> > +
> > +     test_user_set_lower_safe(vcpu);
> > +     ksft_test_result_pass("test_user_set_lower_safe\n");
> > +
> > +     kvm_vm_free(vm);
> > +
> > +     ksft_finished();
> > +}
> > --
> > 2.41.0.585.gd2178a4bd4-goog
> >
>
> --
> Thanks,
> Oliver

Thanks,
Jing

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