Re: [PATCH v4 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Dec 14, 2021 at 05:28:09PM +0000, Oliver Upton wrote:
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> the value for now.
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  9 ++++++++
>  arch/arm64/kvm/sys_regs.c       | 39 ++++++++++++++++++++++++++-------
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..46f800bda045 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -129,7 +129,16 @@
>  #define SYS_DBGWCRn_EL1(n)		sys_reg(2, 0, 0, n, 7)
>  #define SYS_MDRAR_EL1			sys_reg(2, 0, 1, 0, 0)
>  #define SYS_OSLAR_EL1			sys_reg(2, 0, 1, 0, 4)
> +
> +#define SYS_OSLAR_OSLK			BIT(0)
> +
>  #define SYS_OSLSR_EL1			sys_reg(2, 0, 1, 1, 4)
> +
> +#define SYS_OSLSR_OSLK			BIT(1)
> +
> +#define SYS_OSLSR_OSLM_MASK		(BIT(3) | BIT(0))
> +#define SYS_OSLSR_OSLM			BIT(3)

Since `OSLM` is the field as a whole, I think this should have another level of
hierarchy, e.g.

#define SYS_OSLSR_OSLM_MASK			(BIT(3) | BIT(0))
#define SYS_OSLSR_OSLM_NI			0
#define SYS_OSLSR_OSLM_OSLK			BIT(3)

[...]

> +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_from_write_only(vcpu, p, r);
> +
> +	/* Forward the OSLK bit to OSLSR */
> +	oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> +	if (p->regval & SYS_OSLAR_OSLK)
> +		oslsr |= SYS_OSLSR_OSLK;
> +
> +	__vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> +	return true;
> +}

Does changing this affect existing userspace? Previosuly it could read
OSLAR_EL1 as 0, whereas now that should be rejected.

That might be fine, and if so, it would be good to call that out in the commit
message.

[...]

> @@ -309,9 +331,14 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>  	if (err)
>  		return err;
>  
> -	if (val != rd->val)
> +	/*
> +	 * The only modifiable bit is the OSLK bit. Refuse the write if
> +	 * userspace attempts to change any other bit in the register.
> +	 */
> +	if ((val & ~SYS_OSLSR_OSLK) != SYS_OSLSR_OSLM)
>  		return -EINVAL;

How about:

	if ((val ^ rd->val) & ~SYS_OSLSR_OSLK)
		return -EINVAL;

... so that we don't need to hard-code the expected value here, and can more
easily change it in future?

[...]

> @@ -1463,8 +1486,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, SYS_OSLSR_OSLM,
>  		.set_user = set_oslsr_el1, },
>  	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> @@ -1937,7 +1960,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 },

As above, I have a slight concern that this could adversely affect existing
userspace, but I can also believe that's fine.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel