[PATCH 0/3] KVM: arm64: Fixes for the exposed debug architecture

[PATCH 0/3] KVM: arm64: Fixes for the exposed debug architecture

[Oliver Upton]

I had a conversation with Marc about some of the quirks around the debug
architecture on KVM and incorporated some of his suggestions into a
series here. Of course, any glaring mistakes/choices made in this series
is on me :-)

Anyhow:

KVM's implementation of the debug architecture is a bit deviant as it
stands. For one, KVM handles the OS Lock as RAZ/WI, even though the
architecture mandates it. Additionally, KVM advertises more than it can
actually support: FEAT_DoubleLock is exposed as implemented to the
guest, though OSDLR_EL1 is handled as RAZ/WI too.

Only v8.2+ revisions of the debug architecture permit implementations to
omit DoubleLock. Fortunately, the delta between v8.0 and v8.2 is
entirely focused on external debug, a feature that KVM does not support
and likely never will. So, there isn't much of a hurdle to bump KVM's
reported DebugVer to v8.2, thereby allowing KVM to omit DoubleLock from
ID_AA64DFR0_EL1. Of the remaining bits of external debug visible to the
guest, the only additional thing to address is the OSLAR_EL1 issue by
simply context switching the host/guest values.

Patch 1 changes the way KVM backs OSLSR_EL1 in the sys reg table.
Instead of returning a static value from its handler, stash a copy of it
in kvm_cpu_context and return that when read.

Patch 2 makes the material change of allowing a guest to actually toggle
the OSLK bit by redirecting writes to OSLAR_EL1.OSLK to OSLSR_EL1.OSLK.
When saving context, simply stash the value of OSLSR_EL1. On resume,
apply OSLSR_EL1.OSLK to OSLAR_EL1.OSLK.

Finally, Patch 3 raises the KVM debug architecture to v8.2 and exposes
FEAT_DoubleLock as NI to the guest. With the changes to OSLAR_EL1 in
this series, KVM now does what it says on the tin.

This series applies cleanly to 5.15-rc4, and was (lightly) tested by
booting 5.15-rc4 as a kvmtool guest on this kernel.

Oliver Upton (3):
  KVM: arm64: Stash OSLSR_EL1 in the cpu context
  KVM: arm64: Allow the guest to change the OS Lock status
  KVM: arm64: Raise KVM's reported debug architecture to v8.2

 arch/arm64/include/asm/kvm_host.h          |  1 +
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  5 +++
 arch/arm64/kvm/sys_regs.c                  | 42 ++++++++++++++++------
 3 files changed, 37 insertions(+), 11 deletions(-)

-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 1/3] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

An upcoming change to KVM will context switch the OS Lock status between
guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
using the stored value.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/sys_regs.c         | 13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8be56d5342b..c98f65c4a1f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,6 +172,7 @@ enum vcpu_sysreg {
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
+	OSLSR_EL1,	/* OS Lock Status Register */
 
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..0eb03e7508fe 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -291,12 +291,11 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	if (p->is_write) {
+	if (p->is_write)
 		return ignore_write(vcpu, p);
-	} else {
-		p->regval = (1 << 3);
-		return true;
-	}
+
+	p->regval = vcpu_read_sys_reg(vcpu, r->reg);
+	return true;
 }
 
 static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
@@ -1441,7 +1440,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008 },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
@@ -1916,7 +1915,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
 	DBGBXVR(1),
 	/* DBGOSLSR */
-	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1 },
+	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
 	DBGBXVR(2),
 	DBGBXVR(3),
 	/* DBGOSDLR */
-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 2/3] KVM: arm64: Allow the guest to change the OS Lock status

[Oliver Upton]

KVM diverges from the architecture in the way it handles the OSLAR_EL1
register. While the architecture requires that the register be WO and
that the OSLK bit is 1 out of reset, KVM implements the register as
RAZ/WI.

Align KVM with the architecture by permitting writes to OSLAR_EL1. Since
the register is WO, stash the OS Lock status bit in OSLSR_EL1 and
context switch the status between host/guest. Additionally, change the
reset value of the OSLK bit to 1.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  5 +++++
 arch/arm64/kvm/sys_regs.c                  | 22 +++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
index de7e14c862e6..a65dab34f85b 100644
--- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
+++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
@@ -65,6 +65,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
 	ctxt_sys_reg(ctxt, SP_EL1)	= read_sysreg(sp_el1);
 	ctxt_sys_reg(ctxt, ELR_EL1)	= read_sysreg_el1(SYS_ELR);
 	ctxt_sys_reg(ctxt, SPSR_EL1)	= read_sysreg_el1(SYS_SPSR);
+
+	ctxt_sys_reg(ctxt, OSLSR_EL1)	= read_sysreg(oslsr_el1);
 }
 
 static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
@@ -149,6 +151,9 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
 	write_sysreg(ctxt_sys_reg(ctxt, SP_EL1),	sp_el1);
 	write_sysreg_el1(ctxt_sys_reg(ctxt, ELR_EL1),	SYS_ELR);
 	write_sysreg_el1(ctxt_sys_reg(ctxt, SPSR_EL1),	SYS_SPSR);
+
+	/* restore OSLSR_EL1 by writing the OSLK bit to OSLAR_EL1 */
+	write_sysreg((ctxt_sys_reg(ctxt, OSLSR_EL1) >> 1) & 1, oslar_el1);
 }
 
 static inline void __sysreg_restore_el2_return_state(struct kvm_cpu_context *ctxt)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0eb03e7508fe..0840ae081290 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -298,6 +298,22 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 oslsr;
+
+	if (!p->is_write)
+		return read_zero(vcpu, p);
+
+	/* preserve all but the OSLK bit */
+	oslsr = vcpu_read_sys_reg(vcpu, OSLSR_EL1) & ~0x2ull;
+	vcpu_write_sys_reg(vcpu, OSLSR_EL1, oslsr | ((p->regval & 1) << 1));
+	return true;
+}
+
+
 static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
 				   struct sys_reg_params *p,
 				   const struct sys_reg_desc *r)
@@ -1439,8 +1455,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	DBG_BCR_BVR_WCR_WVR_EL1(15),
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008 },
+	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
@@ -1912,7 +1928,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 
 	DBGBXVR(0),
 	/* DBGOSLAR */
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
+	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
 	DBGBXVR(1),
 	/* DBGOSLSR */
 	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
-- 
2.33.0.1079.g6e70778dc9-goog

[PATCH 3/3] KVM: arm64: Raise KVM's reported debug architecture to v8.2

[Oliver Upton]

The additions made to the Debug architecture between v8.0 and v8.2 are
only applicable to external debug. KVM does not (and likely will never)
support external debug, so KVM can proudly report support for v8.2 to
its guests.

Raise the reported Debug architecture to v8.2. Additionally, v8.2 makes
FEAT_DoubleLock optional. Even though KVM never supported it in the
first place, report DoubleLock as not implemented now as the
architecture permits it for v8.2.

Cc: Reiji Watanabe <reijiw@google.com>
Cc: Ricardo Koller <ricarkol@google.com>
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0840ae081290..f56ee5830d18 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1109,9 +1109,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 				 ARM64_FEATURE_MASK(ID_AA64ISAR1_GPI));
 		break;
 	case SYS_ID_AA64DFR0_EL1:
-		/* Limit debug to ARMv8.0 */
+		/* Limit debug to ARMv8.2 */
 		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
-		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
+		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 8);
+
+		/* Hide DoubleLock from guests */
+		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK);
+		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK), 0xf);
+
 		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-- 
2.33.0.1079.g6e70778dc9-goog

Re: [PATCH 2/3] KVM: arm64: Allow the guest to change the OS Lock status

[Marc Zyngier]

Hi Oliver,

On Fri, 29 Oct 2021 01:32:01 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM diverges from the architecture in the way it handles the OSLAR_EL1
> register. While the architecture requires that the register be WO and
> that the OSLK bit is 1 out of reset, KVM implements the register as
> RAZ/WI.
> 
> Align KVM with the architecture by permitting writes to OSLAR_EL1. Since
> the register is WO, stash the OS Lock status bit in OSLSR_EL1 and
> context switch the status between host/guest. Additionally, change the
> reset value of the OSLK bit to 1.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  5 +++++
>  arch/arm64/kvm/sys_regs.c                  | 22 +++++++++++++++++++---
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index de7e14c862e6..a65dab34f85b 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -65,6 +65,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  	ctxt_sys_reg(ctxt, SP_EL1)	= read_sysreg(sp_el1);
>  	ctxt_sys_reg(ctxt, ELR_EL1)	= read_sysreg_el1(SYS_ELR);
>  	ctxt_sys_reg(ctxt, SPSR_EL1)	= read_sysreg_el1(SYS_SPSR);
> +
> +	ctxt_sys_reg(ctxt, OSLSR_EL1)	= read_sysreg(oslsr_el1);

Why do we need to save/restore this outside of the debug context? It
seems to me that this is only needed if debug has been enabled by the
guest (KVM_ARM64_DEBUG_DIRTY being set), as we will have trapped the
OSLAR_EL1 access otherwise. I don't think we need to deal with this
register outside of this context, as debug exceptions cannot happen
otherwise (BRK excepted, of course).

>  }
>  
>  static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
> @@ -149,6 +151,9 @@ static inline void __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>  	write_sysreg(ctxt_sys_reg(ctxt, SP_EL1),	sp_el1);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, ELR_EL1),	SYS_ELR);
>  	write_sysreg_el1(ctxt_sys_reg(ctxt, SPSR_EL1),	SYS_SPSR);
> +
> +	/* restore OSLSR_EL1 by writing the OSLK bit to OSLAR_EL1 */
> +	write_sysreg((ctxt_sys_reg(ctxt, OSLSR_EL1) >> 1) & 1, oslar_el1);

Please introduce some eye-pleasing bit definitions ("Here, there, and
everywhere", to quote someone famous).

>  }
>  
>  static inline void __sysreg_restore_el2_return_state(struct kvm_cpu_context *ctxt)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0eb03e7508fe..0840ae081290 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -298,6 +298,22 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> +			   struct sys_reg_params *p,
> +			   const struct sys_reg_desc *r)
> +{
> +	u64 oslsr;
> +
> +	if (!p->is_write)
> +		return read_zero(vcpu, p);

This really should be an UNDEF (and it really should UNDEF in HW, but
we are being, maybe pointlessly, cautious).

> +
> +	/* preserve all but the OSLK bit */
> +	oslsr = vcpu_read_sys_reg(vcpu, OSLSR_EL1) & ~0x2ull;
> +	vcpu_write_sys_reg(vcpu, OSLSR_EL1, oslsr | ((p->regval & 1) << 1));
> +	return true;
> +}
> +
> +

Extra newline.

>  static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
>  				   struct sys_reg_params *p,
>  				   const struct sys_reg_desc *r)
> @@ -1439,8 +1455,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	DBG_BCR_BVR_WCR_WVR_EL1(15),
>  
>  	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> -	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> -	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008 },
> +	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> +	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A },
>  	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
> @@ -1912,7 +1928,7 @@ static const struct sys_reg_desc cp14_regs[] = {
>  
>  	DBGBXVR(0),
>  	/* DBGOSLAR */
> -	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> +	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
>  	DBGBXVR(1),
>  	/* DBGOSLSR */
>  	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Marc Zyngier]

On Fri, 29 Oct 2021 01:32:00 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> An upcoming change to KVM will context switch the OS Lock status between
> guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> using the stored value.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/sys_regs.c         | 13 ++++++-------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8be56d5342b..c98f65c4a1f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -172,6 +172,7 @@ enum vcpu_sysreg {
>  	MDSCR_EL1,	/* Monitor Debug System Control Register */
>  	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
>  	DISR_EL1,	/* Deferred Interrupt Status Register */
> +	OSLSR_EL1,	/* OS Lock Status Register */

Please move it one line up, next to the rest of the debug stuff
(DISR_EL1 is RAS and not debug).

>
>  	/* Performance Monitors Registers */
>  	PMCR_EL0,	/* Control Register */
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 1d46e185f31e..0eb03e7508fe 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -291,12 +291,11 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>  			   struct sys_reg_params *p,
>  			   const struct sys_reg_desc *r)
>  {
> -	if (p->is_write) {
> +	if (p->is_write)
>  		return ignore_write(vcpu, p);

This should be UNDEF (though the HW should catch that, really).

> -	} else {
> -		p->regval = (1 << 3);
> -		return true;
> -	}
> +
> +	p->regval = vcpu_read_sys_reg(vcpu, r->reg);
> +	return true;
>  }
>  
>  static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
> @@ -1441,7 +1440,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  
>  	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> -	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1 },
> +	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008 },
>  	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
> @@ -1916,7 +1915,7 @@ static const struct sys_reg_desc cp14_regs[] = {
>  	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
>  	DBGBXVR(1),
>  	/* DBGOSLSR */
> -	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1 },
> +	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
>  	DBGBXVR(2),
>  	DBGBXVR(3),
>  	/* DBGOSDLR */

Please update tools/testing/selftests/kvm/aarch64/get-reg-list.c
before Andrew catches you red-handed! :D

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] KVM: arm64: Raise KVM's reported debug architecture to v8.2

[Marc Zyngier]

On Fri, 29 Oct 2021 01:32:02 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> The additions made to the Debug architecture between v8.0 and v8.2 are
> only applicable to external debug. KVM does not (and likely will never)
> support external debug, so KVM can proudly report support for v8.2 to
> its guests.
> 
> Raise the reported Debug architecture to v8.2. Additionally, v8.2 makes
> FEAT_DoubleLock optional. Even though KVM never supported it in the
> first place, report DoubleLock as not implemented now as the
> architecture permits it for v8.2.
> 
> Cc: Reiji Watanabe <reijiw@google.com>
> Cc: Ricardo Koller <ricarkol@google.com>
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0840ae081290..f56ee5830d18 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1109,9 +1109,14 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  				 ARM64_FEATURE_MASK(ID_AA64ISAR1_GPI));
>  		break;
>  	case SYS_ID_AA64DFR0_EL1:
> -		/* Limit debug to ARMv8.0 */
> +		/* Limit debug to ARMv8.2 */
>  		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> -		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> +		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 8);
> +
> +		/* Hide DoubleLock from guests */
> +		val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK);
> +		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK), 0CF);
> +

One issue with that is that this will break migration from an older
kernel (DFR0 will be different between source and destination).

You'll need a set_user handler and deal with it in a similar way to
CSV2/CSV3.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] KVM: arm64: Raise KVM's reported debug architecture to v8.2

[Oliver Upton]

Hey Marc,

On Fri, Oct 29, 2021 at 4:31 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 29 Oct 2021 01:32:02 +0100,
> Oliver Upton <oupton@google.com> wrote:
[...]
> >       case SYS_ID_AA64DFR0_EL1:
> > -             /* Limit debug to ARMv8.0 */
> > +             /* Limit debug to ARMv8.2 */
> >               val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> > -             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> > +             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 8);
> > +
> > +             /* Hide DoubleLock from guests */
> > +             val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK);
> > +             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK), 0CF);
> > +
>
> One issue with that is that this will break migration from an older
> kernel (DFR0 will be different between source and destination).
>
> You'll need a set_user handler and deal with it in a similar way to
> CSV2/CSV3.

Yeah, definitely so. In that case, unless we're strongly motivated to
expose these changes soon, I'll just punt the ID register changes
until Reiji's series [1] lands, as anything I add for a writable DFR0
will invariably be scrapped in favor of his work.

I'll post v2 of this series folding in your feedback (thx for quick
review, btw), less this patch.

[1] https://patchwork.kernel.org/project/kvm/cover/20211012043535.500493-1-reijiw@google.com/

--
Thanks,
Oliver

Re: [PATCH 3/3] KVM: arm64: Raise KVM's reported debug architecture to v8.2

[Marc Zyngier]

On Fri, 29 Oct 2021 19:18:13 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> Hey Marc,
> 
> On Fri, Oct 29, 2021 at 4:31 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Fri, 29 Oct 2021 01:32:02 +0100,
> > Oliver Upton <oupton@google.com> wrote:
> [...]
> > >       case SYS_ID_AA64DFR0_EL1:
> > > -             /* Limit debug to ARMv8.0 */
> > > +             /* Limit debug to ARMv8.2 */
> > >               val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> > > -             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> > > +             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 8);
> > > +
> > > +             /* Hide DoubleLock from guests */
> > > +             val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK);
> > > +             val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DOUBLELOCK), 0CF);
> > > +
> >
> > One issue with that is that this will break migration from an older
> > kernel (DFR0 will be different between source and destination).
> >
> > You'll need a set_user handler and deal with it in a similar way to
> > CSV2/CSV3.
> 
> Yeah, definitely so. In that case, unless we're strongly motivated to
> expose these changes soon, I'll just punt the ID register changes
> until Reiji's series [1] lands, as anything I add for a writable DFR0
> will invariably be scrapped in favor of his work.

Yeah, I think that's a sensible thing to do. I need to find the
bandwidth to look into these patches...

> I'll post v2 of this series folding in your feedback (thx for quick
> review, btw), less this patch.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.