| Message ID | 20240625133508.259829-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: nv: Add support for address translation instructions | expand |
On 6/25/24 19:05, Marc Zyngier wrote: > As KVM is about to grow a full emulation for the AT instructions, > add the layout of the PAR_EL1 register in its non-D128 configuration. Right, there are two variants for PAR_EL1 i.e D128 and non-D128. Probably it makes sense to define all these PAR_EL1 fields in arch/arm64/include/asm/sysreg.h, until arch/arm64/tools/sysreg evolves to accommodate different bit field layouts for the same register. > > Note that the constants are a bit ugly, as the register has two > layouts, based on the state of the F bit. Just wondering if it would be better to append 'VALID/INVALID' suffix for the fields to differentiate between when F = 0 and when F = 1 ? s/SYS_PAR_EL1_FST/SYS_PAR_INVALID_FST_EL1 s/SYS_PAR_EL1_SH/SYS_PAR_VALID_SH_EL1 Or something similar. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/sysreg.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index be41528194569..15c073359c9e9 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -325,7 +325,25 @@ > #define SYS_PAR_EL1 sys_reg(3, 0, 7, 4, 0) > > #define SYS_PAR_EL1_F BIT(0) > +/* When PAR_EL1.F == 1 */ > #define SYS_PAR_EL1_FST GENMASK(6, 1) > +#define SYS_PAR_EL1_PTW BIT(8) > +#define SYS_PAR_EL1_S BIT(9) > +#define SYS_PAR_EL1_AssuredOnly BIT(12) > +#define SYS_PAR_EL1_TopLevel BIT(13) > +#define SYS_PAR_EL1_Overlay BIT(14) > +#define SYS_PAR_EL1_DirtyBit BIT(15) > +#define SYS_PAR_EL1_F1_IMPDEF GENMASK_ULL(63, 48) > +#define SYS_PAR_EL1_F1_RES0 (BIT(7) | BIT(10) | GENMASK_ULL(47, 16)) > +#define SYS_PAR_EL1_RES1 BIT(11) > +/* When PAR_EL1.F == 0 */ > +#define SYS_PAR_EL1_SH GENMASK_ULL(8, 7) > +#define SYS_PAR_EL1_NS BIT(9) > +#define SYS_PAR_EL1_F0_IMPDEF BIT(10) > +#define SYS_PAR_EL1_NSE BIT(11) > +#define SYS_PAR_EL1_PA GENMASK_ULL(51, 12) > +#define SYS_PAR_EL1_ATTR GENMASK_ULL(63, 56) > +#define SYS_PAR_EL1_F0_RES0 (GENMASK_ULL(6, 1) | GENMASK_ULL(55, 52)) > > /*** Statistical Profiling Extension ***/ > #define PMSEVFR_EL1_RES0_IMP \
On Fri, 12 Jul 2024 08:06:31 +0100, Anshuman Khandual <anshuman.khandual@arm.com> wrote: > > > > On 6/25/24 19:05, Marc Zyngier wrote: > > As KVM is about to grow a full emulation for the AT instructions, > > add the layout of the PAR_EL1 register in its non-D128 configuration. > > Right, there are two variants for PAR_EL1 i.e D128 and non-D128. Probably it makes > sense to define all these PAR_EL1 fields in arch/arm64/include/asm/sysreg.h, until > arch/arm64/tools/sysreg evolves to accommodate different bit field layouts for the > same register. This is really sorely needed, because we can't describe any of the registers that changes layout depending on another control bit. Take for example any of the EL2 registers affected by HCR_EL2.E2H. However, I have no interest in defining *any* D128 format. I take it that whoever will eventually add D128 support to the kernel (and KVM) will take care of that. > > > > > Note that the constants are a bit ugly, as the register has two > > layouts, based on the state of the F bit. > > Just wondering if it would be better to append 'VALID/INVALID' suffix > for the fields to differentiate between when F = 0 and when F = 1 ? > > s/SYS_PAR_EL1_FST/SYS_PAR_INVALID_FST_EL1 > s/SYS_PAR_EL1_SH/SYS_PAR_VALID_SH_EL1 > > Or something similar. I find it pretty horrible. If anything, because "VALID/INVALID" doesn't say anything of *what* is invalid. Also, there is no "VALID" definition in the register, and an aborted translation does not make the register invalid, quite the opposite -- it is full of crucial information. Which is why I used the F0/F1 prefixes, making it clear (at least in my view) that the description is tied to a particular value of the PAR_EL1.F bit. Finally, most of the bit layouts are unambiguous: a field of any given name only exists in a given layout of the register. This means we can safely have names that match the ARM ARM description without any visual pollution. The only ambiguities are with generic names such as RES0 and IMPDEF. Given that we almost never use these bits for anything, I don't think the use of a F-specific prefix is a problem. But yeah, naming is hard. M.
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index be41528194569..15c073359c9e9 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -325,7 +325,25 @@ #define SYS_PAR_EL1 sys_reg(3, 0, 7, 4, 0) #define SYS_PAR_EL1_F BIT(0) +/* When PAR_EL1.F == 1 */ #define SYS_PAR_EL1_FST GENMASK(6, 1) +#define SYS_PAR_EL1_PTW BIT(8) +#define SYS_PAR_EL1_S BIT(9) +#define SYS_PAR_EL1_AssuredOnly BIT(12) +#define SYS_PAR_EL1_TopLevel BIT(13) +#define SYS_PAR_EL1_Overlay BIT(14) +#define SYS_PAR_EL1_DirtyBit BIT(15) +#define SYS_PAR_EL1_F1_IMPDEF GENMASK_ULL(63, 48) +#define SYS_PAR_EL1_F1_RES0 (BIT(7) | BIT(10) | GENMASK_ULL(47, 16)) +#define SYS_PAR_EL1_RES1 BIT(11) +/* When PAR_EL1.F == 0 */ +#define SYS_PAR_EL1_SH GENMASK_ULL(8, 7) +#define SYS_PAR_EL1_NS BIT(9) +#define SYS_PAR_EL1_F0_IMPDEF BIT(10) +#define SYS_PAR_EL1_NSE BIT(11) +#define SYS_PAR_EL1_PA GENMASK_ULL(51, 12) +#define SYS_PAR_EL1_ATTR GENMASK_ULL(63, 56) +#define SYS_PAR_EL1_F0_RES0 (GENMASK_ULL(6, 1) | GENMASK_ULL(55, 52)) /*** Statistical Profiling Extension ***/ #define PMSEVFR_EL1_RES0_IMP \
As KVM is about to grow a full emulation for the AT instructions, add the layout of the PAR_EL1 register in its non-D128 configuration. Note that the constants are a bit ugly, as the register has two layouts, based on the state of the F bit. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/sysreg.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)