Linux KVM/arm64 development list
 help / color / mirror / Atom feed
* [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression
@ 2020-11-02 18:50 Andrew Jones
  2020-11-02 18:50 ` [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Andrew Jones @ 2020-11-02 18:50 UTC (permalink / raw)
  To: kvmarm; +Cc: maz, xu910121, Dave.Martin

张东旭 <xu910121@sina.com> reported a regression seen with CentOS
when migrating from an old kernel to a new one. The problem was
that QEMU rejected the migration since KVM_GET_REG_LIST reported
a register was missing on the destination. Extra registers are OK
on the destination, but not missing ones. The regression reproduces
with upstream kernels when migrating from a 4.15 or later kernel,
up to one with commit 73433762fcae ("KVM: arm64/sve: System register
context switch and access support"), to a kernel that includes that
commit, e.g. the latest mainline (5.10-rc2).

The first patch of this series is the fix. The next two patches,
which don't have any intended functional changes, allow ID_SANITISED
to be used for registers that flip between exposing features and
being RAZ, which allows some code to be removed.

v2:
 - CC stable [Marc]
 - Only one RAZ flag is enough [Marc]
 - Move id_visibility() up by read_id_reg() since they'll likely
   be maintained together [drew]

Andrew Jones (3):
  KVM: arm64: Don't hide ID registers from userspace
  KVM: arm64: Check RAZ visibility in ID register accessors
  KVM: arm64: Remove AA64ZFR0_EL1 accessors

 arch/arm64/kvm/sys_regs.c | 96 +++++++++++----------------------------
 arch/arm64/kvm/sys_regs.h | 10 ++++
 2 files changed, 37 insertions(+), 69 deletions(-)

-- 
2.26.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace
  2020-11-02 18:50 [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
@ 2020-11-02 18:50 ` Andrew Jones
  2020-11-03 11:18   ` Dave Martin
  2020-11-02 18:50 ` [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-02 18:50 UTC (permalink / raw)
  To: kvmarm; +Cc: maz, xu910121, Dave.Martin, stable

ID registers are RAZ until they've been allocated a purpose, but
that doesn't mean they should be removed from the KVM_GET_REG_LIST
list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
is hidden from userspace when its function is not present. Removing
the userspace visibility checks is enough to reexpose it, as it
already behaves as RAZ when the function is not present.

Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
Cc: <stable@vger.kernel.org> # v5.2+
Reported-by: 张东旭 <xu910121@sina.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index fb12d3ef423a..6ff0c15531ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
 }
 
-/* Visibility overrides for SVE-specific ID registers */
-static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
-				      const struct sys_reg_desc *rd)
-{
-	if (vcpu_has_sve(vcpu))
-		return 0;
-
-	return REG_HIDDEN_USER;
-}
-
 /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
 static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
 {
@@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val;
 
-	if (WARN_ON(!vcpu_has_sve(vcpu)))
-		return -ENOENT;
-
 	val = guest_id_aa64zfr0_el1(vcpu);
 	return reg_to_user(uaddr, &val, reg->id);
 }
@@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 	int err;
 	u64 val;
 
-	if (WARN_ON(!vcpu_has_sve(vcpu)))
-		return -ENOENT;
-
 	err = reg_from_user(&val, uaddr, id);
 	if (err)
 		return err;
@@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
+	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
 	ID_UNALLOCATED(4,5),
 	ID_UNALLOCATED(4,6),
 	ID_UNALLOCATED(4,7),
-- 
2.26.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
  2020-11-02 18:50 [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
  2020-11-02 18:50 ` [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
@ 2020-11-02 18:50 ` Andrew Jones
  2020-11-03 11:23   ` Dave Martin
  2020-11-02 18:50 ` [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
  2020-11-03 11:37 ` [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Dave Martin
  3 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-02 18:50 UTC (permalink / raw)
  To: kvmarm; +Cc: maz, xu910121, Dave.Martin

The instruction encodings of ID registers are preallocated. Until an
encoding is assigned a purpose the register is RAZ. KVM's general ID
register accessor functions already support both paths, RAZ or not.
If for each ID register we can determine if it's RAZ or not, then all
ID registers can build on the general functions. The register visibility
function allows us to check whether a register should be completely
hidden or not, extending it to also report when the register should
be RAZ or not allows us to use it for ID registers as well.

No functional change intended.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
 arch/arm64/kvm/sys_regs.h | 10 ++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 6ff0c15531ca..b8822a20b1ea 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1153,6 +1153,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 	return val;
 }
 
+static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
+				  const struct sys_reg_desc *r)
+{
+	return 0;
+}
+
 /* cpufeature ID register access trap handlers */
 
 static bool __access_id_reg(struct kvm_vcpu *vcpu,
@@ -1171,7 +1177,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
 			  struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	return __access_id_reg(vcpu, p, r, false);
+	bool raz = sysreg_visible_as_raz(vcpu, r);
+
+	return __access_id_reg(vcpu, p, r, raz);
 }
 
 static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
@@ -1283,13 +1291,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
 static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	return __get_id_reg(vcpu, rd, uaddr, false);
+	bool raz = sysreg_visible_as_raz(vcpu, rd);
+
+	return __get_id_reg(vcpu, rd, uaddr, raz);
 }
 
 static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 		      const struct kvm_one_reg *reg, void __user *uaddr)
 {
-	return __set_id_reg(vcpu, rd, uaddr, false);
+	bool raz = sysreg_visible_as_raz(vcpu, rd);
+
+	return __set_id_reg(vcpu, rd, uaddr, raz);
 }
 
 static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
@@ -1381,6 +1393,7 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	.access	= access_id_reg,		\
 	.get_user = get_id_reg,			\
 	.set_user = set_id_reg,			\
+	.visibility = id_visibility,		\
 }
 
 /*
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 5a6fc30f5989..9d3ef7cfa116 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -61,6 +61,7 @@ struct sys_reg_desc {
 
 #define REG_HIDDEN_USER		(1 << 0) /* hidden from userspace ioctls */
 #define REG_HIDDEN_GUEST	(1 << 1) /* hidden from guest */
+#define REG_RAZ			(1 << 2) /* RAZ from userspace ioctls and guest */
 
 static __printf(2, 3)
 inline void print_sys_reg_msg(const struct sys_reg_params *p,
@@ -129,6 +130,15 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
 	return r->visibility(vcpu, r) & REG_HIDDEN_USER;
 }
 
+static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
+					 const struct sys_reg_desc *r)
+{
+	if (likely(!r->visibility))
+		return false;
+
+	return r->visibility(vcpu, r) & REG_RAZ;
+}
+
 static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
 			      const struct sys_reg_desc *i2)
 {
-- 
2.26.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
  2020-11-02 18:50 [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
  2020-11-02 18:50 ` [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
  2020-11-02 18:50 ` [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
@ 2020-11-02 18:50 ` Andrew Jones
  2020-11-03 11:32   ` Dave Martin
  2020-11-03 11:37 ` [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Dave Martin
  3 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-02 18:50 UTC (permalink / raw)
  To: kvmarm; +Cc: maz, xu910121, Dave.Martin

The AA64ZFR0_EL1 accessors are just the general accessors with
its visibility function open-coded. It also skips the if-else
chain in read_id_reg, but there's no reason not to go there.
Indeed consolidating ID register accessors and removing lines
of code make it worthwhile.

No functional change intended.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 61 +++++++--------------------------------
 1 file changed, 11 insertions(+), 50 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b8822a20b1ea..e2d6fb83280e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1156,6 +1156,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
 				  const struct sys_reg_desc *r)
 {
+	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
+			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
+
+	switch (id) {
+	case SYS_ID_AA64ZFR0_EL1:
+		if (!vcpu_has_sve(vcpu))
+			return REG_RAZ;
+		break;
+	}
+
 	return 0;
 }
 
@@ -1203,55 +1213,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
 }
 
-/* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
-static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
-{
-	if (!vcpu_has_sve(vcpu))
-		return 0;
-
-	return read_sanitised_ftr_reg(SYS_ID_AA64ZFR0_EL1);
-}
-
-static bool access_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
-				   struct sys_reg_params *p,
-				   const struct sys_reg_desc *rd)
-{
-	if (p->is_write)
-		return write_to_read_only(vcpu, p, rd);
-
-	p->regval = guest_id_aa64zfr0_el1(vcpu);
-	return true;
-}
-
-static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
-		const struct sys_reg_desc *rd,
-		const struct kvm_one_reg *reg, void __user *uaddr)
-{
-	u64 val;
-
-	val = guest_id_aa64zfr0_el1(vcpu);
-	return reg_to_user(uaddr, &val, reg->id);
-}
-
-static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
-		const struct sys_reg_desc *rd,
-		const struct kvm_one_reg *reg, void __user *uaddr)
-{
-	const u64 id = sys_reg_to_index(rd);
-	int err;
-	u64 val;
-
-	err = reg_from_user(&val, uaddr, id);
-	if (err)
-		return err;
-
-	/* This is what we mean by invariant: you can't change it. */
-	if (val != guest_id_aa64zfr0_el1(vcpu))
-		return -EINVAL;
-
-	return 0;
-}
-
 /*
  * cpufeature ID register user accessors
  *
@@ -1515,7 +1476,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
+	ID_SANITISED(ID_AA64ZFR0_EL1),
 	ID_UNALLOCATED(4,5),
 	ID_UNALLOCATED(4,6),
 	ID_UNALLOCATED(4,7),
-- 
2.26.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace
  2020-11-02 18:50 ` [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
@ 2020-11-03 11:18   ` Dave Martin
  2020-11-03 13:32     ` Andrew Jones
  0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2020-11-03 11:18 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm, stable

On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> ID registers are RAZ until they've been allocated a purpose, but
> that doesn't mean they should be removed from the KVM_GET_REG_LIST
> list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> is hidden from userspace when its function is not present. Removing
> the userspace visibility checks is enough to reexpose it, as it
> already behaves as RAZ when the function is not present.

Pleae state what the patch does.  (The subject line serves as a summary
of that, but the commit message should make sense without it.)

Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
RAZ with this change?  (I'm not saying it doesn't, but the code is not
trivial to follow...)

> 
> Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> Cc: <stable@vger.kernel.org> # v5.2+
> Reported-by: 张东旭 <xu910121@sina.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index fb12d3ef423a..6ff0c15531ca 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>  }
>  
> -/* Visibility overrides for SVE-specific ID registers */
> -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> -				      const struct sys_reg_desc *rd)
> -{
> -	if (vcpu_has_sve(vcpu))
> -		return 0;
> -
> -	return REG_HIDDEN_USER;

In light of this change, I think that REG_HIDDEN_GUEST and
REG_HIDDEN_USER are always either both set or both clear.  Given the
discussion on this issue, I'm thinking it probably doesn't even make
sense for these to be independent (?)

If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
simplifying the code appropriately.

(In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
was trying to achieve.)

> -}
> -
>  /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
>  static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
>  {
> @@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  {
>  	u64 val;
>  
> -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> -		return -ENOENT;
> -
>  	val = guest_id_aa64zfr0_el1(vcpu);
>  	return reg_to_user(uaddr, &val, reg->id);
>  }
> @@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  	int err;
>  	u64 val;
>  
> -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> -		return -ENOENT;
> -
>  	err = reg_from_user(&val, uaddr, id);
>  	if (err)
>  		return err;
> @@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
>  	ID_UNALLOCATED(4,5),
>  	ID_UNALLOCATED(4,6),
>  	ID_UNALLOCATED(4,7),

Otherwise looks reasonable.

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
  2020-11-02 18:50 ` [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
@ 2020-11-03 11:23   ` Dave Martin
  2020-11-03 13:38     ` Andrew Jones
  0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2020-11-03 11:23 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Mon, Nov 02, 2020 at 07:50:36PM +0100, Andrew Jones wrote:
> The instruction encodings of ID registers are preallocated. Until an
> encoding is assigned a purpose the register is RAZ. KVM's general ID
> register accessor functions already support both paths, RAZ or not.
> If for each ID register we can determine if it's RAZ or not, then all
> ID registers can build on the general functions. The register visibility
> function allows us to check whether a register should be completely
> hidden or not, extending it to also report when the register should
> be RAZ or not allows us to use it for ID registers as well.

Nit: no statement of what the patch does.

You might want to point out that the introduced REG_RAZ functionality is
intentionally not used in this patch.

> No functional change intended.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
>  arch/arm64/kvm/sys_regs.h | 10 ++++++++++
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 6ff0c15531ca..b8822a20b1ea 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1153,6 +1153,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  	return val;
>  }
>  
> +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> +				  const struct sys_reg_desc *r)
> +{
> +	return 0;
> +}
> +
>  /* cpufeature ID register access trap handlers */
>  
>  static bool __access_id_reg(struct kvm_vcpu *vcpu,
> @@ -1171,7 +1177,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
>  			  struct sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
> -	return __access_id_reg(vcpu, p, r, false);
> +	bool raz = sysreg_visible_as_raz(vcpu, r);
> +
> +	return __access_id_reg(vcpu, p, r, raz);
>  }
>  
>  static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> @@ -1283,13 +1291,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
>  static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	return __get_id_reg(vcpu, rd, uaddr, false);
> +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> +
> +	return __get_id_reg(vcpu, rd, uaddr, raz);
>  }
>  
>  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  		      const struct kvm_one_reg *reg, void __user *uaddr)
>  {
> -	return __set_id_reg(vcpu, rd, uaddr, false);
> +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> +
> +	return __set_id_reg(vcpu, rd, uaddr, raz);
>  }
>  
>  static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> @@ -1381,6 +1393,7 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	.access	= access_id_reg,		\
>  	.get_user = get_id_reg,			\
>  	.set_user = set_id_reg,			\
> +	.visibility = id_visibility,		\

This is just the default for ID_SANITISED, right?

>  }
>  
>  /*
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 5a6fc30f5989..9d3ef7cfa116 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -61,6 +61,7 @@ struct sys_reg_desc {
>  
>  #define REG_HIDDEN_USER		(1 << 0) /* hidden from userspace ioctls */
>  #define REG_HIDDEN_GUEST	(1 << 1) /* hidden from guest */
> +#define REG_RAZ			(1 << 2) /* RAZ from userspace ioctls and guest */
>  
>  static __printf(2, 3)
>  inline void print_sys_reg_msg(const struct sys_reg_params *p,
> @@ -129,6 +130,15 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
>  	return r->visibility(vcpu, r) & REG_HIDDEN_USER;
>  }
>  
> +static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
> +					 const struct sys_reg_desc *r)
> +{
> +	if (likely(!r->visibility))
> +		return false;
> +
> +	return r->visibility(vcpu, r) & REG_RAZ;
> +}
> +

[...]

Looks reasonable, I think.

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
  2020-11-02 18:50 ` [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
@ 2020-11-03 11:32   ` Dave Martin
  2020-11-03 13:46     ` Andrew Jones
  0 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2020-11-03 11:32 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Mon, Nov 02, 2020 at 07:50:37PM +0100, Andrew Jones wrote:
> The AA64ZFR0_EL1 accessors are just the general accessors with
> its visibility function open-coded. It also skips the if-else
> chain in read_id_reg, but there's no reason not to go there.
> Indeed consolidating ID register accessors and removing lines
> of code make it worthwhile.
> 
> No functional change intended.

Nit: No statement of what the patch does.

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 61 +++++++--------------------------------
>  1 file changed, 11 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index b8822a20b1ea..e2d6fb83280e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1156,6 +1156,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
>  				  const struct sys_reg_desc *r)
>  {
> +	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> +			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> +
> +	switch (id) {
> +	case SYS_ID_AA64ZFR0_EL1:
> +		if (!vcpu_has_sve(vcpu))
> +			return REG_RAZ;
> +		break;
> +	}
> +

This should work, but I'm not sure it's preferable to giving affected
registers their own visibility check function.

Multiplexing all the ID regs through this one checker function will
introduce a bit of overhead for always-non-RAZ ID regs, but I'd guess
the impact is negligible given the other overheads on these paths.

>  	return 0;
>  }
>  
> @@ -1203,55 +1213,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>  }
>  
> -/* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> -static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> -{
> -	if (!vcpu_has_sve(vcpu))
> -		return 0;
> -
> -	return read_sanitised_ftr_reg(SYS_ID_AA64ZFR0_EL1);
> -}
> -
> -static bool access_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -				   struct sys_reg_params *p,
> -				   const struct sys_reg_desc *rd)
> -{
> -	if (p->is_write)
> -		return write_to_read_only(vcpu, p, rd);
> -
> -	p->regval = guest_id_aa64zfr0_el1(vcpu);
> -	return true;
> -}
> -
> -static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -		const struct sys_reg_desc *rd,
> -		const struct kvm_one_reg *reg, void __user *uaddr)
> -{
> -	u64 val;
> -
> -	val = guest_id_aa64zfr0_el1(vcpu);
> -	return reg_to_user(uaddr, &val, reg->id);
> -}
> -
> -static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> -		const struct sys_reg_desc *rd,
> -		const struct kvm_one_reg *reg, void __user *uaddr)
> -{
> -	const u64 id = sys_reg_to_index(rd);
> -	int err;
> -	u64 val;
> -
> -	err = reg_from_user(&val, uaddr, id);
> -	if (err)
> -		return err;
> -
> -	/* This is what we mean by invariant: you can't change it. */
> -	if (val != guest_id_aa64zfr0_el1(vcpu))
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
>  /*
>   * cpufeature ID register user accessors
>   *
> @@ -1515,7 +1476,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> +	ID_SANITISED(ID_AA64ZFR0_EL1),

If keeping a dedicated helper, we could have a special macro for that, say

	ID_SANITISED_VISIBILITY(ID_AA64ZFR0_EL1, id_aa64zfr0_el1_visibility)

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression
  2020-11-02 18:50 [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
                   ` (2 preceding siblings ...)
  2020-11-02 18:50 ` [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
@ 2020-11-03 11:37 ` Dave Martin
  2020-11-03 13:52   ` Andrew Jones
  3 siblings, 1 reply; 16+ messages in thread
From: Dave Martin @ 2020-11-03 11:37 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Mon, Nov 02, 2020 at 07:50:34PM +0100, Andrew Jones wrote:
> 张东旭 <xu910121@sina.com> reported a regression seen with CentOS
> when migrating from an old kernel to a new one. The problem was
> that QEMU rejected the migration since KVM_GET_REG_LIST reported
> a register was missing on the destination. Extra registers are OK
> on the destination, but not missing ones. The regression reproduces
> with upstream kernels when migrating from a 4.15 or later kernel,
> up to one with commit 73433762fcae ("KVM: arm64/sve: System register
> context switch and access support"), to a kernel that includes that
> commit, e.g. the latest mainline (5.10-rc2).
> 
> The first patch of this series is the fix. The next two patches,
> which don't have any intended functional changes, allow ID_SANITISED
> to be used for registers that flip between exposing features and
> being RAZ, which allows some code to be removed.

Is it worth updating Documentation/virt/kvm/api.rst to clarify the
expected use during VM migrations, and the guarantees that are expected
to hold between migratable kernel versions?  Currently the specification
is a mixture of "surely it's obvious" and "whatever makes QEMU work".

I guess that caught me out, but I'll let others judge whether other
people are likely to get similarly confused.

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace
  2020-11-03 11:18   ` Dave Martin
@ 2020-11-03 13:32     ` Andrew Jones
  2020-11-04 16:11       ` Dave Martin
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-03 13:32 UTC (permalink / raw)
  To: Dave Martin; +Cc: maz, xu910121, kvmarm, stable

On Tue, Nov 03, 2020 at 11:18:19AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> > ID registers are RAZ until they've been allocated a purpose, but
> > that doesn't mean they should be removed from the KVM_GET_REG_LIST
> > list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> > is hidden from userspace when its function is not present. Removing
> > the userspace visibility checks is enough to reexpose it, as it
> > already behaves as RAZ when the function is not present.
> 
> Pleae state what the patch does.  (The subject line serves as a summary
> of that, but the commit message should make sense without it.)

I don't like "This patch ..." type of sentences in the commit message,
but unless you have a better suggestion, then I'd just add a sentence
like

"This patch ensures we still expose sysreg '3, 0, 0, 4, 4'
(ID_AA64ZFR0_EL1) to userspace as RAZ when SVE is not implemented."

> 
> Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
> RAZ with this change?  (I'm not saying it doesn't, but the code is not
> trivial to follow...)

guest_id_aa64zfr0_el1() returns zero for the register when !vcpu_has_sve(),
and all the accessors (userspace and guest) build on it.

I'm not sure how helpful it would be to add that sentence to the commit
message though.

> 
> > 
> > Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> > Cc: <stable@vger.kernel.org> # v5.2+
> > Reported-by: 张东旭 <xu910121@sina.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 18 +-----------------
> >  1 file changed, 1 insertion(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index fb12d3ef423a..6ff0c15531ca 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> >  }
> >  
> > -/* Visibility overrides for SVE-specific ID registers */
> > -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> > -				      const struct sys_reg_desc *rd)
> > -{
> > -	if (vcpu_has_sve(vcpu))
> > -		return 0;
> > -
> > -	return REG_HIDDEN_USER;
> 
> In light of this change, I think that REG_HIDDEN_GUEST and
> REG_HIDDEN_USER are always either both set or both clear.  Given the
> discussion on this issue, I'm thinking it probably doesn't even make
> sense for these to be independent (?)
> 
> If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
> simplifying the code appropriately.
> 
> (In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
> was trying to achieve.)

We could consolidate REG_HIDDEN_GUEST and REG_HIDDEN_USER into REG_HIDDEN,
which ZCR_EL1 and ptrauth registers will still use.

> 
> > -}
> > -
> >  /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> >  static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> >  {
> > @@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >  {
> >  	u64 val;
> >  
> > -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> > -		return -ENOENT;
> > -
> >  	val = guest_id_aa64zfr0_el1(vcpu);
> >  	return reg_to_user(uaddr, &val, reg->id);
> >  }
> > @@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >  	int err;
> >  	u64 val;
> >  
> > -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> > -		return -ENOENT;
> > -
> >  	err = reg_from_user(&val, uaddr, id);
> >  	if (err)
> >  		return err;
> > @@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	ID_SANITISED(ID_AA64PFR1_EL1),
> >  	ID_UNALLOCATED(4,2),
> >  	ID_UNALLOCATED(4,3),
> > -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> > +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> >  	ID_UNALLOCATED(4,5),
> >  	ID_UNALLOCATED(4,6),
> >  	ID_UNALLOCATED(4,7),
> 
> Otherwise looks reasonable.
>

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
  2020-11-03 11:23   ` Dave Martin
@ 2020-11-03 13:38     ` Andrew Jones
  2020-11-04 16:31       ` Dave Martin
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-03 13:38 UTC (permalink / raw)
  To: Dave Martin; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 11:23:54AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:36PM +0100, Andrew Jones wrote:
> > The instruction encodings of ID registers are preallocated. Until an
> > encoding is assigned a purpose the register is RAZ. KVM's general ID
> > register accessor functions already support both paths, RAZ or not.
> > If for each ID register we can determine if it's RAZ or not, then all
> > ID registers can build on the general functions. The register visibility
> > function allows us to check whether a register should be completely
> > hidden or not, extending it to also report when the register should
> > be RAZ or not allows us to use it for ID registers as well.
> 
> Nit: no statement of what the patch does.

Hmm, I'm not sure what "...extending it to also report when the register
should be RAZ or not allows us to use it for ID registers as well." is
missing, other than spelling out that a new flag is being added for the
extension. Please provide a suggestion.

> 
> You might want to point out that the introduced REG_RAZ functionality is
> intentionally not used in this patch.

OK

> 
> > No functional change intended.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
> >  arch/arm64/kvm/sys_regs.h | 10 ++++++++++
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 6ff0c15531ca..b8822a20b1ea 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1153,6 +1153,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> >  	return val;
> >  }
> >  
> > +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > +				  const struct sys_reg_desc *r)
> > +{
> > +	return 0;
> > +}
> > +
> >  /* cpufeature ID register access trap handlers */
> >  
> >  static bool __access_id_reg(struct kvm_vcpu *vcpu,
> > @@ -1171,7 +1177,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
> >  			  struct sys_reg_params *p,
> >  			  const struct sys_reg_desc *r)
> >  {
> > -	return __access_id_reg(vcpu, p, r, false);
> > +	bool raz = sysreg_visible_as_raz(vcpu, r);
> > +
> > +	return __access_id_reg(vcpu, p, r, raz);
> >  }
> >  
> >  static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > @@ -1283,13 +1291,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> >  static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >  		      const struct kvm_one_reg *reg, void __user *uaddr)
> >  {
> > -	return __get_id_reg(vcpu, rd, uaddr, false);
> > +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> > +
> > +	return __get_id_reg(vcpu, rd, uaddr, raz);
> >  }
> >  
> >  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >  		      const struct kvm_one_reg *reg, void __user *uaddr)
> >  {
> > -	return __set_id_reg(vcpu, rd, uaddr, false);
> > +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> > +
> > +	return __set_id_reg(vcpu, rd, uaddr, raz);
> >  }
> >  
> >  static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > @@ -1381,6 +1393,7 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >  	.access	= access_id_reg,		\
> >  	.get_user = get_id_reg,			\
> >  	.set_user = set_id_reg,			\
> > +	.visibility = id_visibility,		\
> 
> This is just the default for ID_SANITISED, right?

The default was not to have a visibility function. Now it has one. It
still behaves the same way in this patch, though, because it just returns
zero.

> 
> >  }
> >  
> >  /*
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index 5a6fc30f5989..9d3ef7cfa116 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -61,6 +61,7 @@ struct sys_reg_desc {
> >  
> >  #define REG_HIDDEN_USER		(1 << 0) /* hidden from userspace ioctls */
> >  #define REG_HIDDEN_GUEST	(1 << 1) /* hidden from guest */
> > +#define REG_RAZ			(1 << 2) /* RAZ from userspace ioctls and guest */
> >  
> >  static __printf(2, 3)
> >  inline void print_sys_reg_msg(const struct sys_reg_params *p,
> > @@ -129,6 +130,15 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
> >  	return r->visibility(vcpu, r) & REG_HIDDEN_USER;
> >  }
> >  
> > +static inline bool sysreg_visible_as_raz(const struct kvm_vcpu *vcpu,
> > +					 const struct sys_reg_desc *r)
> > +{
> > +	if (likely(!r->visibility))
> > +		return false;
> > +
> > +	return r->visibility(vcpu, r) & REG_RAZ;
> > +}
> > +
> 
> [...]
> 
> Looks reasonable, I think.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
  2020-11-03 11:32   ` Dave Martin
@ 2020-11-03 13:46     ` Andrew Jones
  2020-11-04 16:36       ` Dave Martin
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-03 13:46 UTC (permalink / raw)
  To: Dave Martin; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 11:32:08AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:37PM +0100, Andrew Jones wrote:
> > The AA64ZFR0_EL1 accessors are just the general accessors with
> > its visibility function open-coded. It also skips the if-else
> > chain in read_id_reg, but there's no reason not to go there.
> > Indeed consolidating ID register accessors and removing lines
> > of code make it worthwhile.
> > 
> > No functional change intended.
> 
> Nit: No statement of what the patch does.

I can duplicate the summary in the commit message?

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 61 +++++++--------------------------------
> >  1 file changed, 11 insertions(+), 50 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index b8822a20b1ea..e2d6fb83280e 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1156,6 +1156,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> >  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> >  				  const struct sys_reg_desc *r)
> >  {
> > +	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> > +			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> > +
> > +	switch (id) {
> > +	case SYS_ID_AA64ZFR0_EL1:
> > +		if (!vcpu_has_sve(vcpu))
> > +			return REG_RAZ;
> > +		break;
> > +	}
> > +
> 
> This should work, but I'm not sure it's preferable to giving affected
> registers their own visibility check function.
> 
> Multiplexing all the ID regs through this one checker function will
> introduce a bit of overhead for always-non-RAZ ID regs, but I'd guess
> the impact is negligible given the other overheads on these paths.

Yes, my though was that a switch isn't going to generate much overhead
and consolidating the ID registers cleans things up a bit.

> 
> >  	return 0;
> >  }
> >  
> > @@ -1203,55 +1213,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> >  }
> >  
> > -/* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> > -static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> > -{
> > -	if (!vcpu_has_sve(vcpu))
> > -		return 0;
> > -
> > -	return read_sanitised_ftr_reg(SYS_ID_AA64ZFR0_EL1);
> > -}
> > -
> > -static bool access_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > -				   struct sys_reg_params *p,
> > -				   const struct sys_reg_desc *rd)
> > -{
> > -	if (p->is_write)
> > -		return write_to_read_only(vcpu, p, rd);
> > -
> > -	p->regval = guest_id_aa64zfr0_el1(vcpu);
> > -	return true;
> > -}
> > -
> > -static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > -		const struct sys_reg_desc *rd,
> > -		const struct kvm_one_reg *reg, void __user *uaddr)
> > -{
> > -	u64 val;
> > -
> > -	val = guest_id_aa64zfr0_el1(vcpu);
> > -	return reg_to_user(uaddr, &val, reg->id);
> > -}
> > -
> > -static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > -		const struct sys_reg_desc *rd,
> > -		const struct kvm_one_reg *reg, void __user *uaddr)
> > -{
> > -	const u64 id = sys_reg_to_index(rd);
> > -	int err;
> > -	u64 val;
> > -
> > -	err = reg_from_user(&val, uaddr, id);
> > -	if (err)
> > -		return err;
> > -
> > -	/* This is what we mean by invariant: you can't change it. */
> > -	if (val != guest_id_aa64zfr0_el1(vcpu))
> > -		return -EINVAL;
> > -
> > -	return 0;
> > -}
> > -
> >  /*
> >   * cpufeature ID register user accessors
> >   *
> > @@ -1515,7 +1476,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	ID_SANITISED(ID_AA64PFR1_EL1),
> >  	ID_UNALLOCATED(4,2),
> >  	ID_UNALLOCATED(4,3),
> > -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> > +	ID_SANITISED(ID_AA64ZFR0_EL1),
> 
> If keeping a dedicated helper, we could have a special macro for that, say
> 
> 	ID_SANITISED_VISIBILITY(ID_AA64ZFR0_EL1, id_aa64zfr0_el1_visibility)

I considered this first, but decided the switch, like read_id_reg's
if-else chain, is probably not going to introduce much overhead.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression
  2020-11-03 11:37 ` [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Dave Martin
@ 2020-11-03 13:52   ` Andrew Jones
  2020-11-04 16:41     ` Dave Martin
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Jones @ 2020-11-03 13:52 UTC (permalink / raw)
  To: Dave Martin; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 11:37:27AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:34PM +0100, Andrew Jones wrote:
> > 张东旭 <xu910121@sina.com> reported a regression seen with CentOS
> > when migrating from an old kernel to a new one. The problem was
> > that QEMU rejected the migration since KVM_GET_REG_LIST reported
> > a register was missing on the destination. Extra registers are OK
> > on the destination, but not missing ones. The regression reproduces
> > with upstream kernels when migrating from a 4.15 or later kernel,
> > up to one with commit 73433762fcae ("KVM: arm64/sve: System register
> > context switch and access support"), to a kernel that includes that
> > commit, e.g. the latest mainline (5.10-rc2).
> > 
> > The first patch of this series is the fix. The next two patches,
> > which don't have any intended functional changes, allow ID_SANITISED
> > to be used for registers that flip between exposing features and
> > being RAZ, which allows some code to be removed.
> 
> Is it worth updating Documentation/virt/kvm/api.rst to clarify the
> expected use during VM migrations, and the guarantees that are expected
> to hold between migratable kernel versions?  Currently the specification
> is a mixture of "surely it's obvious" and "whatever makes QEMU work".
> 
> I guess that caught me out, but I'll let others judge whether other
> people are likely to get similarly confused.
>

I'm not sure what section this would fit in in api.rst. It feels like
this should be a higher level document that covers the migration
guarantees of the API in general. Of course, with host cpu passthrough,
nothing is really guaranteed. The upgrade path is reasonable and probably
doable though.

Thanks,
drew

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace
  2020-11-03 13:32     ` Andrew Jones
@ 2020-11-04 16:11       ` Dave Martin
  0 siblings, 0 replies; 16+ messages in thread
From: Dave Martin @ 2020-11-04 16:11 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm, stable

On Tue, Nov 03, 2020 at 02:32:15PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:18:19AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> > > ID registers are RAZ until they've been allocated a purpose, but
> > > that doesn't mean they should be removed from the KVM_GET_REG_LIST
> > > list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> > > is hidden from userspace when its function is not present. Removing
> > > the userspace visibility checks is enough to reexpose it, as it
> > > already behaves as RAZ when the function is not present.
> > 
> > Pleae state what the patch does.  (The subject line serves as a summary
> > of that, but the commit message should make sense without it.)
> 
> I don't like "This patch ..." type of sentences in the commit message,
> but unless you have a better suggestion, then I'd just add a sentence
> like
> 
> "This patch ensures we still expose sysreg '3, 0, 0, 4, 4'
> (ID_AA64ZFR0_EL1) to userspace as RAZ when SVE is not implemented."

I'm not sure the sysreg encoding number is really needed.
submitting-patches.rst also says such statements should be in the
imperative.  Why not delete the "Removing the userspace visibility
checks..." sentence above and writing:

	Expose ID_AA64ZFR0_EL1 to userspace as RAZ when SVE is not
	implemented.

	Removing the userspace visibility checks is enough to reexpose it,
	as it already behaves as RAZ for the guest when SVE is not present.

(The background to this gripe is that "traditional" mailers may invoke a
standalone editor on the message body when composing reply, so the
subject line may not be visible...)

> 
> > 
> > Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
> > RAZ with this change?  (I'm not saying it doesn't, but the code is not
> > trivial to follow...)
> 
> guest_id_aa64zfr0_el1() returns zero for the register when !vcpu_has_sve(),
> and all the accessors (userspace and guest) build on it.
> 
> I'm not sure how helpful it would be to add that sentence to the commit
> message though.

No worries, I don't think you need to add anthing.  I figured out how
this works after my previously reply, so you can put my question down to
me being slow on the uptake...

> 
> > 
> > > 
> > > Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> > > Cc: <stable@vger.kernel.org> # v5.2+
> > > Reported-by: 张东旭 <xu910121@sina.com>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 18 +-----------------
> > >  1 file changed, 1 insertion(+), 17 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index fb12d3ef423a..6ff0c15531ca 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> > >  }
> > >  
> > > -/* Visibility overrides for SVE-specific ID registers */
> > > -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> > > -				      const struct sys_reg_desc *rd)
> > > -{
> > > -	if (vcpu_has_sve(vcpu))
> > > -		return 0;
> > > -
> > > -	return REG_HIDDEN_USER;
> > 
> > In light of this change, I think that REG_HIDDEN_GUEST and
> > REG_HIDDEN_USER are always either both set or both clear.  Given the
> > discussion on this issue, I'm thinking it probably doesn't even make
> > sense for these to be independent (?)
> > 
> > If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
> > simplifying the code appropriately.
> > 
> > (In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
> > was trying to achieve.)
> 
> We could consolidate REG_HIDDEN_GUEST and REG_HIDDEN_USER into REG_HIDDEN,
> which ZCR_EL1 and ptrauth registers will still use.

Sounds good to me.  Getting rid of _both_ the old names well help avoid
accidents too.

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
  2020-11-03 13:38     ` Andrew Jones
@ 2020-11-04 16:31       ` Dave Martin
  0 siblings, 0 replies; 16+ messages in thread
From: Dave Martin @ 2020-11-04 16:31 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 02:38:36PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:23:54AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:36PM +0100, Andrew Jones wrote:
> > > The instruction encodings of ID registers are preallocated. Until an
> > > encoding is assigned a purpose the register is RAZ. KVM's general ID
> > > register accessor functions already support both paths, RAZ or not.
> > > If for each ID register we can determine if it's RAZ or not, then all
> > > ID registers can build on the general functions. The register visibility
> > > function allows us to check whether a register should be completely
> > > hidden or not, extending it to also report when the register should
> > > be RAZ or not allows us to use it for ID registers as well.
> > 
> > Nit: no statement of what the patch does.
> 
> Hmm, I'm not sure what "...extending it to also report when the register
> should be RAZ or not allows us to use it for ID registers as well." is
> missing, other than spelling out that a new flag is being added for the
> extension. Please provide a suggestion.

Well, that's a subordinate clause, not a statement.  The containing
sentence is a statement about the _implications_ of doing it, but
nothing says that it is actually done.

Often, a less condensed repeat of the subject line is enough, say,
something like the following, as a separate paragraph at the end:

	Check for RAZ visibility in the ID register accessor functions.

(Or rather, there should be a concise statement in the commit message
saying what the patch does, and the subject line should be a suitably
condensed version of _that_.)

You might want to add a simple statement of what is achieved:

	This allows the RAZ case to be handled in a generic way
	for all system registers.

That makes the intention and value of the patch easy to spot, while the
wordy paragraph is available for anyone who wants to understand the
background and rationale in more detail.

(This is just my view, but I think it's generally helpful to reviewers
to follow this rough pattern -- it makes it easy to skip non-critical
parts of the description and come back to them later on.  I might
propose edits in submitting-patches.rst to make this clearer -- and if
they are shot down in flames then I will shut up ;)

> 
> > 
> > You might want to point out that the introduced REG_RAZ functionality is
> > intentionally not used in this patch.
> 
> OK
> 
> > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
> > >  arch/arm64/kvm/sys_regs.h | 10 ++++++++++
> > >  2 files changed, 26 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 6ff0c15531ca..b8822a20b1ea 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1153,6 +1153,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > >  	return val;
> > >  }
> > >  
> > > +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > > +				  const struct sys_reg_desc *r)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > >  /* cpufeature ID register access trap handlers */
> > >  
> > >  static bool __access_id_reg(struct kvm_vcpu *vcpu,
> > > @@ -1171,7 +1177,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
> > >  			  struct sys_reg_params *p,
> > >  			  const struct sys_reg_desc *r)
> > >  {
> > > -	return __access_id_reg(vcpu, p, r, false);
> > > +	bool raz = sysreg_visible_as_raz(vcpu, r);
> > > +
> > > +	return __access_id_reg(vcpu, p, r, raz);
> > >  }
> > >  
> > >  static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > > @@ -1283,13 +1291,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> > >  static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >  		      const struct kvm_one_reg *reg, void __user *uaddr)
> > >  {
> > > -	return __get_id_reg(vcpu, rd, uaddr, false);
> > > +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> > > +
> > > +	return __get_id_reg(vcpu, rd, uaddr, raz);
> > >  }
> > >  
> > >  static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >  		      const struct kvm_one_reg *reg, void __user *uaddr)
> > >  {
> > > -	return __set_id_reg(vcpu, rd, uaddr, false);
> > > +	bool raz = sysreg_visible_as_raz(vcpu, rd);
> > > +
> > > +	return __set_id_reg(vcpu, rd, uaddr, raz);
> > >  }
> > >  
> > >  static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > @@ -1381,6 +1393,7 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > >  	.access	= access_id_reg,		\
> > >  	.get_user = get_id_reg,			\
> > >  	.set_user = set_id_reg,			\
> > > +	.visibility = id_visibility,		\
> > 
> > This is just the default for ID_SANITISED, right?
> 
> The default was not to have a visibility function. Now it has one. It
> still behaves the same way in this patch, though, because it just returns
> zero.

Ack.

Don't worry about this; again, I was halfway through remembering how the
code in this file worked...

[...]

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
  2020-11-03 13:46     ` Andrew Jones
@ 2020-11-04 16:36       ` Dave Martin
  0 siblings, 0 replies; 16+ messages in thread
From: Dave Martin @ 2020-11-04 16:36 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 02:46:40PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:32:08AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:37PM +0100, Andrew Jones wrote:
> > > The AA64ZFR0_EL1 accessors are just the general accessors with
> > > its visibility function open-coded. It also skips the if-else
> > > chain in read_id_reg, but there's no reason not to go there.
> > > Indeed consolidating ID register accessors and removing lines
> > > of code make it worthwhile.
> > > 
> > > No functional change intended.
> > 
> > Nit: No statement of what the patch does.
> 
> I can duplicate the summary in the commit message?

Generally, yes, though there is the opportunity to restore the missing
words and make a proper sentence out of it.  See my response to patch 2.

> > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 61 +++++++--------------------------------
> > >  1 file changed, 11 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index b8822a20b1ea..e2d6fb83280e 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1156,6 +1156,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > >  static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > >  				  const struct sys_reg_desc *r)
> > >  {
> > > +	u32 id = sys_reg((u32)r->Op0, (u32)r->Op1,
> > > +			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
> > > +
> > > +	switch (id) {
> > > +	case SYS_ID_AA64ZFR0_EL1:
> > > +		if (!vcpu_has_sve(vcpu))
> > > +			return REG_RAZ;
> > > +		break;
> > > +	}
> > > +
> > 
> > This should work, but I'm not sure it's preferable to giving affected
> > registers their own visibility check function.
> > 
> > Multiplexing all the ID regs through this one checker function will
> > introduce a bit of overhead for always-non-RAZ ID regs, but I'd guess
> > the impact is negligible given the other overheads on these paths.
> 
> Yes, my though was that a switch isn't going to generate much overhead
> and consolidating the ID registers cleans things up a bit.

Well, no.  I don't have a particularly strong view on this.

The style of the code is being pulled in multiple directions in this
file already, so this doesn't introduce a new inconsistency as such.

If the number of registers handled in here becomes large then we might
want to review the situation again.

> 
> > 
> > >  	return 0;
> > >  }
> > >  
> > > @@ -1203,55 +1213,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> > >  }
> > >  
> > > -/* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> > > -static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> > > -{
> > > -	if (!vcpu_has_sve(vcpu))
> > > -		return 0;
> > > -
> > > -	return read_sanitised_ftr_reg(SYS_ID_AA64ZFR0_EL1);
> > > -}
> > > -
> > > -static bool access_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > > -				   struct sys_reg_params *p,
> > > -				   const struct sys_reg_desc *rd)
> > > -{
> > > -	if (p->is_write)
> > > -		return write_to_read_only(vcpu, p, rd);
> > > -
> > > -	p->regval = guest_id_aa64zfr0_el1(vcpu);
> > > -	return true;
> > > -}
> > > -
> > > -static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > > -		const struct sys_reg_desc *rd,
> > > -		const struct kvm_one_reg *reg, void __user *uaddr)
> > > -{
> > > -	u64 val;
> > > -
> > > -	val = guest_id_aa64zfr0_el1(vcpu);
> > > -	return reg_to_user(uaddr, &val, reg->id);
> > > -}
> > > -
> > > -static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > > -		const struct sys_reg_desc *rd,
> > > -		const struct kvm_one_reg *reg, void __user *uaddr)
> > > -{
> > > -	const u64 id = sys_reg_to_index(rd);
> > > -	int err;
> > > -	u64 val;
> > > -
> > > -	err = reg_from_user(&val, uaddr, id);
> > > -	if (err)
> > > -		return err;
> > > -
> > > -	/* This is what we mean by invariant: you can't change it. */
> > > -	if (val != guest_id_aa64zfr0_el1(vcpu))
> > > -		return -EINVAL;
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  /*
> > >   * cpufeature ID register user accessors
> > >   *
> > > @@ -1515,7 +1476,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >  	ID_SANITISED(ID_AA64PFR1_EL1),
> > >  	ID_UNALLOCATED(4,2),
> > >  	ID_UNALLOCATED(4,3),
> > > -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> > > +	ID_SANITISED(ID_AA64ZFR0_EL1),
> > 
> > If keeping a dedicated helper, we could have a special macro for that, say
> > 
> > 	ID_SANITISED_VISIBILITY(ID_AA64ZFR0_EL1, id_aa64zfr0_el1_visibility)
> 
> I considered this first, but decided the switch, like read_id_reg's
> if-else chain, is probably not going to introduce much overhead.

Agreed.

I don't have a real problem with this patch in its current form.

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression
  2020-11-03 13:52   ` Andrew Jones
@ 2020-11-04 16:41     ` Dave Martin
  0 siblings, 0 replies; 16+ messages in thread
From: Dave Martin @ 2020-11-04 16:41 UTC (permalink / raw)
  To: Andrew Jones; +Cc: maz, xu910121, kvmarm

On Tue, Nov 03, 2020 at 02:52:44PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:37:27AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:34PM +0100, Andrew Jones wrote:
> > > 张东旭 <xu910121@sina.com> reported a regression seen with CentOS
> > > when migrating from an old kernel to a new one. The problem was
> > > that QEMU rejected the migration since KVM_GET_REG_LIST reported
> > > a register was missing on the destination. Extra registers are OK
> > > on the destination, but not missing ones. The regression reproduces
> > > with upstream kernels when migrating from a 4.15 or later kernel,
> > > up to one with commit 73433762fcae ("KVM: arm64/sve: System register
> > > context switch and access support"), to a kernel that includes that
> > > commit, e.g. the latest mainline (5.10-rc2).
> > > 
> > > The first patch of this series is the fix. The next two patches,
> > > which don't have any intended functional changes, allow ID_SANITISED
> > > to be used for registers that flip between exposing features and
> > > being RAZ, which allows some code to be removed.
> > 
> > Is it worth updating Documentation/virt/kvm/api.rst to clarify the
> > expected use during VM migrations, and the guarantees that are expected
> > to hold between migratable kernel versions?  Currently the specification
> > is a mixture of "surely it's obvious" and "whatever makes QEMU work".
> > 
> > I guess that caught me out, but I'll let others judge whether other
> > people are likely to get similarly confused.
> >
> 
> I'm not sure what section this would fit in in api.rst. It feels like
> this should be a higher level document that covers the migration
> guarantees of the API in general. Of course, with host cpu passthrough,
> nothing is really guaranteed. The upgrade path is reasonable and probably
> doable though.

I agree that QEMU is the documentation in practice :P

This may be a situation where strategic vagueness is the best policy,
since in practice people attempting migration will always rely on more
than we can stricly guarantee in the generic API.  The generic rule is
probably "knock yourself out, YMMV".

If there's no clear place to write something up, then I guess we are at
least not making things worse.

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-11-04 16:42 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-02 18:50 [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
2020-11-02 18:50 ` [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
2020-11-03 11:18   ` Dave Martin
2020-11-03 13:32     ` Andrew Jones
2020-11-04 16:11       ` Dave Martin
2020-11-02 18:50 ` [PATCH v2 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
2020-11-03 11:23   ` Dave Martin
2020-11-03 13:38     ` Andrew Jones
2020-11-04 16:31       ` Dave Martin
2020-11-02 18:50 ` [PATCH v2 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
2020-11-03 11:32   ` Dave Martin
2020-11-03 13:46     ` Andrew Jones
2020-11-04 16:36       ` Dave Martin
2020-11-03 11:37 ` [PATCH v2 0/3] KVM: arm64: Fix get-reg-list regression Dave Martin
2020-11-03 13:52   ` Andrew Jones
2020-11-04 16:41     ` Dave Martin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox