| Message ID | 20230801152007.337272-2-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable writable for idregs DFR0,PFR0, MMFR{0,1,2,3} | expand |
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.
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
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.
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
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?
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.
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)
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
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)
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(+)