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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

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

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