diff mbox series

[v2] KVM: arm64: Make the exposed feature bits in AA64DFR0_EL1 writable from userspace

Message ID 20240815155954.85480-1-shameerali.kolothum.thodi@huawei.com (mailing list archive)
State New, archived
Headers show
Series [v2] KVM: arm64: Make the exposed feature bits in AA64DFR0_EL1 writable from userspace | expand

Commit Message

Shameerali Kolothum Thodi Aug. 15, 2024, 3:59 p.m. UTC 
KVM exposes the OS double lock feature bit to Guests but returns
RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between
systems where this feature differ. Add support to make this feature
writable from userspace by setting the mask bit. While at it, set the
mask bits for the exposed WRPs(Number of Watchpoints) as well.
Also update the selftest to cover these fields.

However we still can't make BRPs and CTX_CMPs fields writable, because
as per ARM ARM DDI 0487K.a, section G2.8.2 Breakpoint types and
linking of breakpoints, highest numbered breakpoints must be context aware
breakpoints. And it will be problematic if userspace decreases the number
of non-context aware breakpoints as it will make the context aware
breakpoints for the guest mapped to a wrong one.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
   v1 --> v2:
   Removed making BRPs and CTX_CMPs writable.
   v1: https://lore.kernel.org/all/20240813142835.77180-1-shameerali.kolothum.thodi@huawei.com/
---
 arch/arm64/kvm/sys_regs.c                         | 13 ++++++++++++-
 tools/testing/selftests/kvm/aarch64/set_id_regs.c |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Oliver Upton Aug. 15, 2024, 5:42 p.m. UTC | #1 
On Thu, Aug 15, 2024 at 04:59:54PM +0100, Shameer Kolothum wrote:
> KVM exposes the OS double lock feature bit to Guests but returns
> RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between
> systems where this feature differ. Add support to make this feature
> writable from userspace by setting the mask bit. While at it, set the
> mask bits for the exposed WRPs(Number of Watchpoints) as well.
> Also update the selftest to cover these fields.
> 
> However we still can't make BRPs and CTX_CMPs fields writable, because
> as per ARM ARM DDI 0487K.a, section G2.8.2 Breakpoint types and
> linking of breakpoints, highest numbered breakpoints must be context aware
> breakpoints. And it will be problematic if userspace decreases the number
> of non-context aware breakpoints as it will make the context aware
> breakpoints for the guest mapped to a wrong one.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>    v1 --> v2:
>    Removed making BRPs and CTX_CMPs writable.
>    v1: https://lore.kernel.org/all/20240813142835.77180-1-shameerali.kolothum.thodi@huawei.com/
> ---
>  arch/arm64/kvm/sys_regs.c                         | 13 ++++++++++++-
>  tools/testing/selftests/kvm/aarch64/set_id_regs.c |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c90324060436..e77cd6d1abb5 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2376,7 +2376,18 @@ 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 |
> +	/*
> +	 * We can't still make BRPs and CTX_CMPx writable as highest
> +	 * numbered breakpoints must be context aware breakpoints(ARM ARM
> +	 * DDI 0487K.a, section G2.8.2 Breakpoint types and linking of
> +	 * breakpoints). Hence, if the number of non-context aware breakpoints
> +	 * for the Guest is decreased by userspace, that will be problematic
> +	 * as KVM will map context aware breakpoints for the vCPU to different
> +	 * numbered breakpoints for the pCPU.

A few minor nitpicks:

 - BRPs counts the total number of breakpoints, not just the non-context
   aware BRPs

 - KVM doesn't do any register mapping at all, which is why we disallow
   changing the breakpoints altogether

 - The citation is for the AArch32 view of the debug architecture, not
   AArch64.

Perhaps use this wording:

	/*
	 * Prior to FEAT_Debugv8.9, the architecture defines context-aware
	 * breakpoints (CTX_CMPs) as the highest numbered breakpoints (BRPs).
	 * KVM does not trap + emulate the breakpoint registers, and as such
	 * cannot support a layout that misaligns with the underlying hardware.
	 * While it may be possible to describe a subset that aligns with
	 * hardware, just prevent changes to BRPs and CTX_CMPs altogether for
	 * simplicity.
	 *
	 * See DDI0487K.a, section D2.8.3 Breakpoint types and linking
	 * of breakpoints for more details.
	 */

Besides that, LGTM:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

> +	 */
> +	  .val = ID_AA64DFR0_EL1_DoubleLock_MASK |
> +		 ID_AA64DFR0_EL1_WRPs_MASK |
> +		 ID_AA64DFR0_EL1_PMUVer_MASK |
>  		 ID_AA64DFR0_EL1_DebugVer_MASK, },
>  	ID_SANITISED(ID_AA64DFR1_EL1),
>  	ID_UNALLOCATED(5,2),
> diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> index d20981663831..6edc5412abe8 100644
> --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> @@ -68,6 +68,8 @@ struct test_feature_reg {
>  	}
>  
>  static const struct reg_ftr_bits ftr_id_aa64dfr0_el1[] = {
> +	S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DoubleLock, 0),
> +	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, WRPs, 0),
>  	S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, PMUVer, 0),
>  	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DebugVer, ID_AA64DFR0_EL1_DebugVer_IMP),
>  	REG_FTR_END,
> -- 
> 2.45.2
>
Shameerali Kolothum Thodi Aug. 16, 2024, 6:37 a.m. UTC | #2 
> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Thursday, August 15, 2024 6:42 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> maz@kernel.org; will@kernel.org; catalin.marinas@arm.com;
> james.morse@arm.com; suzuki.poulose@arm.com; yuzenghui
> <yuzenghui@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH v2] KVM: arm64: Make the exposed feature bits in
> AA64DFR0_EL1 writable from userspace
> 
> On Thu, Aug 15, 2024 at 04:59:54PM +0100, Shameer Kolothum wrote:
> > KVM exposes the OS double lock feature bit to Guests but returns
> > RAZ/WI on Guest OSDLR_EL1 access. This breaks Guest migration between
> > systems where this feature differ. Add support to make this feature
> > writable from userspace by setting the mask bit. While at it, set the
> > mask bits for the exposed WRPs(Number of Watchpoints) as well.
> > Also update the selftest to cover these fields.
> >
> > However we still can't make BRPs and CTX_CMPs fields writable, because
> > as per ARM ARM DDI 0487K.a, section G2.8.2 Breakpoint types and
> > linking of breakpoints, highest numbered breakpoints must be context
> > aware breakpoints. And it will be problematic if userspace decreases
> > the number of non-context aware breakpoints as it will make the
> > context aware breakpoints for the guest mapped to a wrong one.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >    v1 --> v2:
> >    Removed making BRPs and CTX_CMPs writable.
> >    v1:
> > https://lore.kernel.org/all/20240813142835.77180-1-shameerali.kolothum
> > .thodi@huawei.com/
> > ---
> >  arch/arm64/kvm/sys_regs.c                         | 13 ++++++++++++-
> >  tools/testing/selftests/kvm/aarch64/set_id_regs.c |  2 ++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index c90324060436..e77cd6d1abb5 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -2376,7 +2376,18 @@ 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 |
> > +	/*
> > +	 * We can't still make BRPs and CTX_CMPx writable as highest
> > +	 * numbered breakpoints must be context aware breakpoints(ARM
> ARM
> > +	 * DDI 0487K.a, section G2.8.2 Breakpoint types and linking of
> > +	 * breakpoints). Hence, if the number of non-context aware
> breakpoints
> > +	 * for the Guest is decreased by userspace, that will be problematic
> > +	 * as KVM will map context aware breakpoints for the vCPU to
> different
> > +	 * numbered breakpoints for the pCPU.
> 
> A few minor nitpicks:
> 
>  - BRPs counts the total number of breakpoints, not just the non-context
>    aware BRPs
> 
>  - KVM doesn't do any register mapping at all, which is why we disallow
>    changing the breakpoints altogether

Ok. I will update the commit log and comment here.
> 
>  - The citation is for the AArch32 view of the debug architecture, not
>    AArch64.

Oops, my bad..indeed it is section D2.8.3.

> Perhaps use this wording:
> 
> 	/*
> 	 * Prior to FEAT_Debugv8.9, the architecture defines context-aware
> 	 * breakpoints (CTX_CMPs) as the highest numbered breakpoints
> (BRPs).
> 	 * KVM does not trap + emulate the breakpoint registers, and as such
> 	 * cannot support a layout that misaligns with the underlying
> hardware.
> 	 * While it may be possible to describe a subset that aligns with
> 	 * hardware, just prevent changes to BRPs and CTX_CMPs altogether
> for
> 	 * simplicity.
> 	 *
> 	 * See DDI0487K.a, section D2.8.3 Breakpoint types and linking
> 	 * of breakpoints for more details.
> 	 */
> 
> Besides that, LGTM:
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks. I will respin with the updates.

Shameer
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c90324060436..e77cd6d1abb5 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2376,7 +2376,18 @@  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 |
+	/*
+	 * We can't still make BRPs and CTX_CMPx writable as highest
+	 * numbered breakpoints must be context aware breakpoints(ARM ARM
+	 * DDI 0487K.a, section G2.8.2 Breakpoint types and linking of
+	 * breakpoints). Hence, if the number of non-context aware breakpoints
+	 * for the Guest is decreased by userspace, that will be problematic
+	 * as KVM will map context aware breakpoints for the vCPU to different
+	 * numbered breakpoints for the pCPU.
+	 */
+	  .val = ID_AA64DFR0_EL1_DoubleLock_MASK |
+		 ID_AA64DFR0_EL1_WRPs_MASK |
+		 ID_AA64DFR0_EL1_PMUVer_MASK |
 		 ID_AA64DFR0_EL1_DebugVer_MASK, },
 	ID_SANITISED(ID_AA64DFR1_EL1),
 	ID_UNALLOCATED(5,2),
diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
index d20981663831..6edc5412abe8 100644
--- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
+++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
@@ -68,6 +68,8 @@  struct test_feature_reg {
 	}
 
 static const struct reg_ftr_bits ftr_id_aa64dfr0_el1[] = {
+	S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DoubleLock, 0),
+	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, WRPs, 0),
 	S_REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, PMUVer, 0),
 	REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64DFR0_EL1, DebugVer, ID_AA64DFR0_EL1_DebugVer_IMP),
 	REG_FTR_END,