[RFC 0/4] kvm: arm64: emulate ID registers

[RFC 0/4] kvm: arm64: emulate ID registers

[Peng Liang]

In AArch64, guest will read the same values of the ID regsiters with
host.  Both of them read the values from arm64_ftr_regs.  This patch
series add support to emulate and configure ID registers so that we can
control the value of ID registers that guest read.

Peng Liang (4):
  arm64: add a helper function to traverse arm64_ftr_regs
  kvm: arm64: emulate the ID registers
  kvm: arm64: make ID registers configurable
  kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

 arch/arm64/include/asm/cpufeature.h |  2 ++
 arch/arm64/include/asm/kvm_host.h   |  2 ++
 arch/arm64/kernel/cpufeature.c      | 13 ++++++++
 arch/arm64/kvm/arm.c                | 21 ++++++++++++
 arch/arm64/kvm/sys_regs.c           | 50 ++++++++++++++++++++++-------
 include/uapi/linux/kvm.h            | 12 +++++++
 6 files changed, 89 insertions(+), 11 deletions(-)

-- 
2.18.4

[RFC 1/4] arm64: add a helper function to traverse arm64_ftr_regs

[Peng Liang]

If we want to emulate ID registers, we need to initialize ID registers
firstly.  This commit is to add a helper function to traverse
arm64_ftr_regs so that we can initialize ID registers from
arm64_ftr_regs.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/include/asm/cpufeature.h |  2 ++
 arch/arm64/kernel/cpufeature.c      | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 89b4f0142c28..2ba7c4f11d8a 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -79,6 +79,8 @@ struct arm64_ftr_reg {
 
 extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
+
 /*
  * CPU capabilities:
  *
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index a389b999482e..eb4c78b85a35 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
 	return regp->sys_val;
 }
 
+int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
+{
+	int i, ret;
+
+	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
+		ret = (*op)(arm64_ftr_regs[i].sys_id,
+			    arm64_ftr_regs[i].reg->sys_val, argp);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 #define read_sysreg_case(r)	\
 	case r:		return read_sysreg_s(r)
 
-- 
2.18.4

[RFC 2/4] kvm: arm64: emulate the ID registers

[Peng Liang]

To emulate the ID registers, we need a place to storage the values of
the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.

This commit has no functional changes but only code refactor.  When
initializing a vcpu, get the values of the ID registers from
arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
the value from kvm_arch_vcpu when getting/setting the value of the ID
regs.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/include/asm/kvm_host.h |  2 ++
 arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
 include/uapi/linux/kvm.h          | 11 +++++++++++
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f81151ad3d3c..7f7bd36702f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
 		u64 last_steal;
 		gpa_t base;
 	} steal;
+
+	struct id_registers idregs;
 };
 
 /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 73e12869afe3..18ebbe1c64ee 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
 	return 0;
 }
 
+static int get_cpu_ftr(u32 id, u64 val, void *argp)
+{
+	struct id_registers *idregs = argp;
+
+	/*
+	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
+	 * where 1<=crm<8, 0<=op2<8.
+	 */
+	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
+	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
+		idregs->regs[idregs->num].sys_id = id;
+		idregs->regs[idregs->num].sys_val = val;
+		idregs->num++;
+	}
+
+	return 0;
+}
+
 int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 {
 	int err;
@@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	if (err)
 		return err;
 
+	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
+
 	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
 }
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 138961d7ebe3..776c2757a01e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
+{
+	int i;
+
+	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
+		if (vcpu->arch.idregs.regs[i].sys_id == id)
+			return &vcpu->arch.idregs.regs[i];
+	}
+	return NULL;
+}
+
+static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
+{
+	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
+
+	BUG_ON(!ri);
+	return ri->sys_val;
+}
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
-static u64 read_id_reg(const struct kvm_vcpu *vcpu,
+static u64 read_id_reg(struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)
 {
 	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
 			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
-	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
+	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
 
 	if (id == SYS_ID_AA64PFR0_EL1) {
 		if (!vcpu_has_sve(vcpu))
@@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
  * are stored, and for set_id_reg() we don't allow the effective value
  * to be changed.
  */
-static int __get_id_reg(const struct kvm_vcpu *vcpu,
+static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
@@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
 	return reg_to_user(uaddr, &val, id);
 }
 
-static int __set_id_reg(const struct kvm_vcpu *vcpu,
+static int __set_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
 			bool raz)
 {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index f6d86033c4fa..1029444d04aa 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
 	__s32	tablefd;
 };
 
+#define ID_REG_MAX_NUMS 64
+struct id_reg_info {
+	uint64_t sys_id;
+	uint64_t sys_val;
+};
+
+struct id_registers {
+	struct id_reg_info regs[ID_REG_MAX_NUMS];
+	uint64_t num;
+};
+
 /*
  * ioctls for VM fds
  */
-- 
2.18.4

[RFC 3/4] kvm: arm64: make ID registers configurable

[Peng Liang]

It's time to make ID registers configurable.  When userspace (but not
guest) want to set the values of ID registers, save the value in
kvm_arch_vcpu so that guest can read the modified values.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 776c2757a01e..f98635489966 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
 	return ri->sys_val;
 }
 
+static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value)
+{
+	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
+
+	BUG_ON(!ri);
+	ri->sys_val = value;
+}
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
 static u64 read_id_reg(struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)
@@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 
 /*
  * cpufeature ID register user accessors
- *
- * For now, these registers are immutable for userspace, so no values
- * are stored, and for set_id_reg() we don't allow the effective value
- * to be changed.
  */
 static int __get_id_reg(struct kvm_vcpu *vcpu,
 			const struct sys_reg_desc *rd, void __user *uaddr,
@@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
 	if (err)
 		return err;
 
-	/* This is what we mean by invariant: you can't change it. */
-	if (val != read_id_reg(vcpu, rd, raz))
-		return -EINVAL;
+	if (raz) {
+		if (val != read_id_reg(vcpu, rd, raz))
+			return -EINVAL;
+	} else {
+		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
+				     (u32)rd->CRm, (u32)rd->Op2);
+		kvm_set_id_reg(vcpu, reg_id, val);
+	}
 
 	return 0;
 }
-- 
2.18.4

[RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
supports to set CPU features in AArch64.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 arch/arm64/kvm/arm.c     | 1 +
 include/uapi/linux/kvm.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 18ebbe1c64ee..72b9e8fc606f 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
 	case KVM_CAP_ARM_NISV_TO_USER:
 	case KVM_CAP_ARM_INJECT_EXT_DABT:
+	case KVM_CAP_ARM_CPU_FEATURE:
 		r = 1;
 		break;
 	case KVM_CAP_ARM_SET_DEVICE_ADDR:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 1029444d04aa..0eca4f7c7fef 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_LAST_CPU 184
 #define KVM_CAP_SMALLER_MAXPHYADDR 185
 #define KVM_CAP_S390_DIAG318 186
+#define KVM_CAP_ARM_CPU_FEATURE 187
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
2.18.4

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Andrew Jones]

On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote:
> To emulate the ID registers, we need a place to storage the values of
> the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
> 
> This commit has no functional changes but only code refactor.  When
> initializing a vcpu, get the values of the ID registers from
> arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
> the value from kvm_arch_vcpu when getting/setting the value of the ID
> regs.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 ++
>  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
>  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
>  include/uapi/linux/kvm.h          | 11 +++++++++++
>  4 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f81151ad3d3c..7f7bd36702f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	struct id_registers idregs;
>  };
>  
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 73e12869afe3..18ebbe1c64ee 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
>  	return 0;
>  }
>  
> +static int get_cpu_ftr(u32 id, u64 val, void *argp)
> +{
> +	struct id_registers *idregs = argp;
> +
> +	/*
> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> +	 * where 1<=crm<8, 0<=op2<8.
> +	 */
> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
> +		idregs->regs[idregs->num].sys_id = id;
> +		idregs->regs[idregs->num].sys_val = val;
> +		idregs->num++;

This num++ means we should ensure get_cpu_ftr() is only used once per
VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2

> +	}
> +
> +	return 0;
> +}
> +
>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  {
>  	int err;
> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	if (err)
>  		return err;
>  
> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
> +
>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>  }
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 138961d7ebe3..776c2757a01e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
> +{
> +	int i;
> +
> +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
> +		if (vcpu->arch.idregs.regs[i].sys_id == id)
> +			return &vcpu->arch.idregs.regs[i];

With a derived index we don't need to search. Just do

 if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 ||
     sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0)
      return NULL;

 return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; 
 

> +	}
> +	return NULL;
> +}
> +
> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
> +{
> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> +
> +	BUG_ON(!ri);
> +	return ri->sys_val;
> +}
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
>  {
>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
>  
>  	if (id == SYS_ID_AA64PFR0_EL1) {
>  		if (!vcpu_has_sve(vcpu))
> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>   * are stored, and for set_id_reg() we don't allow the effective value
>   * to be changed.
>   */
> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
>  	return reg_to_user(uaddr, &val, id);
>  }
>  
> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f6d86033c4fa..1029444d04aa 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
>  	__s32	tablefd;
>  };
>  
> +#define ID_REG_MAX_NUMS 64
> +struct id_reg_info {
> +	uint64_t sys_id;
> +	uint64_t sys_val;

I'm not sure the 'sys_' prefix is necessary.

> +};
> +
> +struct id_registers {
> +	struct id_reg_info regs[ID_REG_MAX_NUMS];
> +	uint64_t num;
> +};
> +

This is arch specific, so there should be ARMv8 in the names.

>  /*
>   * ioctls for VM fds
>   */
> -- 
> 2.18.4
> 

Re: [RFC 3/4] kvm: arm64: make ID registers configurable

[Andrew Jones]

On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote:
> It's time to make ID registers configurable.  When userspace (but not
> guest) want to set the values of ID registers, save the value in
> kvm_arch_vcpu so that guest can read the modified values.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 776c2757a01e..f98635489966 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
>  	return ri->sys_val;
>  }
>  
> +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value)
> +{
> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> +
> +	BUG_ON(!ri);
> +	ri->sys_val = value;
> +}
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
>  static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
> @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  
>  /*
>   * cpufeature ID register user accessors
> - *
> - * For now, these registers are immutable for userspace, so no values
> - * are stored, and for set_id_reg() we don't allow the effective value
> - * to be changed.
>   */
>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
> @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>  	if (err)
>  		return err;
>  
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != read_id_reg(vcpu, rd, raz))
> -		return -EINVAL;
> +	if (raz) {
> +		if (val != read_id_reg(vcpu, rd, raz))
> +			return -EINVAL;
> +	} else {
> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +				     (u32)rd->CRm, (u32)rd->Op2);
> +		kvm_set_id_reg(vcpu, reg_id, val);

So userspace can set the ID registers to whatever they want? I think each
register should have its own sanity checks applied before accepting the
input.

Thanks,
drew

> +	}
>  
>  	return 0;
>  }
> -- 
> 2.18.4
> 

Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Andrew Jones]

On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote:
> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
> supports to set CPU features in AArch64.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/arm.c     | 1 +
>  include/uapi/linux/kvm.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 18ebbe1c64ee..72b9e8fc606f 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>  	case KVM_CAP_ARM_NISV_TO_USER:
>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
> +	case KVM_CAP_ARM_CPU_FEATURE:
>  		r = 1;
>  		break;
>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 1029444d04aa..0eca4f7c7fef 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
>  #define KVM_CAP_LAST_CPU 184
>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>  #define KVM_CAP_S390_DIAG318 186
> +#define KVM_CAP_ARM_CPU_FEATURE 187
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.18.4
>

All new caps should be documented in Documentation/virt/kvm/api.rst

Thanks,
drew 

Re: [RFC 0/4] kvm: arm64: emulate ID registers

[Andrew Jones]

On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote:
> In AArch64, guest will read the same values of the ID regsiters with
> host.  Both of them read the values from arm64_ftr_regs.  This patch
> series add support to emulate and configure ID registers so that we can
> control the value of ID registers that guest read.
> 
> Peng Liang (4):
>   arm64: add a helper function to traverse arm64_ftr_regs
>   kvm: arm64: emulate the ID registers
>   kvm: arm64: make ID registers configurable
>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> 
>  arch/arm64/include/asm/cpufeature.h |  2 ++
>  arch/arm64/include/asm/kvm_host.h   |  2 ++
>  arch/arm64/kernel/cpufeature.c      | 13 ++++++++
>  arch/arm64/kvm/arm.c                | 21 ++++++++++++
>  arch/arm64/kvm/sys_regs.c           | 50 ++++++++++++++++++++++-------
>  include/uapi/linux/kvm.h            | 12 +++++++
>  6 files changed, 89 insertions(+), 11 deletions(-)
> 
> -- 
> 2.18.4
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>

Hi Peng,

I'm glad to see an effort has started in trying to prepare for CPU models,
allowing migration beyond identical hosts. How have you tested this
series? I.e. what userspace did you use and with what additional patches?
KVM changes like these are also easily tested with KVM selftests[*]. Have
you considered posting a selftest?

[*] Linux repo: tools/testing/selftests/kvm/

Thanks,
drew

Re: [RFC 3/4] kvm: arm64: make ID registers configurable

[Marc Zyngier]

On 2020-08-13 07:05, Peng Liang wrote:
> It's time to make ID registers configurable.  When userspace (but not
> guest) want to set the values of ID registers, save the value in
> kvm_arch_vcpu so that guest can read the modified values.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 776c2757a01e..f98635489966 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, 
> u64 id)
>  	return ri->sys_val;
>  }
> 
> +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value)
> +{
> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> +
> +	BUG_ON(!ri);
> +	ri->sys_val = value;
> +}
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
>  static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
> @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu 
> *vcpu,
> 
>  /*
>   * cpufeature ID register user accessors
> - *
> - * For now, these registers are immutable for userspace, so no values
> - * are stored, and for set_id_reg() we don't allow the effective value
> - * to be changed.
>   */
>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
> @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>  	if (err)
>  		return err;
> 
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != read_id_reg(vcpu, rd, raz))
> -		return -EINVAL;
> +	if (raz) {
> +		if (val != read_id_reg(vcpu, rd, raz))
> +			return -EINVAL;
> +	} else {
> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
> +				     (u32)rd->CRm, (u32)rd->Op2);
> +		kvm_set_id_reg(vcpu, reg_id, val);
> +	}
> 
>  	return 0;
>  }

This cannot work. If userspace can override an idreg, it cannot
conflict with anything the HW is capable of. It also cannot
conflict with features that the host doesn't want to expose
to the guest.

Another thing is that you now have features that do not
match the MIDR (you can describe an A57 with SVE, for example).
This will trigger issues in guests, as the combination isn't expected.

And then there is the eternal story about errata workarounds.
If you can override the ID regs, how can the guest mitigate
errata that you are now hiding from it?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Andrew Jones]

On Thu, Aug 13, 2020 at 11:05:58AM +0200, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote:
> > To emulate the ID registers, we need a place to storage the values of
> > the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
> > 
> > This commit has no functional changes but only code refactor.  When
> > initializing a vcpu, get the values of the ID registers from
> > arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
> > the value from kvm_arch_vcpu when getting/setting the value of the ID
> > regs.
> > 
> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> > Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  2 ++
> >  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
> >  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
> >  include/uapi/linux/kvm.h          | 11 +++++++++++
> >  4 files changed, 56 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index f81151ad3d3c..7f7bd36702f7 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
> >  		u64 last_steal;
> >  		gpa_t base;
> >  	} steal;
> > +
> > +	struct id_registers idregs;
> >  };
> >  
> >  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 73e12869afe3..18ebbe1c64ee 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
> >  	return 0;
> >  }
> >  
> > +static int get_cpu_ftr(u32 id, u64 val, void *argp)
> > +{
> > +	struct id_registers *idregs = argp;
> > +
> > +	/*
> > +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> > +	 * where 1<=crm<8, 0<=op2<8.
> > +	 */
> > +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> > +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
> > +		idregs->regs[idregs->num].sys_id = id;
> > +		idregs->regs[idregs->num].sys_val = val;
> > +		idregs->num++;
> 
> This num++ means we should ensure get_cpu_ftr() is only used once per
> VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >  {
> >  	int err;
> > @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >  	if (err)
> >  		return err;
> >  
> > +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
> > +
> >  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
> >  }
> >  
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 138961d7ebe3..776c2757a01e 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> >  	return true;
> >  }
> >  
> > +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
> > +		if (vcpu->arch.idregs.regs[i].sys_id == id)
> > +			return &vcpu->arch.idregs.regs[i];
> 
> With a derived index we don't need to search. Just do
> 
>  if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 ||
>      sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0)
>       return NULL;
> 
>  return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; 
>  
> 
> > +	}
> > +	return NULL;
> > +}
> > +
> > +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
> > +{
> > +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> > +
> > +	BUG_ON(!ri);
> > +	return ri->sys_val;
> > +}
> > +
> >  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> > -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > +static u64 read_id_reg(struct kvm_vcpu *vcpu,
> >  		struct sys_reg_desc const *r, bool raz)
> >  {
> >  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> >  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> > -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> > +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
> >  
> >  	if (id == SYS_ID_AA64PFR0_EL1) {
> >  		if (!vcpu_has_sve(vcpu))
> > @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >   * are stored, and for set_id_reg() we don't allow the effective value
> >   * to be changed.
> >   */
> > -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> > +static int __get_id_reg(struct kvm_vcpu *vcpu,
> >  			const struct sys_reg_desc *rd, void __user *uaddr,
> >  			bool raz)
> >  {
> > @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
> >  	return reg_to_user(uaddr, &val, id);
> >  }
> >  
> > -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> > +static int __set_id_reg(struct kvm_vcpu *vcpu,
> >  			const struct sys_reg_desc *rd, void __user *uaddr,
> >  			bool raz)
> >  {
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index f6d86033c4fa..1029444d04aa 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
> >  	__s32	tablefd;
> >  };
> >  
> > +#define ID_REG_MAX_NUMS 64
> > +struct id_reg_info {
> > +	uint64_t sys_id;
> > +	uint64_t sys_val;
> 
> I'm not sure the 'sys_' prefix is necessary.
> 
> > +};
> > +
> > +struct id_registers {
> > +	struct id_reg_info regs[ID_REG_MAX_NUMS];
> > +	uint64_t num;
> > +};
> > +
> 
> This is arch specific, so there should be ARMv8 in the names.

Also, why are id_reg_info and id_registers UAPI?

Thanks,
drew

> 
> >  /*
> >   * ioctls for VM fds
> >   */
> > -- 
> > 2.18.4
> > 
> 

Re: [RFC 0/4] kvm: arm64: emulate ID registers

[Andrew Jones]

On Thu, Aug 13, 2020 at 02:05:13PM +0800, Peng Liang wrote:
> In AArch64, guest will read the same values of the ID regsiters with
> host.  Both of them read the values from arm64_ftr_regs.  This patch
> series add support to emulate and configure ID registers so that we can
> control the value of ID registers that guest read.
> 
> Peng Liang (4):
>   arm64: add a helper function to traverse arm64_ftr_regs
>   kvm: arm64: emulate the ID registers
>   kvm: arm64: make ID registers configurable
>   kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension
> 
>  arch/arm64/include/asm/cpufeature.h |  2 ++
>  arch/arm64/include/asm/kvm_host.h   |  2 ++
>  arch/arm64/kernel/cpufeature.c      | 13 ++++++++
>  arch/arm64/kvm/arm.c                | 21 ++++++++++++
>  arch/arm64/kvm/sys_regs.c           | 50 ++++++++++++++++++++++-------
>  include/uapi/linux/kvm.h            | 12 +++++++
>  6 files changed, 89 insertions(+), 11 deletions(-)
> 
> -- 
> 2.18.4
> 

Hi Peng,

After having looked at the QEMU series I don't believe this is the right
approach to CPU models. As stated in my reply [*] this approach appears
to just be shifting the problem wholesale to the user. On the KVM side,
I think we should start by auditing all the ID registers to see what
should be masked and what should be under user control. Then, we need
to consider what MIDR a guest that can migrate between hosts of different
MIDRs should have. And that'll require addressing the errata problem in
one way or another.

[*] https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg02587.html

Thanks,
drew

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Peng Liang]

On 8/13/2020 5:05 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote:
>> To emulate the ID registers, we need a place to storage the values of
>> the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
>>
>> This commit has no functional changes but only code refactor.  When
>> initializing a vcpu, get the values of the ID registers from
>> arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
>> the value from kvm_arch_vcpu when getting/setting the value of the ID
>> regs.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  2 ++
>>  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
>>  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
>>  include/uapi/linux/kvm.h          | 11 +++++++++++
>>  4 files changed, 56 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index f81151ad3d3c..7f7bd36702f7 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
>>  		u64 last_steal;
>>  		gpa_t base;
>>  	} steal;
>> +
>> +	struct id_registers idregs;
>>  };
>>  
>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 73e12869afe3..18ebbe1c64ee 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
>>  	return 0;
>>  }
>>  
>> +static int get_cpu_ftr(u32 id, u64 val, void *argp)
>> +{
>> +	struct id_registers *idregs = argp;
>> +
>> +	/*
>> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
>> +	 * where 1<=crm<8, 0<=op2<8.
>> +	 */
>> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
>> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
>> +		idregs->regs[idregs->num].sys_id = id;
>> +		idregs->regs[idregs->num].sys_val = val;
>> +		idregs->num++;
> 
> This num++ means we should ensure get_cpu_ftr() is only used once per
> VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>  {
>>  	int err;
>> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>  	if (err)
>>  		return err;
>>  
>> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
>> +
>>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>>  }
>>  
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 138961d7ebe3..776c2757a01e 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>>  	return true;
>>  }
>>  
>> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
>> +		if (vcpu->arch.idregs.regs[i].sys_id == id)
>> +			return &vcpu->arch.idregs.regs[i];
> 
> With a derived index we don't need to search. Just do
> 
>  if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 ||
>      sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0)
>       return NULL;
> 
>  return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; 
>  
> 

Thank you for your suggestions.

>> +	}
>> +	return NULL;
>> +}
>> +
>> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
>> +{
>> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
>> +
>> +	BUG_ON(!ri);
>> +	return ri->sys_val;
>> +}
>> +
>>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>>  		struct sys_reg_desc const *r, bool raz)
>>  {
>>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
>> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
>> +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
>>  
>>  	if (id == SYS_ID_AA64PFR0_EL1) {
>>  		if (!vcpu_has_sve(vcpu))
>> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>>   * are stored, and for set_id_reg() we don't allow the effective value
>>   * to be changed.
>>   */
>> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
>> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>>  			bool raz)
>>  {
>> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
>>  	return reg_to_user(uaddr, &val, id);
>>  }
>>  
>> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
>> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>>  			bool raz)
>>  {
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index f6d86033c4fa..1029444d04aa 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
>>  	__s32	tablefd;
>>  };
>>  
>> +#define ID_REG_MAX_NUMS 64
>> +struct id_reg_info {
>> +	uint64_t sys_id;
>> +	uint64_t sys_val;
> 
> I'm not sure the 'sys_' prefix is necessary.
> 
>> +};
>> +
>> +struct id_registers {
>> +	struct id_reg_info regs[ID_REG_MAX_NUMS];
>> +	uint64_t num;
>> +};
>> +
> 
> This is arch specific, so there should be ARMv8 in the names.

Some names are not very suitable, I'll change them.

> 
>>  /*
>>   * ioctls for VM fds
>>   */
>> -- 
>> 2.18.4
>>
> 
> .
> 

Re: [RFC 4/4] kvm: arm64: add KVM_CAP_ARM_CPU_FEATURE extension

[Peng Liang]

On 8/13/2020 5:10 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 02:05:17PM +0800, Peng Liang wrote:
>> Add KVM_CAP_ARM_CPU_FEATURE extension for userpace to check whether KVM
>> supports to set CPU features in AArch64.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/arm.c     | 1 +
>>  include/uapi/linux/kvm.h | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 18ebbe1c64ee..72b9e8fc606f 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -194,6 +194,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_ARM_IRQ_LINE_LAYOUT_2:
>>  	case KVM_CAP_ARM_NISV_TO_USER:
>>  	case KVM_CAP_ARM_INJECT_EXT_DABT:
>> +	case KVM_CAP_ARM_CPU_FEATURE:
>>  		r = 1;
>>  		break;
>>  	case KVM_CAP_ARM_SET_DEVICE_ADDR:
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 1029444d04aa..0eca4f7c7fef 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1035,6 +1035,7 @@ struct kvm_ppc_resize_hpt {
>>  #define KVM_CAP_LAST_CPU 184
>>  #define KVM_CAP_SMALLER_MAXPHYADDR 185
>>  #define KVM_CAP_S390_DIAG318 186
>> +#define KVM_CAP_ARM_CPU_FEATURE 187
>>  
>>  #ifdef KVM_CAP_IRQ_ROUTING
>>  
>> -- 
>> 2.18.4
>>
> 
> All new caps should be documented in Documentation/virt/kvm/api.rst
> 
> Thanks,
> drew 
> 
> .
> 
Sorry, I'll document it.

Thanks,
Peng

Re: [RFC 3/4] kvm: arm64: make ID registers configurable

[Peng Liang]

On 8/13/2020 5:09 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 02:05:16PM +0800, Peng Liang wrote:
>> It's time to make ID registers configurable.  When userspace (but not
>> guest) want to set the values of ID registers, save the value in
>> kvm_arch_vcpu so that guest can read the modified values.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 776c2757a01e..f98635489966 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1111,6 +1111,14 @@ static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
>>  	return ri->sys_val;
>>  }
>>  
>> +static void kvm_set_id_reg(struct kvm_vcpu *vcpu, u64 id, u64 value)
>> +{
>> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
>> +
>> +	BUG_ON(!ri);
>> +	ri->sys_val = value;
>> +}
>> +
>>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
>>  static u64 read_id_reg(struct kvm_vcpu *vcpu,
>>  		struct sys_reg_desc const *r, bool raz)
>> @@ -1252,10 +1260,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>>  
>>  /*
>>   * cpufeature ID register user accessors
>> - *
>> - * For now, these registers are immutable for userspace, so no values
>> - * are stored, and for set_id_reg() we don't allow the effective value
>> - * to be changed.
>>   */
>>  static int __get_id_reg(struct kvm_vcpu *vcpu,
>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>> @@ -1279,9 +1283,14 @@ static int __set_id_reg(struct kvm_vcpu *vcpu,
>>  	if (err)
>>  		return err;
>>  
>> -	/* This is what we mean by invariant: you can't change it. */
>> -	if (val != read_id_reg(vcpu, rd, raz))
>> -		return -EINVAL;
>> +	if (raz) {
>> +		if (val != read_id_reg(vcpu, rd, raz))
>> +			return -EINVAL;
>> +	} else {
>> +		u32 reg_id = sys_reg((u32)rd->Op0, (u32)rd->Op1, (u32)rd->CRn,
>> +				     (u32)rd->CRm, (u32)rd->Op2);
>> +		kvm_set_id_reg(vcpu, reg_id, val);
> 
> So userspace can set the ID registers to whatever they want? I think each
> register should have its own sanity checks applied before accepting the
> input.
> 
> Thanks,
> drew
> 
>> +	}
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.18.4
>>
> 
> .
> 

Yea, sanity checkers are necessary and I'm working on it.  I think we should make
sure that every ID fields should be checked to match the HW capabilities so that
guest will not be confused because of a careless hypervisor.

Thanks,
Peng

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Peng Liang]

On 8/13/2020 6:02 PM, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 11:05:58AM +0200, Andrew Jones wrote:
>> On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote:
>>> To emulate the ID registers, we need a place to storage the values of
>>> the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
>>>
>>> This commit has no functional changes but only code refactor.  When
>>> initializing a vcpu, get the values of the ID registers from
>>> arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
>>> the value from kvm_arch_vcpu when getting/setting the value of the ID
>>> regs.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  2 ++
>>>  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
>>>  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
>>>  include/uapi/linux/kvm.h          | 11 +++++++++++
>>>  4 files changed, 56 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index f81151ad3d3c..7f7bd36702f7 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
>>>  		u64 last_steal;
>>>  		gpa_t base;
>>>  	} steal;
>>> +
>>> +	struct id_registers idregs;
>>>  };
>>>  
>>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index 73e12869afe3..18ebbe1c64ee 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
>>>  	return 0;
>>>  }
>>>  
>>> +static int get_cpu_ftr(u32 id, u64 val, void *argp)
>>> +{
>>> +	struct id_registers *idregs = argp;
>>> +
>>> +	/*
>>> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
>>> +	 * where 1<=crm<8, 0<=op2<8.
>>> +	 */
>>> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
>>> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
>>> +		idregs->regs[idregs->num].sys_id = id;
>>> +		idregs->regs[idregs->num].sys_val = val;
>>> +		idregs->num++;
>>
>> This num++ means we should ensure get_cpu_ftr() is only used once per
>> VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2
>>
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>>  {
>>>  	int err;
>>> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
>>> +
>>>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>>>  }
>>>  
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 138961d7ebe3..776c2757a01e 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
>>>  	return true;
>>>  }
>>>  
>>> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
>>> +		if (vcpu->arch.idregs.regs[i].sys_id == id)
>>> +			return &vcpu->arch.idregs.regs[i];
>>
>> With a derived index we don't need to search. Just do
>>
>>  if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 ||
>>      sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0)
>>       return NULL;
>>
>>  return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; 
>>  
>>
>>> +	}
>>> +	return NULL;
>>> +}
>>> +
>>> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
>>> +{
>>> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
>>> +
>>> +	BUG_ON(!ri);
>>> +	return ri->sys_val;
>>> +}
>>> +
>>>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
>>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>>>  		struct sys_reg_desc const *r, bool raz)
>>>  {
>>>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>>>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
>>> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
>>> +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
>>>  
>>>  	if (id == SYS_ID_AA64PFR0_EL1) {
>>>  		if (!vcpu_has_sve(vcpu))
>>> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>>>   * are stored, and for set_id_reg() we don't allow the effective value
>>>   * to be changed.
>>>   */
>>> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
>>> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>>>  			bool raz)
>>>  {
>>> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
>>>  	return reg_to_user(uaddr, &val, id);
>>>  }
>>>  
>>> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
>>> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>>>  			const struct sys_reg_desc *rd, void __user *uaddr,
>>>  			bool raz)
>>>  {
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index f6d86033c4fa..1029444d04aa 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
>>>  	__s32	tablefd;
>>>  };
>>>  
>>> +#define ID_REG_MAX_NUMS 64
>>> +struct id_reg_info {
>>> +	uint64_t sys_id;
>>> +	uint64_t sys_val;
>>
>> I'm not sure the 'sys_' prefix is necessary.
>>
>>> +};
>>> +
>>> +struct id_registers {
>>> +	struct id_reg_info regs[ID_REG_MAX_NUMS];
>>> +	uint64_t num;
>>> +};
>>> +
>>
>> This is arch specific, so there should be ARMv8 in the names.
> 
> Also, why are id_reg_info and id_registers UAPI?
> 
> Thanks,
> drew
> 

id_reg_info is for information of an ID register, and id_registers contains
all the ID registers.

Thanks,
Peng

>>
>>>  /*
>>>   * ioctls for VM fds
>>>   */
>>> -- 
>>> 2.18.4
>>>
>>
> 
> .
> 

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Marc Zyngier]

On 2020-08-13 07:05, Peng Liang wrote:
> To emulate the ID registers, we need a place to storage the values of
> the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
> 
> This commit has no functional changes but only code refactor.  When
> initializing a vcpu, get the values of the ID registers from
> arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
> the value from kvm_arch_vcpu when getting/setting the value of the ID
> regs.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 ++
>  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
>  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
>  include/uapi/linux/kvm.h          | 11 +++++++++++
>  4 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index f81151ad3d3c..7f7bd36702f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
>  		u64 last_steal;
>  		gpa_t base;
>  	} steal;
> +
> +	struct id_registers idregs;

System registers are to be stored in the sysreg file. I've already
spent enough time moving them out of the various subsystems.

>  };
> 
>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 73e12869afe3..18ebbe1c64ee 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm,
> unsigned int id)
>  	return 0;
>  }
> 
> +static int get_cpu_ftr(u32 id, u64 val, void *argp)
> +{
> +	struct id_registers *idregs = argp;
> +
> +	/*
> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> +	 * where 1<=crm<8, 0<=op2<8.
> +	 */
> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
> +		idregs->regs[idregs->num].sys_id = id;
> +		idregs->regs[idregs->num].sys_val = val;
> +		idregs->num++;
> +	}
> +
> +	return 0;
> +}
> +
>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  {
>  	int err;
> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	if (err)
>  		return err;
> 
> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
> +
>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
>  }
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 138961d7ebe3..776c2757a01e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu 
> *vcpu,
>  	return true;
>  }
> 
> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
> +{
> +	int i;
> +
> +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
> +		if (vcpu->arch.idregs.regs[i].sys_id == id)
> +			return &vcpu->arch.idregs.regs[i];
> +	}
> +	return NULL;
> +}
> +
> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
> +{
> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> +
> +	BUG_ON(!ri);
> +	return ri->sys_val;
> +}
> +
>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
>  		struct sys_reg_desc const *r, bool raz)
>  {
>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
> 
>  	if (id == SYS_ID_AA64PFR0_EL1) {
>  		if (!vcpu_has_sve(vcpu))
> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu 
> *vcpu,
>   * are stored, and for set_id_reg() we don't allow the effective value
>   * to be changed.
>   */
> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> +static int __get_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu 
> *vcpu,
>  	return reg_to_user(uaddr, &val, id);
>  }
> 
> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> +static int __set_id_reg(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_desc *rd, void __user *uaddr,
>  			bool raz)
>  {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index f6d86033c4fa..1029444d04aa 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
>  	__s32	tablefd;
>  };
> 
> +#define ID_REG_MAX_NUMS 64
> +struct id_reg_info {
> +	uint64_t sys_id;
> +	uint64_t sys_val;
> +};
> +
> +struct id_registers {
> +	struct id_reg_info regs[ID_REG_MAX_NUMS];
> +	uint64_t num;
> +};
> +
>  /*
>   * ioctls for VM fds
>   */

No way this is an acceptable interface. We have the one-reg interface,
which takes a system register encoding.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 2/4] kvm: arm64: emulate the ID registers

[Andrew Jones]

On Fri, Aug 14, 2020 at 07:49:43PM +0800, Peng Liang wrote:
> On 8/13/2020 6:02 PM, Andrew Jones wrote:
> > On Thu, Aug 13, 2020 at 11:05:58AM +0200, Andrew Jones wrote:
> >> On Thu, Aug 13, 2020 at 02:05:15PM +0800, Peng Liang wrote:
> >>> To emulate the ID registers, we need a place to storage the values of
> >>> the ID regsiters.  Maybe putting in kvm_arch_vcpu is a good idea.
> >>>
> >>> This commit has no functional changes but only code refactor.  When
> >>> initializing a vcpu, get the values of the ID registers from
> >>> arm64_ftr_regs and storage them in kvm_arch_vcpu.  And we just read
> >>> the value from kvm_arch_vcpu when getting/setting the value of the ID
> >>> regs.
> >>>
> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> >>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> >>> ---
> >>>  arch/arm64/include/asm/kvm_host.h |  2 ++
> >>>  arch/arm64/kvm/arm.c              | 20 ++++++++++++++++++++
> >>>  arch/arm64/kvm/sys_regs.c         | 27 +++++++++++++++++++++++----
> >>>  include/uapi/linux/kvm.h          | 11 +++++++++++
> >>>  4 files changed, 56 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>> index f81151ad3d3c..7f7bd36702f7 100644
> >>> --- a/arch/arm64/include/asm/kvm_host.h
> >>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>> @@ -336,6 +336,8 @@ struct kvm_vcpu_arch {
> >>>  		u64 last_steal;
> >>>  		gpa_t base;
> >>>  	} steal;
> >>> +
> >>> +	struct id_registers idregs;
> >>>  };
> >>>  
> >>>  /* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
> >>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> >>> index 73e12869afe3..18ebbe1c64ee 100644
> >>> --- a/arch/arm64/kvm/arm.c
> >>> +++ b/arch/arm64/kvm/arm.c
> >>> @@ -262,6 +262,24 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static int get_cpu_ftr(u32 id, u64 val, void *argp)
> >>> +{
> >>> +	struct id_registers *idregs = argp;
> >>> +
> >>> +	/*
> >>> +	 * (Op0, Op1, CRn, CRm, Op2) of ID registers is (3, 0, 0, crm, op2),
> >>> +	 * where 1<=crm<8, 0<=op2<8.
> >>> +	 */
> >>> +	if (sys_reg_Op0(id) == 3 && sys_reg_Op1(id) == 0 &&
> >>> +	    sys_reg_CRn(id) == 0 && sys_reg_CRm(id) > 0) {
> >>> +		idregs->regs[idregs->num].sys_id = id;
> >>> +		idregs->regs[idregs->num].sys_val = val;
> >>> +		idregs->num++;
> >>
> >> This num++ means we should ensure get_cpu_ftr() is only used once per
> >> VCPU, but we don't need 'num'. The index can be derived: (crm<<3)|op2
> >>
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >>>  {
> >>>  	int err;
> >>> @@ -285,6 +303,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
> >>>  	if (err)
> >>>  		return err;
> >>>  
> >>> +	arm64_cpu_ftr_regs_traverse(get_cpu_ftr, &vcpu->arch.idregs);
> >>> +
> >>>  	return create_hyp_mappings(vcpu, vcpu + 1, PAGE_HYP);
> >>>  }
> >>>  
> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >>> index 138961d7ebe3..776c2757a01e 100644
> >>> --- a/arch/arm64/kvm/sys_regs.c
> >>> +++ b/arch/arm64/kvm/sys_regs.c
> >>> @@ -1092,13 +1092,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> >>>  	return true;
> >>>  }
> >>>  
> >>> +static struct id_reg_info *kvm_id_reg(struct kvm_vcpu *vcpu, u64 id)
> >>> +{
> >>> +	int i;
> >>> +
> >>> +	for (i = 0; i < vcpu->arch.idregs.num; ++i) {
> >>> +		if (vcpu->arch.idregs.regs[i].sys_id == id)
> >>> +			return &vcpu->arch.idregs.regs[i];
> >>
> >> With a derived index we don't need to search. Just do
> >>
> >>  if (sys_reg_Op0(id) != 3 || sys_reg_Op1(id) != 0 ||
> >>      sys_reg_CRn(id) != 0 || sys_reg_CRm(id) == 0)
> >>       return NULL;
> >>
> >>  return &vcpu->arch.idregs.regs[(sys_reg_CRm(id)<<3) | sys_reg_Op2(id)]; 
> >>  
> >>
> >>> +	}
> >>> +	return NULL;
> >>> +}
> >>> +
> >>> +static u64 kvm_get_id_reg(struct kvm_vcpu *vcpu, u64 id)
> >>> +{
> >>> +	struct id_reg_info *ri = kvm_id_reg(vcpu, id);
> >>> +
> >>> +	BUG_ON(!ri);
> >>> +	return ri->sys_val;
> >>> +}
> >>> +
> >>>  /* Read a sanitised cpufeature ID register by sys_reg_desc */
> >>> -static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> >>> +static u64 read_id_reg(struct kvm_vcpu *vcpu,
> >>>  		struct sys_reg_desc const *r, bool raz)
> >>>  {
> >>>  	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> >>>  			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> >>> -	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
> >>> +	u64 val = raz ? 0 : kvm_get_id_reg(vcpu, id);
> >>>  
> >>>  	if (id == SYS_ID_AA64PFR0_EL1) {
> >>>  		if (!vcpu_has_sve(vcpu))
> >>> @@ -1238,7 +1257,7 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >>>   * are stored, and for set_id_reg() we don't allow the effective value
> >>>   * to be changed.
> >>>   */
> >>> -static int __get_id_reg(const struct kvm_vcpu *vcpu,
> >>> +static int __get_id_reg(struct kvm_vcpu *vcpu,
> >>>  			const struct sys_reg_desc *rd, void __user *uaddr,
> >>>  			bool raz)
> >>>  {
> >>> @@ -1248,7 +1267,7 @@ static int __get_id_reg(const struct kvm_vcpu *vcpu,
> >>>  	return reg_to_user(uaddr, &val, id);
> >>>  }
> >>>  
> >>> -static int __set_id_reg(const struct kvm_vcpu *vcpu,
> >>> +static int __set_id_reg(struct kvm_vcpu *vcpu,
> >>>  			const struct sys_reg_desc *rd, void __user *uaddr,
> >>>  			bool raz)
> >>>  {
> >>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >>> index f6d86033c4fa..1029444d04aa 100644
> >>> --- a/include/uapi/linux/kvm.h
> >>> +++ b/include/uapi/linux/kvm.h
> >>> @@ -1272,6 +1272,17 @@ struct kvm_vfio_spapr_tce {
> >>>  	__s32	tablefd;
> >>>  };
> >>>  
> >>> +#define ID_REG_MAX_NUMS 64
> >>> +struct id_reg_info {
> >>> +	uint64_t sys_id;
> >>> +	uint64_t sys_val;
> >>
> >> I'm not sure the 'sys_' prefix is necessary.
> >>
> >>> +};
> >>> +
> >>> +struct id_registers {
> >>> +	struct id_reg_info regs[ID_REG_MAX_NUMS];
> >>> +	uint64_t num;
> >>> +};
> >>> +
> >>
> >> This is arch specific, so there should be ARMv8 in the names.
> > 
> > Also, why are id_reg_info and id_registers UAPI?
> > 
> > Thanks,
> > drew
> > 
> 
> id_reg_info is for information of an ID register, and id_registers contains
> all the ID registers.

Obviously. But why is that UAPI? What user interface is using them?

> 
> Thanks,
> Peng
> 
> >>
> >>>  /*
> >>>   * ioctls for VM fds
> >>>   */
> >>> -- 
> >>> 2.18.4
> >>>
> >>
> > 
> > .
> > 
>