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

[Oliver Upton <oliver.upton@linux.dev>]

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\x02
> --- 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
>

-- 
Thanks,
Oliver