RE: [PATCH] KVM: arm64: Make L1Ip feature in CTR_EL0 writable from userspace

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

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Friday, October 18, 2024 11:23 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; oliver.upton@linux.dev;
> james.morse@arm.com; joey.gouly@arm.com; suzuki.poulose@arm.com;
> yuzenghui <yuzenghui@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; linux-arm-kernel@lists.infradead.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH] KVM: arm64: Make L1Ip feature in CTR_EL0 writable
> from userspace
> 
> On Thu, 17 Oct 2024 09:59:25 +0100,
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > Only allow userspace to set VIPT(0b10) or PIPT(0b11) for L1Ip based on
> > what hardware reports as both AIVIVT (0b01) and VPIPT (0b00) are
> > documented as reserved.
> >
> > Using a VIPT for Guest where hardware reports PIPT may lead to over
> > invalidation, but is still correct. Hence, we can allow downgrading
> > PIPT to VIPT, but not the other way around.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> > This is based on the dicsussion here[0].
> >
> https://lore.kernel.org/kvmarm/0db19a081d9e41f08b0043baeef16f16@hua
> wei.com/
> >
> > Also depends on Joey's series[1] as it make use of the ID_FILTERED macro.
> >
> > I am not sure we need to explicitly make the ftr type as FTR_LOWER_SAFE
> > in kvm_arm64_ftr_safe_value() or as mentioned below can depend on
> > arm64_ftr_safe_value() for this ftr bits.
> 
> I think relying on the arch code for this is the right thing to
> do. This was designed to cope with heterogeneous systems where you
> could have both PIPT and VIPT caches in the system, and we don't allow
> a late comer to be VIPT if we're decided on PIPT (hence the
> FTR_EXACT).

Ok. I will keep it as it is.

> 
> >
> > Please take a look and let me know.
> >
> > Thanks,
> > Shameer
> >
> > [0]
> https://lore.kernel.org/kvmarm/0db19a081d9e41f08b0043baeef16f16@hua
> wei.com/
> > [1] https://lore.kernel.org/kvmarm/20241015133923.3910916-1-
> joey.gouly@arm.com/
> > ---
> >  arch/arm64/kvm/sys_regs.c | 32 ++++++++++++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index d97ccf1c1558..819dcb63febd 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1872,6 +1872,28 @@ static int set_id_aa64pfr1_el1(struct kvm_vcpu
> *vcpu,
> >  	return set_id_reg(vcpu, rd, user_val);
> >  }
> >
> > +static int set_ctr_el0(struct kvm_vcpu *vcpu,
> > +		       const struct sys_reg_desc *rd, u64 user_val)
> > +{
> > +	u8 user_L1Ip = SYS_FIELD_GET(CTR_EL0, L1Ip, user_val);
> > +
> > +	/*
> > +	 * Both AIVIVT (0b01) and VPIPT (0b00) are documented as reserved.
> > +	 * Hence only allow to set VIPT(0b10) or PIPT(0b11) for L1Ip based
> > +	 * on what hardware reports.
> > +	 *
> > +	 * Using a VIPT software model on PIPT will lead to over
> invalidation,
> > +	 * but still correct. Hence, we can allow downgrading PIPT to VIPT,
> > +	 * but not the other way around. This is handled via
> arm64_ftr_safe_value()
> > +	 * as CTR_EL0 ftr_bits has L1Ip field type FTR_EXACT with safe value
> > +	 * set as VIPT)
> > +	 */
> > +	if (user_L1Ip < CTR_EL0_L1Ip_VIPT)
> > +		return -EINVAL;
> 
> I'm not overly fond of this, because the ordering of cache types is
> arbitrary (it really is an enumeration). I would rather see the
> allowed cache types explicitly listed. It doesn't change a thing, but
> makes it all much more readable.

Does that mean, just check explicitly rather than user_L1Ip < CTR_EL0_L1Ip_VIPT ?
something like,

If ((user_L1Ip !=  CTR_EL0_L1Ip_VIPT) &&  (user_L1Ip !=  CTR_EL0_L1Ip_PIPT)
	return -EINVAL;

But isn't this cache policy type values described in ARM ARM ? So not sure
how they can end up having different enum values.

Or you meant something totally different and I missed it!

Thanks,
Shameer