diff mbox series

[v11,05/12] KVM: arm64: Bump up the default KVM sanitised debug version to v8p8

Message ID 20231003230408.3405722-6-oliver.upton@linux.dev (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Enable 'writable' ID registers | expand

Commit Message

Oliver Upton Oct. 3, 2023, 11:04 p.m. UTC 
Since ID_AA64DFR0_EL1 and ID_DFR0_EL1 are now writable from userspace,
it is safe to bump up the default KVM sanitised debug version to v8p8.

Signed-off-by: Jing Zhang <jingzhangos@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/sys_regs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Marc Zyngier Oct. 4, 2023, 8:57 a.m. UTC | #1 
On Wed, 04 Oct 2023 00:04:01 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Since ID_AA64DFR0_EL1 and ID_DFR0_EL1 are now writable from userspace,
> it is safe to bump up the default KVM sanitised debug version to v8p8.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

The SoB sequence looks odd. Either you're the author, and Jing's SoB
shouldn't be there without a Co-DB tag, or you've lost Jing's
attribution (which sometimes happens when rebasing and squashing
patches together).

> ---
>  arch/arm64/kvm/sys_regs.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8fbfe61fe7bc..b342c96e08f4 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1496,8 +1496,7 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  {
>  	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
>  
> -	/* Limit debug to ARMv8.0 */
> -	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
> +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
>  
>  	/*
>  	 * Only initialize the PMU version if the vCPU was configured with one.
> @@ -1557,6 +1556,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
>  	if (kvm_vcpu_has_pmu(vcpu))
>  		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
>  
> +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
> +

For consistency, you should also repaint DBGDIDR, which has a
hardcoded '6' (ARMv8) as the supported debug version.

Thanks,

	M.
Oliver Upton Oct. 4, 2023, 5:08 p.m. UTC | #2 
On Wed, Oct 04, 2023 at 09:57:44AM +0100, Marc Zyngier wrote:
> On Wed, 04 Oct 2023 00:04:01 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Since ID_AA64DFR0_EL1 and ID_DFR0_EL1 are now writable from userspace,
> > it is safe to bump up the default KVM sanitised debug version to v8p8.
> > 
> > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> 
> The SoB sequence looks odd. Either you're the author, and Jing's SoB
> shouldn't be there without a Co-DB tag, or you've lost Jing's
> attribution (which sometimes happens when rebasing and squashing
> patches together).

This is an artifact of me applying Jing's version of the series and
hacking on top of it.

> > ---
> >  arch/arm64/kvm/sys_regs.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 8fbfe61fe7bc..b342c96e08f4 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1496,8 +1496,7 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> >  {
> >  	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> >  
> > -	/* Limit debug to ARMv8.0 */
> > -	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
> > +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
> >  
> >  	/*
> >  	 * Only initialize the PMU version if the vCPU was configured with one.
> > @@ -1557,6 +1556,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> >  	if (kvm_vcpu_has_pmu(vcpu))
> >  		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
> >  
> > +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
> > +
> 
> For consistency, you should also repaint DBGDIDR, which has a
> hardcoded '6' (ARMv8) as the supported debug version.

Ah, good point. I'll extract the value from the ID register much like we
do for other fields in DBGDIDR:

commit b92565ca433f611ea0901a6098d72f91be84cdb0
Author: Oliver Upton <oliver.upton@linux.dev>
Date:   Wed Oct 4 17:03:17 2023 +0000

    KVM: arm64: Advertise selected DebugVer in DBGDIDR.Version
    
    Much like we do for other fields, extract the Debug architecture version
    from the ID register to populate the corresponding field in DBGDIDR.
    Rewrite the existing sysreg field extractors to use SYS_FIELD_GET() for
    consistency.
    
    Suggested-by: Marc Zyngier <maz@kernel.org>
    Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index cf1b2def53db..57c8190d5438 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2510,12 +2510,13 @@ static bool trap_dbgdidr(struct kvm_vcpu *vcpu,
 	} else {
 		u64 dfr = IDREG(vcpu->kvm, SYS_ID_AA64DFR0_EL1);
 		u64 pfr = IDREG(vcpu->kvm, SYS_ID_AA64PFR0_EL1);
-		u32 el3 = !!cpuid_feature_extract_unsigned_field(pfr, ID_AA64PFR0_EL1_EL3_SHIFT);
+		u32 el3 = !!SYS_FIELD_GET(ID_AA64PFR0_EL1, EL3, pfr);
 
-		p->regval = ((((dfr >> ID_AA64DFR0_EL1_WRPs_SHIFT) & 0xf) << 28) |
-			     (((dfr >> ID_AA64DFR0_EL1_BRPs_SHIFT) & 0xf) << 24) |
-			     (((dfr >> ID_AA64DFR0_EL1_CTX_CMPs_SHIFT) & 0xf) << 20)
-			     | (6 << 16) | (1 << 15) | (el3 << 14) | (el3 << 12));
+		p->regval = ((SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr) << 28) |
+			     (SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr) << 24) |
+			     (SYS_FIELD_GET(ID_AA64DFR0_EL1, CTX_CMPs, dfr) << 20) |
+			     (SYS_FIELD_GET(ID_AA64DFR0_EL1, DebugVer, dfr) << 16) |
+			     (1 << 15) | (el3 << 14) | (el3 << 12));
 		return true;
 	}
 }
Marc Zyngier Oct. 4, 2023, 5:46 p.m. UTC | #3 
On Wed, 04 Oct 2023 18:08:33 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Oct 04, 2023 at 09:57:44AM +0100, Marc Zyngier wrote:
> > On Wed, 04 Oct 2023 00:04:01 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > Since ID_AA64DFR0_EL1 and ID_DFR0_EL1 are now writable from userspace,
> > > it is safe to bump up the default KVM sanitised debug version to v8p8.
> > > 
> > > Signed-off-by: Jing Zhang <jingzhangos@google.com>
> > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> > 
> > The SoB sequence looks odd. Either you're the author, and Jing's SoB
> > shouldn't be there without a Co-DB tag, or you've lost Jing's
> > attribution (which sometimes happens when rebasing and squashing
> > patches together).
> 
> This is an artifact of me applying Jing's version of the series and
> hacking on top of it.
> 
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 8fbfe61fe7bc..b342c96e08f4 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1496,8 +1496,7 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > >  {
> > >  	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > >  
> > > -	/* Limit debug to ARMv8.0 */
> > > -	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
> > > +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
> > >  
> > >  	/*
> > >  	 * Only initialize the PMU version if the vCPU was configured with one.
> > > @@ -1557,6 +1556,8 @@ static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
> > >  	if (kvm_vcpu_has_pmu(vcpu))
> > >  		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
> > >  
> > > +	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
> > > +
> > 
> > For consistency, you should also repaint DBGDIDR, which has a
> > hardcoded '6' (ARMv8) as the supported debug version.
> 
> Ah, good point. I'll extract the value from the ID register much like we
> do for other fields in DBGDIDR:
> 
> commit b92565ca433f611ea0901a6098d72f91be84cdb0
> Author: Oliver Upton <oliver.upton@linux.dev>
> Date:   Wed Oct 4 17:03:17 2023 +0000
> 
>     KVM: arm64: Advertise selected DebugVer in DBGDIDR.Version
>     
>     Much like we do for other fields, extract the Debug architecture version
>     from the ID register to populate the corresponding field in DBGDIDR.
>     Rewrite the existing sysreg field extractors to use SYS_FIELD_GET() for
>     consistency.
>     
>     Suggested-by: Marc Zyngier <maz@kernel.org>
>     Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8fbfe61fe7bc..b342c96e08f4 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1496,8 +1496,7 @@  static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
 
-	/* Limit debug to ARMv8.0 */
-	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, IMP);
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_AA64DFR0_EL1, DebugVer, V8P8);
 
 	/*
 	 * Only initialize the PMU version if the vCPU was configured with one.
@@ -1557,6 +1556,8 @@  static u64 read_sanitised_id_dfr0_el1(struct kvm_vcpu *vcpu,
 	if (kvm_vcpu_has_pmu(vcpu))
 		val |= SYS_FIELD_PREP(ID_DFR0_EL1, PerfMon, perfmon);
 
+	val = ID_REG_LIMIT_FIELD_ENUM(val, ID_DFR0_EL1, CopDbg, Debugv8p8);
+
 	return val;
 }
 
@@ -2013,7 +2014,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	  .set_user = set_id_dfr0_el1,
 	  .visibility = aa32_id_visibility,
 	  .reset = read_sanitised_id_dfr0_el1,
-	  .val = ID_DFR0_EL1_PerfMon_MASK, },
+	  .val = ID_DFR0_EL1_PerfMon_MASK |
+		 ID_DFR0_EL1_CopDbg_MASK, },
 	ID_HIDDEN(ID_AFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR0_EL1),
 	AA32_ID_SANITISED(ID_MMFR1_EL1),
@@ -2062,7 +2064,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	  .get_user = get_id_reg,
 	  .set_user = set_id_aa64dfr0_el1,
 	  .reset = read_sanitised_id_aa64dfr0_el1,
-	  .val = ID_AA64DFR0_EL1_PMUVer_MASK, },
+	  .val = ID_AA64DFR0_EL1_PMUVer_MASK |
+		 ID_AA64DFR0_EL1_DebugVer_MASK, },
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
 	ID_UNALLOCATED(5,3),