| Message ID | 20230719031926.752931-3-shahuang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Define name for the original RES1 bit but now functinal bit | expand |
> On Jul 18, 2023, at 8:19 PM, Shaoqin Huang <shahuang@redhat.com> wrote: > > Currently some fields in SCTLR_EL1 don't define a name and directly used > in the SCTLR_EL1_RES1, that's not good now since these fields have been > functional and have a name. > > According to the ARM DDI 0487J.a, define the name related to these > fields. > > Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com> > Signed-off-by: Shaoqin Huang <shahuang@redhat.com> Thanks for doing that! While I debugged my SCTLR issue on EFI, I did try to grep “SPAN", and this patch would’ve certainly helped.
Hi Shaoqin, On 7/19/23 05:19, Shaoqin Huang wrote: > Currently some fields in SCTLR_EL1 don't define a name and directly used > in the SCTLR_EL1_RES1, that's not good now since these fields have been > functional and have a name. > > According to the ARM DDI 0487J.a, define the name related to these > fields. > > Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com> > Signed-off-by: Shaoqin Huang <shahuang@redhat.com> > --- > lib/arm64/asm/sysreg.h | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h > index c7f529d..9c68698 100644 > --- a/lib/arm64/asm/sysreg.h > +++ b/lib/arm64/asm/sysreg.h > @@ -80,17 +80,26 @@ asm( > #define ICC_GRPEN1_EL1 sys_reg(3, 0, 12, 12, 7) > > /* System Control Register (SCTLR_EL1) bits */ > +#define SCTLR_EL1_LSMAOE _BITUL(29) > +#define SCTLR_EL1_NTLSMD _BITUL(28) > #define SCTLR_EL1_EE _BITUL(25) > +#define SCTLR_EL1_SPAN _BITUL(23) > +#define SCTLR_EL1_EIS _BITUL(22) > +#define SCTLR_EL1_TSCXT _BITUL(20) > #define SCTLR_EL1_WXN _BITUL(19) > #define SCTLR_EL1_I _BITUL(12) > +#define SCTLR_EL1_EOS _BITUL(11) > +#define SCTLR_EL1_SED _BITUL(8) > +#define SCTLR_EL1_ITD _BITUL(7) > #define SCTLR_EL1_SA0 _BITUL(4) > #define SCTLR_EL1_SA _BITUL(3) > #define SCTLR_EL1_C _BITUL(2) > #define SCTLR_EL1_A _BITUL(1) > #define SCTLR_EL1_M _BITUL(0) > > -#define SCTLR_EL1_RES1 (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \ > - _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29)) > +#define SCTLR_EL1_RES1 (SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \ > + SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \ > + SCTLR_EL1_NTLSMD | SCTLR_EL1_LSMAOE) > #define INIT_SCTLR_EL1_MMU_OFF \ > SCTLR_EL1_RES1 > The change looks good to me (although _BITULL remark still holds). Independently on this patch the _RES1 terminology looks odd to me. For example ESO bit is RES1 only if FEAT_ExS is not implemented. Maybe I misunderstand why it was named that way but to me RES1 means another thing. If confirmed we could simply drop SCTLR_EL1_RES1 which is not used elsewhere and directly define INIT_SCTLR_EL1_MMU_OF. Thanks Eric
Hi Eric, On 7/20/23 01:46, Eric Auger wrote: > Hi Shaoqin, > > On 7/19/23 05:19, Shaoqin Huang wrote: >> Currently some fields in SCTLR_EL1 don't define a name and directly used >> in the SCTLR_EL1_RES1, that's not good now since these fields have been >> functional and have a name. >> >> According to the ARM DDI 0487J.a, define the name related to these >> fields. >> >> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com> >> Signed-off-by: Shaoqin Huang <shahuang@redhat.com> >> --- >> lib/arm64/asm/sysreg.h | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h >> index c7f529d..9c68698 100644 >> --- a/lib/arm64/asm/sysreg.h >> +++ b/lib/arm64/asm/sysreg.h >> @@ -80,17 +80,26 @@ asm( >> #define ICC_GRPEN1_EL1 sys_reg(3, 0, 12, 12, 7) >> >> /* System Control Register (SCTLR_EL1) bits */ >> +#define SCTLR_EL1_LSMAOE _BITUL(29) >> +#define SCTLR_EL1_NTLSMD _BITUL(28) >> #define SCTLR_EL1_EE _BITUL(25) >> +#define SCTLR_EL1_SPAN _BITUL(23) >> +#define SCTLR_EL1_EIS _BITUL(22) >> +#define SCTLR_EL1_TSCXT _BITUL(20) >> #define SCTLR_EL1_WXN _BITUL(19) >> #define SCTLR_EL1_I _BITUL(12) >> +#define SCTLR_EL1_EOS _BITUL(11) >> +#define SCTLR_EL1_SED _BITUL(8) >> +#define SCTLR_EL1_ITD _BITUL(7) >> #define SCTLR_EL1_SA0 _BITUL(4) >> #define SCTLR_EL1_SA _BITUL(3) >> #define SCTLR_EL1_C _BITUL(2) >> #define SCTLR_EL1_A _BITUL(1) >> #define SCTLR_EL1_M _BITUL(0) >> >> -#define SCTLR_EL1_RES1 (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \ >> - _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29)) >> +#define SCTLR_EL1_RES1 (SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \ >> + SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \ >> + SCTLR_EL1_NTLSMD | SCTLR_EL1_LSMAOE) >> #define INIT_SCTLR_EL1_MMU_OFF \ >> SCTLR_EL1_RES1 >> > The change looks good to me (although _BITULL remark still holds). > > Independently on this patch the _RES1 terminology looks odd to me. For > example ESO bit is RES1 only if FEAT_ExS is not implemented. Maybe I > misunderstand why it was named that way but to me RES1 means another > thing. If confirmed we could simply drop SCTLR_EL1_RES1 which is not > used elsewhere and directly define INIT_SCTLR_EL1_MMU_OF. The SCTLR_EL1_RES1 was originally defined by commit 10b65ce ("arm64: Configure SCTLR_EL1 at boot"). It seems at that time the BIT(11), yes the EOS bit is undefined, and must be reserved for 1, but now it has a definition which is EOS. It should have no effect if set EOS bit to 1 since its originally res to 1. SCTLR_EL1_RES1 can simply be dropped since it has no other user. Thanks, Shaoqin > > Thanks > > Eric >
diff --git a/lib/arm64/asm/sysreg.h b/lib/arm64/asm/sysreg.h index c7f529d..9c68698 100644 --- a/lib/arm64/asm/sysreg.h +++ b/lib/arm64/asm/sysreg.h @@ -80,17 +80,26 @@ asm( #define ICC_GRPEN1_EL1 sys_reg(3, 0, 12, 12, 7) /* System Control Register (SCTLR_EL1) bits */ +#define SCTLR_EL1_LSMAOE _BITUL(29) +#define SCTLR_EL1_NTLSMD _BITUL(28) #define SCTLR_EL1_EE _BITUL(25) +#define SCTLR_EL1_SPAN _BITUL(23) +#define SCTLR_EL1_EIS _BITUL(22) +#define SCTLR_EL1_TSCXT _BITUL(20) #define SCTLR_EL1_WXN _BITUL(19) #define SCTLR_EL1_I _BITUL(12) +#define SCTLR_EL1_EOS _BITUL(11) +#define SCTLR_EL1_SED _BITUL(8) +#define SCTLR_EL1_ITD _BITUL(7) #define SCTLR_EL1_SA0 _BITUL(4) #define SCTLR_EL1_SA _BITUL(3) #define SCTLR_EL1_C _BITUL(2) #define SCTLR_EL1_A _BITUL(1) #define SCTLR_EL1_M _BITUL(0) -#define SCTLR_EL1_RES1 (_BITUL(7) | _BITUL(8) | _BITUL(11) | _BITUL(20) | \ - _BITUL(22) | _BITUL(23) | _BITUL(28) | _BITUL(29)) +#define SCTLR_EL1_RES1 (SCTLR_EL1_ITD | SCTLR_EL1_SED | SCTLR_EL1_EOS | \ + SCTLR_EL1_TSCXT | SCTLR_EL1_EIS | SCTLR_EL1_SPAN | \ + SCTLR_EL1_NTLSMD | SCTLR_EL1_LSMAOE) #define INIT_SCTLR_EL1_MMU_OFF \ SCTLR_EL1_RES1
Currently some fields in SCTLR_EL1 don't define a name and directly used in the SCTLR_EL1_RES1, that's not good now since these fields have been functional and have a name. According to the ARM DDI 0487J.a, define the name related to these fields. Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com> Signed-off-by: Shaoqin Huang <shahuang@redhat.com> --- lib/arm64/asm/sysreg.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)