[PATCH 0/2] kvm/arm64: unimplemented sysreg improvements

[PATCH 0/2] kvm/arm64: unimplemented sysreg improvements

[Mark Rutland]

While testing some other patches, I realised that KVM's logging of
trapped sysreg accesses can log inconsistent information, and this is
arguably unnecessary for IMPLEMENTATION DEFINED system registers.

This patches fix that up, ensureing that logged information is
consistent, and avoiding logging for IMPLEMENTATION DEFINED registers.

Mark.

Mark Rutland (2):
  kvm/arm64: sanely ratelimit sysreg messages
  kvm/arm64: don't log IMP DEF sysreg traps

 arch/arm64/kvm/sys_regs.c | 20 ++++++++++++++------
 arch/arm64/kvm/sys_regs.h | 17 +++++++++++++++--
 2 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.11.0


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

[PATCH 1/2] kvm/arm64: sanely ratelimit sysreg messages

[Mark Rutland]

Currently kvm_pr_unimpl() is ratelimited, so print_sys_reg_instr() won't
spam the console. However, someof its callers try to print some
contextual information with kvm_err(), which is not ratelimited. This
means that in some cases the context may be printed without the sysreg
encoding, which isn't all that useful.

Let's ensure that both are consistently printed together and
ratelimited, by refactoring print_sys_reg_instr() so that some callers
can provide it with an arbitrary format string.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
---
 arch/arm64/kvm/sys_regs.c | 12 ++++++------
 arch/arm64/kvm/sys_regs.h | 17 +++++++++++++++--
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 46822afc57e0..d128abd38656 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2098,9 +2098,9 @@ static void unhandled_cp_access(struct kvm_vcpu *vcpu,
 		WARN_ON(1);
 	}
 
-	kvm_err("Unsupported guest CP%d access at: %08lx [%08lx]\n",
-		cp, *vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
-	print_sys_reg_instr(params);
+	print_sys_reg_msg(params,
+			  "Unsupported guest CP%d access at: %08lx [%08lx]\n",
+			  cp, *vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
 	kvm_inject_undefined(vcpu);
 }
 
@@ -2249,9 +2249,9 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 	if (likely(r)) {
 		perform_access(vcpu, params, r);
 	} else {
-		kvm_err("Unsupported guest sys_reg access at: %lx [%08lx]\n",
-			*vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
-		print_sys_reg_instr(params);
+		print_sys_reg_msg(params,
+				  "Unsupported guest sys_reg access at: %lx [%08lx]\n",
+				  *vcpu_pc(vcpu), *vcpu_cpsr(vcpu));
 		kvm_inject_undefined(vcpu);
 	}
 	return 1;
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index 9bca0312d798..5a6fc30f5989 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -62,11 +62,24 @@ struct sys_reg_desc {
 #define REG_HIDDEN_USER		(1 << 0) /* hidden from userspace ioctls */
 #define REG_HIDDEN_GUEST	(1 << 1) /* hidden from guest */
 
-static inline void print_sys_reg_instr(const struct sys_reg_params *p)
+static __printf(2, 3)
+inline void print_sys_reg_msg(const struct sys_reg_params *p,
+				       char *fmt, ...)
 {
+	va_list va;
+
+	va_start(va, fmt);
 	/* Look, we even formatted it for you to paste into the table! */
-	kvm_pr_unimpl(" { Op0(%2u), Op1(%2u), CRn(%2u), CRm(%2u), Op2(%2u), func_%s },\n",
+	kvm_pr_unimpl("%pV { Op0(%2u), Op1(%2u), CRn(%2u), CRm(%2u), Op2(%2u), func_%s },\n",
+		      &(struct va_format){ fmt, &va },
 		      p->Op0, p->Op1, p->CRn, p->CRm, p->Op2, p->is_write ? "write" : "read");
+	va_end(va);
+}
+
+static inline void print_sys_reg_instr(const struct sys_reg_params *p)
+{
+	/* GCC warns on an empty format string */
+	print_sys_reg_msg(p, "%s", "");
 }
 
 static inline bool ignore_write(struct kvm_vcpu *vcpu,
-- 
2.11.0


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

[PATCH 2/2] kvm/arm64: don't log IMP DEF sysreg traps

[Mark Rutland]

We don't intend to support IMPLEMENATION DEFINED system registers, but
have to trap them (and emulate them as UNDEFINED). These traps aren't
interesting to the system administrator or to the KVM developers, so
let's not bother logging when we do so.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
---
 arch/arm64/kvm/sys_regs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d128abd38656..61f019104841 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2233,6 +2233,12 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
 				NULL, 0);
 }
 
+static bool is_imp_def_sys_reg(struct sys_reg_params *params)
+{
+	// See ARM DDI 0487E.a, section D12.3.2
+	return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011;
+}
+
 static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *params)
 {
@@ -2248,6 +2254,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 
 	if (likely(r)) {
 		perform_access(vcpu, params, r);
+	} else if (is_imp_def_sysreg(params)) {
+		kvm_inject_undefined(vcpu);
 	} else {
 		print_sys_reg_msg(params,
 				  "Unsupported guest sys_reg access at: %lx [%08lx]\n",
-- 
2.11.0


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

Re: [PATCH 2/2] kvm/arm64: don't log IMP DEF sysreg traps

[Marc Zyngier]

On Thu, 05 Dec 2019 18:06:52 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> We don't intend to support IMPLEMENATION DEFINED system registers, but
> have to trap them (and emulate them as UNDEFINED). These traps aren't
> interesting to the system administrator or to the KVM developers, so
> let's not bother logging when we do so.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> ---
>  arch/arm64/kvm/sys_regs.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index d128abd38656..61f019104841 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2233,6 +2233,12 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  				NULL, 0);
>  }
>  
> +static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> +{
> +	// See ARM DDI 0487E.a, section D12.3.2
> +	return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011;
> +}
> +
>  static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>  			   struct sys_reg_params *params)
>  {
> @@ -2248,6 +2254,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>  
>  	if (likely(r)) {
>  		perform_access(vcpu, params, r);
> +	} else if (is_imp_def_sysreg(params)) {

Meh. Doesn't compile... :-(
Fixing it locally.

	M.

-- 
Jazz is not dead, it just smells funny.

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

Re: [PATCH 2/2] kvm/arm64: don't log IMP DEF sysreg traps

[Mark Rutland]

On Fri, Dec 06, 2019 at 07:35:56PM +0000, Marc Zyngier wrote:
> On Thu, 05 Dec 2019 18:06:52 +0000,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > We don't intend to support IMPLEMENATION DEFINED system registers, but
> > have to trap them (and emulate them as UNDEFINED). These traps aren't
> > interesting to the system administrator or to the KVM developers, so
> > let's not bother logging when we do so.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Julien Thierry <julien.thierry.kdev@gmail.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: kvmarm@lists.cs.columbia.edu
> > ---
> >  arch/arm64/kvm/sys_regs.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index d128abd38656..61f019104841 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2233,6 +2233,12 @@ int kvm_handle_cp14_32(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >  				NULL, 0);
> >  }
> >  
> > +static bool is_imp_def_sys_reg(struct sys_reg_params *params)
> > +{
> > +	// See ARM DDI 0487E.a, section D12.3.2
> > +	return params->Op0 == 3 && (params->CRn & 0b1011) == 0b1011;
> > +}
> > +
> >  static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> >  			   struct sys_reg_params *params)
> >  {
> > @@ -2248,6 +2254,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> >  
> >  	if (likely(r)) {
> >  		perform_access(vcpu, params, r);
> > +	} else if (is_imp_def_sysreg(params)) {
> 
> Meh. Doesn't compile... :-(
> Fixing it locally.

Whoops, sorry about that. I "fixed" this at the last moment to match
emulate_sys_reg(), but evidently failed to rebuild. I had tested the
patch before the rename on my machine, at least.

Mark.

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

Re: [PATCH 0/2] kvm/arm64: unimplemented sysreg improvements

[Marc Zyngier]

On 2019-12-05 18:06, Mark Rutland wrote:
> While testing some other patches, I realised that KVM's logging of
> trapped sysreg accesses can log inconsistent information, and this is
> arguably unnecessary for IMPLEMENTATION DEFINED system registers.
>
> This patches fix that up, ensureing that logged information is
> consistent, and avoiding logging for IMPLEMENTATION DEFINED 
> registers.
>
> Mark.
>
> Mark Rutland (2):
>   kvm/arm64: sanely ratelimit sysreg messages
>   kvm/arm64: don't log IMP DEF sysreg traps
>
>  arch/arm64/kvm/sys_regs.c | 20 ++++++++++++++------
>  arch/arm64/kvm/sys_regs.h | 17 +++++++++++++++--
>  2 files changed, 29 insertions(+), 8 deletions(-)

Applied, thanks.

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

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