| Message ID | 20241017085925.40532-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make L1Ip feature in CTR_EL0 writable from userspace | expand |
On Thu, 17 Oct 2024, Shameer Kolothum 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> Reviewed-by: Sebastian Ott <sebott@redhat.com>
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@huawei.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). > > Please take a look and let me know. > > Thanks, > Shameer > > [0] https://lore.kernel.org/kvmarm/0db19a081d9e41f08b0043baeef16f16@huawei.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. With this fixed: Reviewed-by: Marc Zyngier <maz@kernel.org> Thanks, M.
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
On Mon, 21 Oct 2024 08:23:24 +0100, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote: > > 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! These are indeed the architected values, but my point is that the architecture doesn't really describe them as an ordered set, which is a difference from the other feature regs. Instead, it is defined as an enumeration by the architecture (there is no order between the possible implementations). Your code is correct, but implies order between the various values. For example, back when VPIPT was still a thing, it had a value of 0b00, which is smaller than VIPT (0b10) and yet you can't emulate VPIPT on top of VIPT. Same thing for AIVIVT. So I'm pretty reluctant to give the impression that there is an order here, and I'd rather enumerate all the valid values. It's not that we have too many anyway. Thanks, M.
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; + + return set_id_reg(vcpu, rd, user_val); +} + /* * cpufeature ID register user accessors * @@ -2614,10 +2636,12 @@ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_CCSIDR2_EL1), undef_access }, { SYS_DESC(SYS_SMIDR_EL1), undef_access }, { SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 }, - ID_WRITABLE(CTR_EL0, CTR_EL0_DIC_MASK | - CTR_EL0_IDC_MASK | - CTR_EL0_DminLine_MASK | - CTR_EL0_IminLine_MASK), + ID_FILTERED(CTR_EL0, ctr_el0, + CTR_EL0_DIC_MASK | + CTR_EL0_IDC_MASK | + CTR_EL0_DminLine_MASK | + CTR_EL0_L1Ip_MASK | + CTR_EL0_IminLine_MASK), { SYS_DESC(SYS_SVCR), undef_access, reset_val, SVCR, 0, .visibility = sme_visibility }, { SYS_DESC(SYS_FPMR), undef_access, reset_val, FPMR, 0, .visibility = fp8_visibility },
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@huawei.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. Please take a look and let me know. Thanks, Shameer [0] https://lore.kernel.org/kvmarm/0db19a081d9e41f08b0043baeef16f16@huawei.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(-)