* [PATCH 1/3] KVM: arm64: Don't hide ID registers from userspace
2020-10-29 20:11 [PATCH 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
@ 2020-10-29 20:11 ` Andrew Jones
2020-10-31 18:09 ` Marc Zyngier
2020-10-29 20:11 ` [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-10-29 20:11 UTC (permalink / raw)
To: kvmarm; +Cc: maz, xu910121, Dave.Martin
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")
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 d9117bc56237..d24e66ee59b3 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.27.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] KVM: arm64: Don't hide ID registers from userspace
2020-10-29 20:11 ` [PATCH 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
@ 2020-10-31 18:09 ` Marc Zyngier
0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2020-10-31 18:09 UTC (permalink / raw)
To: Andrew Jones; +Cc: xu910121, kvmarm, Dave.Martin
On Thu, 29 Oct 2020 20:11:03 +0000,
Andrew Jones <drjones@redhat.com> 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.
>
> Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> Reported-by: 张东旭 <xu910121@sina.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
Since this was broken in 5.2, it probably deserves a Cc stable (which
I can add when applying this).
> ---
> 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 d9117bc56237..d24e66ee59b3 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.27.0
>
>
Looks like the right thing to do, thanks for following up on this.
Dave, are you happy with this?
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
2020-10-29 20:11 [PATCH 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
2020-10-29 20:11 ` [PATCH 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
@ 2020-10-29 20:11 ` Andrew Jones
2020-10-31 18:23 ` Marc Zyngier
2020-10-29 20:11 ` [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
2020-10-30 8:15 ` [PATCH 0/3] KVM: arm64: Fix get-reg-list regression 张东旭
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-10-29 20:11 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 | 20 ++++++++++++++++++++
2 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d24e66ee59b3..9f6151589460 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1171,7 +1171,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_raz_from_guest(vcpu, r);
+
+ return __access_id_reg(vcpu, p, r, raz);
}
static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
@@ -1283,13 +1285,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_raz_from_user(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_raz_from_user(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,
@@ -1375,12 +1381,19 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
return false;
}
+static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ return 0;
+}
+
/* sys_reg_desc initialiser for known cpufeature ID registers */
#define ID_SANITISED(name) { \
SYS_DESC(SYS_##name), \
.access = access_id_reg, \
.get_user = get_id_reg, \
.set_user = set_id_reg, \
+ .visibility = id_visibility, \
}
/*
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 5a6fc30f5989..d5add36c130a 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -61,6 +61,8 @@ 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_USER (1 << 2) /* RAZ from userspace ioctls */
+#define REG_RAZ_GUEST (1 << 3) /* RAZ from guest */
static __printf(2, 3)
inline void print_sys_reg_msg(const struct sys_reg_params *p,
@@ -129,6 +131,24 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
return r->visibility(vcpu, r) & REG_HIDDEN_USER;
}
+static inline bool sysreg_raz_from_guest(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ if (likely(!r->visibility))
+ return false;
+
+ return r->visibility(vcpu, r) & REG_RAZ_GUEST;
+}
+
+static inline bool sysreg_raz_from_user(const struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *r)
+{
+ if (likely(!r->visibility))
+ return false;
+
+ return r->visibility(vcpu, r) & REG_RAZ_USER;
+}
+
static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
const struct sys_reg_desc *i2)
{
--
2.27.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
2020-10-29 20:11 ` [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
@ 2020-10-31 18:23 ` Marc Zyngier
2020-11-02 8:32 ` Andrew Jones
0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2020-10-31 18:23 UTC (permalink / raw)
To: Andrew Jones; +Cc: xu910121, kvmarm, Dave.Martin
On Thu, 29 Oct 2020 20:11:04 +0000,
Andrew Jones <drjones@redhat.com> 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.
>
> 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 | 20 ++++++++++++++++++++
> 2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d24e66ee59b3..9f6151589460 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1171,7 +1171,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_raz_from_guest(vcpu, r);
> +
> + return __access_id_reg(vcpu, p, r, raz);
> }
>
> static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> @@ -1283,13 +1285,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_raz_from_user(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_raz_from_user(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,
> @@ -1375,12 +1381,19 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> return false;
> }
>
> +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *r)
> +{
> + return 0;
> +}
> +
> /* sys_reg_desc initialiser for known cpufeature ID registers */
> #define ID_SANITISED(name) { \
> SYS_DESC(SYS_##name), \
> .access = access_id_reg, \
> .get_user = get_id_reg, \
> .set_user = set_id_reg, \
> + .visibility = id_visibility, \
> }
>
> /*
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index 5a6fc30f5989..d5add36c130a 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -61,6 +61,8 @@ 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_USER (1 << 2) /* RAZ from userspace ioctls */
> +#define REG_RAZ_GUEST (1 << 3) /* RAZ from guest */
>
> static __printf(2, 3)
> inline void print_sys_reg_msg(const struct sys_reg_params *p,
> @@ -129,6 +131,24 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
> return r->visibility(vcpu, r) & REG_HIDDEN_USER;
> }
>
> +static inline bool sysreg_raz_from_guest(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *r)
> +{
> + if (likely(!r->visibility))
> + return false;
> +
> + return r->visibility(vcpu, r) & REG_RAZ_GUEST;
> +}
> +
> +static inline bool sysreg_raz_from_user(const struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *r)
> +{
> + if (likely(!r->visibility))
> + return false;
> +
> + return r->visibility(vcpu, r) & REG_RAZ_USER;
> +}
> +
> static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
> const struct sys_reg_desc *i2)
> {
Is there actually a case for any ID register to have different
RAZ semantics between guest and userspace? I have the feeling that
we'd want them to be consistent at all times. Or did you have any
particular (and future) use case in mind?
Otherwise, looks good.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors
2020-10-31 18:23 ` Marc Zyngier
@ 2020-11-02 8:32 ` Andrew Jones
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2020-11-02 8:32 UTC (permalink / raw)
To: Marc Zyngier; +Cc: xu910121, kvmarm, Dave.Martin
On Sat, Oct 31, 2020 at 06:23:00PM +0000, Marc Zyngier wrote:
> On Thu, 29 Oct 2020 20:11:04 +0000,
> Andrew Jones <drjones@redhat.com> 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.
> >
> > 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 | 20 ++++++++++++++++++++
> > 2 files changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index d24e66ee59b3..9f6151589460 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1171,7 +1171,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_raz_from_guest(vcpu, r);
> > +
> > + return __access_id_reg(vcpu, p, r, raz);
> > }
> >
> > static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > @@ -1283,13 +1285,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_raz_from_user(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_raz_from_user(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,
> > @@ -1375,12 +1381,19 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > return false;
> > }
> >
> > +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc *r)
> > +{
> > + return 0;
> > +}
> > +
> > /* sys_reg_desc initialiser for known cpufeature ID registers */
> > #define ID_SANITISED(name) { \
> > SYS_DESC(SYS_##name), \
> > .access = access_id_reg, \
> > .get_user = get_id_reg, \
> > .set_user = set_id_reg, \
> > + .visibility = id_visibility, \
> > }
> >
> > /*
> > diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> > index 5a6fc30f5989..d5add36c130a 100644
> > --- a/arch/arm64/kvm/sys_regs.h
> > +++ b/arch/arm64/kvm/sys_regs.h
> > @@ -61,6 +61,8 @@ 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_USER (1 << 2) /* RAZ from userspace ioctls */
> > +#define REG_RAZ_GUEST (1 << 3) /* RAZ from guest */
> >
> > static __printf(2, 3)
> > inline void print_sys_reg_msg(const struct sys_reg_params *p,
> > @@ -129,6 +131,24 @@ static inline bool sysreg_hidden_from_user(const struct kvm_vcpu *vcpu,
> > return r->visibility(vcpu, r) & REG_HIDDEN_USER;
> > }
> >
> > +static inline bool sysreg_raz_from_guest(const struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc *r)
> > +{
> > + if (likely(!r->visibility))
> > + return false;
> > +
> > + return r->visibility(vcpu, r) & REG_RAZ_GUEST;
> > +}
> > +
> > +static inline bool sysreg_raz_from_user(const struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc *r)
> > +{
> > + if (likely(!r->visibility))
> > + return false;
> > +
> > + return r->visibility(vcpu, r) & REG_RAZ_USER;
> > +}
> > +
> > static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
> > const struct sys_reg_desc *i2)
> > {
>
> Is there actually a case for any ID register to have different
> RAZ semantics between guest and userspace? I have the feeling that
> we'd want them to be consistent at all times. Or did you have any
> particular (and future) use case in mind?
I was just following the hidden pattern too closely. You're right
that we'll probably only ever need a single RAZ flag. And, if we do
ever need both, then we can always add another flag later.
I'll respin this patch with just one flag.
>
> Otherwise, looks good.
>
Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
2020-10-29 20:11 [PATCH 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
2020-10-29 20:11 ` [PATCH 1/3] KVM: arm64: Don't hide ID registers from userspace Andrew Jones
2020-10-29 20:11 ` [PATCH 2/3] KVM: arm64: Check RAZ visibility in ID register accessors Andrew Jones
@ 2020-10-29 20:11 ` Andrew Jones
2020-10-31 18:31 ` Marc Zyngier
2020-10-30 8:15 ` [PATCH 0/3] KVM: arm64: Fix get-reg-list regression 张东旭
3 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2020-10-29 20:11 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 9f6151589460..1ccaa5f3b081 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1197,55 +1197,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
*
@@ -1384,6 +1335,16 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
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_USER | REG_RAZ_GUEST;
+ break;
+ }
+
return 0;
}
@@ -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.27.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors
2020-10-29 20:11 ` [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
@ 2020-10-31 18:31 ` Marc Zyngier
0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2020-10-31 18:31 UTC (permalink / raw)
To: Andrew Jones; +Cc: xu910121, kvmarm, Dave.Martin
On Thu, 29 Oct 2020 20:11:05 +0000,
Andrew Jones <drjones@redhat.com> 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.
>
> 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 9f6151589460..1ccaa5f3b081 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1197,55 +1197,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
> *
> @@ -1384,6 +1335,16 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> 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_USER | REG_RAZ_GUEST;
> + break;
> + }
> +
> return 0;
> }
>
> @@ -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),
I really like this, as it establishes a central location to control
the visibility of ID regs, should we need to hide a full register.
Once we establish the actual need to separate RAZ controls between
userspace and guest, I'll be happy to take this.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] KVM: arm64: Fix get-reg-list regression
2020-10-29 20:11 [PATCH 0/3] KVM: arm64: Fix get-reg-list regression Andrew Jones
` (2 preceding siblings ...)
2020-10-29 20:11 ` [PATCH 3/3] KVM: arm64: Remove AA64ZFR0_EL1 accessors Andrew Jones
@ 2020-10-30 8:15 ` 张东旭
3 siblings, 0 replies; 9+ messages in thread
From: 张东旭 @ 2020-10-30 8:15 UTC (permalink / raw)
To: Andrew Jones, kvmarm; +Cc: maz, Dave.Martin
在 2020/10/30 上午4:11, Andrew Jones 写道:
> 张东旭 <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-rc1).
>
> 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.
>
> 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 | 20 ++++++++
> 2 files changed, 47 insertions(+), 69 deletions(-)
>
great! I will apply these patches and have a try.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 9+ messages in thread