[PATCH v2 2/7] KVM: arm64: Abstract set/clear of CPTR_EL2 bits behind helper
Marc Zyngier
maz at kernel.org
Tue May 28 00:58:04 PDT 2024
On Wed, 22 May 2024 14:48:19 +0100,
Fuad Tabba <tabba at google.com> wrote:
>
> Hi Marc,
>
> On Tue, May 21, 2024 at 10:08 PM Marc Zyngier <maz at kernel.org> wrote:
> >
> > On Tue, 21 May 2024 17:37:15 +0100,
> > Fuad Tabba <tabba at google.com> wrote:
> > >
> > > The same traps controlled by CPTR_EL2 or CPACR_EL1 need to be
> > > toggled in different parts of the code, but the exact bits and
> > > their polarity differ between these two formats and the mode
> > > (vhe/nvhe/hvhe).
> > >
> > > To reduce the amount of duplicated code and the chance of getting
> > > the wrong bit/polarity or missing a field, abstract the set/clear
> > > of CPTR_EL2 bits behind a helper.
> > >
> > > Since (h)VHE is the way of the future, use the CPACR_EL1 format,
> > > which is a subset of the VHE CPTR_EL2, as a reference.
> > >
> > > No functional change intended.
> > >
> > > Suggested-by: Oliver Upton <oliver.upton at linux.dev>
> > > Signed-off-by: Fuad Tabba <tabba at google.com>
> > > ---
> > > arch/arm64/include/asm/kvm_emulate.h | 34 +++++++++++++++++++++++++
> > > arch/arm64/kvm/hyp/include/hyp/switch.h | 17 +++----------
> > > arch/arm64/kvm/hyp/nvhe/hyp-main.c | 6 +----
> > > 3 files changed, 39 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > index 501e3e019c93..74837d1762e5 100644
> > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > @@ -557,6 +557,40 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu)
> > > vcpu_set_flag((v), e); \
> > > } while (0)
> > >
> > > +
> > > +static inline void __cptr_clear_set_nvhe(u64 cpacr_clr, u64 cpacr_set)
> > > +{
> > > + u64 clr = 0, set = 0;
> > > +
> > > + if (cpacr_clr & CPACR_ELx_FPEN)
> > > + set |= CPTR_EL2_TFP;
> > > + if (cpacr_clr & CPACR_ELx_ZEN)
> > > + set |= CPTR_EL2_TZ;
> > > + if (cpacr_clr & CPACR_ELx_SMEN)
> >
> > These 3 fields are actually pairs of bits. Can we have a compile-time
> > check that both bits are set?
>
> Yes.
>
> > > + set |= CPTR_EL2_TSM;
> > > + if (cpacr_clr & CPACR_ELx_TTA)
> > > + clr |= CPTR_EL2_TTA;
> >
> > How about TCPAC, TAM, and E0POE?
>
> CPTR_EL2 (nvhe) doesn't have an equivalent to E0POE. Should I have a
> compile time check that it's not cleared at all (since it has negative
> polarity), or a runtime check for that only for nvhe?
You're right, and I ended figuring that out while reworking the NV
side of CPTR_EL2 handling.
>
> As for TCPAC, TAM, they're not defined under the CPACR bits, but they
> are in CPTR_EL2, which is what CPACR refers to in VHE. I will handle
> them, but should I rename cpacr_to_cptr() to cptr_vhe_to_nvhe()?
I think this is clear enough in context, but I don't mind either way.
>
> >
> > > +
> > > + if (cpacr_set & CPACR_ELx_FPEN)
> > > + clr |= CPTR_EL2_TFP;
> > > + if (cpacr_set & CPACR_ELx_ZEN)
> > > + clr |= CPTR_EL2_TZ;
> > > + if (cpacr_set & CPACR_ELx_SMEN)
> > > + clr |= CPTR_EL2_TSM;
> > > + if (cpacr_set & CPACR_ELx_TTA)
> > > + set |= CPTR_EL2_TTA;
> >
> > The duplication is pretty unfortunate. Having a single helper that
> > translate a register layout into another would be better.
> > > +
> > > + sysreg_clear_set(cptr_el2, clr, set);
> >
> > And omit this...
> >
> > > +}
> > > +
> > > +static inline void cpacr_clear_set(u64 clr, u64 set)
> > > +{
> > > + if (has_vhe() || has_hvhe())
> > > + sysreg_clear_set(cpacr_el1, clr, set);
> > > + else
> > > + __cptr_clear_set_nvhe(clr, set);
> >
> > So that this could read as:
> >
> > sysreg_clear_set(cptr_el2, cpacr_to_cptr(clr), cpacr_to_cptr(set));
>
> I don't know how this could work with only one function/macro, while
> covering all the fields. The problem is that the polarity of these
> bits vary: some have the same polarity between CPACR/CPTR. and others
> have the opposite polarity. Therefore, the conversion to `clr` is
> different from the conversion to `set`.
Ah, you're absolutely right. I totally glanced over the fact that the
polarities are inverted, totally ruining the fun.
>
> Say we need to clear both CPACR_ELx_FPEN and CPACR_ELx_TTA. The
> problem is that CPACR_ELx_FPEN has the opposite polarity as
> CPTR_EL2_TFP, whereas CPACR_ELx_TTA has the same polarity as
> CPTR_EL2_TTA (different position though).
>
> What would cpacr_to_cptr() return that would work in the code you propose:
> sysreg_clear_set(cptr_el2,
> cpacr_to_cptr(CPACR_ELx_FPEN|CPACR_ELx_TTA), cpacr_to_cptr(0));
>
> Desired result:
> sysreg_clear_set(cptr_el2, CPTR_EL2_TTA, CPTR_EL2_TFP);
>
> I can make it work with two functions/macros:
> __cpacr_to_cptr_clear(clr, set) and __cpacr_to_cptr_set(clr, set) -->
> sysreg_clear_set(cptr_el2, __cpacr_to_cptr_clear(clr, set),
> __cpacr_to_cptr_set(clr, set));
>
> Unfortunately this doesn't get rid of the duplication, but it might be
> tidier (I converted them to macros to add compile-time checks later):
>
> +#define __cpacr_to_cptr_clr(clr, set) \
> + ({ \
> + u64 cptr = 0; \
> + \
> + if ((set) & CPACR_ELx_FPEN) \
> + cptr |= CPTR_EL2_TFP; \
> + if ((set) & CPACR_ELx_ZEN) \
> + cptr |= CPTR_EL2_TZ; \
> + if ((set) & CPACR_ELx_SMEN) \
> + cptr |= CPTR_EL2_TSM; \
> + if ((clr) & CPACR_ELx_TTA) \
> + cptr |= CPTR_EL2_TTA; \
> + if ((clr) & CPTR_EL2_TAM) \
> + cptr |= CPTR_EL2_TAM; \
> + if ((clr) & CPTR_EL2_TCPAC) \
> + cptr |= CPTR_EL2_TCPAC; \
> + \
> + cptr; \
> + })
> +
> +#define __cpacr_to_cptr_set(clr, set) \
> + ({ \
> + u64 cptr = 0; \
> + \
> + if ((clr) & CPACR_ELx_FPEN) \
> + cptr |= CPTR_EL2_TFP; \
> + if ((clr) & CPACR_ELx_ZEN) \
> + cptr |= CPTR_EL2_TZ; \
> + if ((clr) & CPACR_ELx_SMEN) \
> + cptr |= CPTR_EL2_TSM; \
> + if ((set) & CPACR_ELx_TTA) \
> + cptr |= CPTR_EL2_TTA; \
> + if ((set) & CPTR_EL2_TAM) \
> + cptr |= CPTR_EL2_TAM; \
> + if ((set) & CPTR_EL2_TCPAC) \
> + cptr |= CPTR_EL2_TCPAC; \
> + \
> + cptr; \
> + })
> +
> +#define cpacr_clear_set(clr, set) \
> + do { \
> + if (has_vhe() || has_hvhe()) \
> + sysreg_clear_set(cpacr_el1, clr, set); \
> + else \
> + sysreg_clear_set(cptr_el2, \
> + __cpacr_to_cptr_clr(clr, set), \
> + __cpacr_to_cptr_set(clr, set));\
> + } while (0)
>
> What do you think?
I quite like this. It is a large body of code, but it is easy to read
and symmetric.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
More information about the linux-arm-kernel
mailing list