| Message ID | 20240415064758.3250209-2-liaochang1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework the DAIF mask, unmask and track API | expand |
On Mon, Apr 15, 2024 at 06:47:51AM +0000, Liao Chang wrote: > From: Mark Brown <broonie@kernel.org> > > Encodings are provided for ALLINT which allow setting of ALLINT.ALLINT > using an immediate rather than requiring that a register be loaded with > the value to write. Since these don't currently fit within the scheme we > have for sysreg generation add manual encodings like we currently do for > other similar registers such as SVCR. > > Since it is required that these immediate versions be encoded with xzr > as the source register provide asm wrapper which ensure this is the > case. > > Signed-off-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Liao Chang <liaochang1@huawei.com> > --- > arch/arm64/include/asm/nmi.h | 27 +++++++++++++++++++++++++++ > arch/arm64/include/asm/sysreg.h | 2 ++ > 2 files changed, 29 insertions(+) > create mode 100644 arch/arm64/include/asm/nmi.h We have helpers for manipulating PSTATE bits; AFAICT we only need the three lines below: ----8<---- diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 9e8999592f3af..5c209d07ae57e 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -94,18 +94,21 @@ #define PSTATE_PAN pstate_field(0, 4) #define PSTATE_UAO pstate_field(0, 3) +#define PSTATE_ALLINT pstate_field(1, 0) #define PSTATE_SSBS pstate_field(3, 1) #define PSTATE_DIT pstate_field(3, 2) #define PSTATE_TCO pstate_field(3, 4) #define SET_PSTATE_PAN(x) SET_PSTATE((x), PAN) #define SET_PSTATE_UAO(x) SET_PSTATE((x), UAO) +#define SET_PSTATE_ALLINT(x) SET_PSTATE((x), ALLINT) #define SET_PSTATE_SSBS(x) SET_PSTATE((x), SSBS) #define SET_PSTATE_DIT(x) SET_PSTATE((x), DIT) #define SET_PSTATE_TCO(x) SET_PSTATE((x), TCO) #define set_pstate_pan(x) asm volatile(SET_PSTATE_PAN(x)) #define set_pstate_uao(x) asm volatile(SET_PSTATE_UAO(x)) +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) #define set_pstate_ssbs(x) asm volatile(SET_PSTATE_SSBS(x)) #define set_pstate_dit(x) asm volatile(SET_PSTATE_DIT(x)) ---->8---- The addition of <asm/nmi.h> and refrences to <linux/cpumask.h> and arm64_supports_nmi() don't seem like they should be part of this patch. Mark. > > diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h > new file mode 100644 > index 000000000000..0c566c649485 > --- /dev/null > +++ b/arch/arm64/include/asm/nmi.h > @@ -0,0 +1,27 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright (C) 2022 ARM Ltd. > + */ > +#ifndef __ASM_NMI_H > +#define __ASM_NMI_H > + > +#ifndef __ASSEMBLER__ > + > +#include <linux/cpumask.h> > + > +extern bool arm64_supports_nmi(void); > + > +#endif /* !__ASSEMBLER__ */ > + > +static __always_inline void _allint_clear(void) > +{ > + asm volatile(__msr_s(SYS_ALLINT_CLR, "xzr")); > +} > + > +static __always_inline void _allint_set(void) > +{ > + asm volatile(__msr_s(SYS_ALLINT_SET, "xzr")); > +} > + > +#endif > + > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 9e8999592f3a..b105773c57ca 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -167,6 +167,8 @@ > * System registers, organised loosely by encoding but grouped together > * where the architected name contains an index. e.g. ID_MMFR<n>_EL1. > */ > +#define SYS_ALLINT_CLR sys_reg(0, 1, 4, 0, 0) > +#define SYS_ALLINT_SET sys_reg(0, 1, 4, 1, 0) > #define SYS_SVCR_SMSTOP_SM_EL0 sys_reg(0, 3, 4, 2, 3) > #define SYS_SVCR_SMSTART_SM_EL0 sys_reg(0, 3, 4, 3, 3) > #define SYS_SVCR_SMSTOP_SMZA_EL0 sys_reg(0, 3, 4, 6, 3) > -- > 2.34.1 >
在 2024/5/4 0:00, Mark Rutland 写道: > On Mon, Apr 15, 2024 at 06:47:51AM +0000, Liao Chang wrote: >> From: Mark Brown <broonie@kernel.org> >> >> Encodings are provided for ALLINT which allow setting of ALLINT.ALLINT >> using an immediate rather than requiring that a register be loaded with >> the value to write. Since these don't currently fit within the scheme we >> have for sysreg generation add manual encodings like we currently do for >> other similar registers such as SVCR. >> >> Since it is required that these immediate versions be encoded with xzr >> as the source register provide asm wrapper which ensure this is the >> case. >> >> Signed-off-by: Mark Brown <broonie@kernel.org> >> Signed-off-by: Liao Chang <liaochang1@huawei.com> >> --- >> arch/arm64/include/asm/nmi.h | 27 +++++++++++++++++++++++++++ >> arch/arm64/include/asm/sysreg.h | 2 ++ >> 2 files changed, 29 insertions(+) >> create mode 100644 arch/arm64/include/asm/nmi.h > > We have helpers for manipulating PSTATE bits; AFAICT we only need the three > lines below: > > ----8<---- > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 9e8999592f3af..5c209d07ae57e 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -94,18 +94,21 @@ > > #define PSTATE_PAN pstate_field(0, 4) > #define PSTATE_UAO pstate_field(0, 3) > +#define PSTATE_ALLINT pstate_field(1, 0) > #define PSTATE_SSBS pstate_field(3, 1) > #define PSTATE_DIT pstate_field(3, 2) > #define PSTATE_TCO pstate_field(3, 4) > > #define SET_PSTATE_PAN(x) SET_PSTATE((x), PAN) > #define SET_PSTATE_UAO(x) SET_PSTATE((x), UAO) > +#define SET_PSTATE_ALLINT(x) SET_PSTATE((x), ALLINT) > #define SET_PSTATE_SSBS(x) SET_PSTATE((x), SSBS) > #define SET_PSTATE_DIT(x) SET_PSTATE((x), DIT) > #define SET_PSTATE_TCO(x) SET_PSTATE((x), TCO) > > #define set_pstate_pan(x) asm volatile(SET_PSTATE_PAN(x)) > #define set_pstate_uao(x) asm volatile(SET_PSTATE_UAO(x)) > +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) > #define set_pstate_ssbs(x) asm volatile(SET_PSTATE_SSBS(x)) > #define set_pstate_dit(x) asm volatile(SET_PSTATE_DIT(x)) > ---->8---- Acked, I strongly prefer reusing existing helpers to introducing new ones. > > The addition of <asm/nmi.h> and refrences to <linux/cpumask.h> and > arm64_supports_nmi() don't seem like they should be part of this patch. Agree, what about to remove the nmi.h completely in this patch? Thanks. > > Mark. > >> >> diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h >> new file mode 100644 >> index 000000000000..0c566c649485 >> --- /dev/null >> +++ b/arch/arm64/include/asm/nmi.h >> @@ -0,0 +1,27 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* >> + * Copyright (C) 2022 ARM Ltd. >> + */ >> +#ifndef __ASM_NMI_H >> +#define __ASM_NMI_H >> + >> +#ifndef __ASSEMBLER__ >> + >> +#include <linux/cpumask.h> >> + >> +extern bool arm64_supports_nmi(void); >> + >> +#endif /* !__ASSEMBLER__ */ >> + >> +static __always_inline void _allint_clear(void) >> +{ >> + asm volatile(__msr_s(SYS_ALLINT_CLR, "xzr")); >> +} >> + >> +static __always_inline void _allint_set(void) >> +{ >> + asm volatile(__msr_s(SYS_ALLINT_SET, "xzr")); >> +} >> + >> +#endif >> + >> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >> index 9e8999592f3a..b105773c57ca 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -167,6 +167,8 @@ >> * System registers, organised loosely by encoding but grouped together >> * where the architected name contains an index. e.g. ID_MMFR<n>_EL1. >> */ >> +#define SYS_ALLINT_CLR sys_reg(0, 1, 4, 0, 0) >> +#define SYS_ALLINT_SET sys_reg(0, 1, 4, 1, 0) >> #define SYS_SVCR_SMSTOP_SM_EL0 sys_reg(0, 3, 4, 2, 3) >> #define SYS_SVCR_SMSTART_SM_EL0 sys_reg(0, 3, 4, 3, 3) >> #define SYS_SVCR_SMSTOP_SMZA_EL0 sys_reg(0, 3, 4, 6, 3) >> -- >> 2.34.1 >>
On Fri, May 03, 2024 at 05:00:49PM +0100, Mark Rutland wrote: > On Mon, Apr 15, 2024 at 06:47:51AM +0000, Liao Chang wrote: > +#define PSTATE_ALLINT pstate_field(1, 0) > +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) Hrm, those helpers are not ideally discoverable, partly due to the system register description for ALLINT not providing any references to this being a general scheme (which is fixable there) and partly due to the use of __emit_inst() with a numeric literal - we should probably add a comment next to the __emit_inst() saying what instruction we are emitting.
Hi, Mark. 在 2024/5/6 23:15, Mark Brown 写道: > On Fri, May 03, 2024 at 05:00:49PM +0100, Mark Rutland wrote: >> On Mon, Apr 15, 2024 at 06:47:51AM +0000, Liao Chang wrote: > >> +#define PSTATE_ALLINT pstate_field(1, 0) > >> +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) > > Hrm, those helpers are not ideally discoverable, partly due to the > system register description for ALLINT not providing any references to > this being a general scheme (which is fixable there) and partly due to Based on the Arm ISA reference manual, the instruction accessing the ALLINT field of PSTATE uses the following encoding: op0 op1 CRn CRm op2 MSR ALLINT, #<imm> 0b00 0b001 0b0100 0b000x 0b000 In this encoding, the 'x' represents the LSB of #<imm>, op1 is fixed as 0b001 and op2 is fixed as 0b000. With this understanding, those helpers seem like a good approach for accessing the PSTATE.ALLINT field. I've aslo confirmed that the binary encoding generated by those helpers is same with the encoding of v3. > the use of __emit_inst() with a numeric literal - we should probably add > a comment next to the __emit_inst() saying what instruction we are > emitting Arm Architecture Reference Manual for A-profile outlines two variants for MSR instructions used to modify PSTATE fields direclty using immediate. The major difference between these variants lies in the CRm field encoding: - 4 bit immediate, examples include "MSR DAIFSET,#Imm4" and "MSR DAIFCLR,#Imm4". The CRm field in this variant uses the least 4 bits of immediate. - 1 bit immediate, currently, only "MSR ALLINT,#Imm1" uses this variant. The CRm field uses only the least 1 bit of immediate. The current implementation of the macro SET_PSTATE() defaults to the 1 bit immediate variant (!!x << PSTATE_Imm_shift). Currently, this macro is used to generate instructions accessing PAN, UAO, SSBS, TCO and DIT which require 1 bit immediate variant, hence I would say it also work for ALLINT as well. Thanks.
On Tue, May 07, 2024 at 03:41:08PM +0800, Liao, Chang wrote: > 在 2024/5/6 23:15, Mark Brown 写道: > > On Fri, May 03, 2024 at 05:00:49PM +0100, Mark Rutland wrote: > >> +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) > > Hrm, those helpers are not ideally discoverable, partly due to the > > system register description for ALLINT not providing any references to > > this being a general scheme (which is fixable there) and partly due to > Based on the Arm ISA reference manual, the instruction accessing the ALLINT > field of PSTATE uses the following encoding: I'm not saying the suggestion is wrong, I'm saying that between the ARM and the way the code is written the helpers aren't as discoverable as they should be, like I say from a code point of view that's mainly because... > op0 op1 CRn CRm op2 > MSR ALLINT, #<imm> 0b00 0b001 0b0100 0b000x 0b000 ...we only have the encoding for the MSR and don't mention the letters 'msr' anywhere. We should improve that to say what we're encoding in the code (in general I'd say that's true for any __emit_inst(), not just these ones).
Hi, Mark 在 2024/5/7 22:52, Mark Brown 写道: > On Tue, May 07, 2024 at 03:41:08PM +0800, Liao, Chang wrote: >> 在 2024/5/6 23:15, Mark Brown 写道: >>> On Fri, May 03, 2024 at 05:00:49PM +0100, Mark Rutland wrote: > >>>> +#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) > >>> Hrm, those helpers are not ideally discoverable, partly due to the >>> system register description for ALLINT not providing any references to >>> this being a general scheme (which is fixable there) and partly due to > >> Based on the Arm ISA reference manual, the instruction accessing the ALLINT >> field of PSTATE uses the following encoding: > > I'm not saying the suggestion is wrong, I'm saying that between the ARM > and the way the code is written the helpers aren't as discoverable as > they should be, like I say from a code point of view that's mainly > because... > >> op0 op1 CRn CRm op2 >> MSR ALLINT, #<imm> 0b00 0b001 0b0100 0b000x 0b000 > > ...we only have the encoding for the MSR and don't mention the letters > 'msr' anywhere. We should improve that to say what we're encoding in > the code (in general I'd say that's true for any __emit_inst(), not just > these ones). Oh, I apologize any confusion in my previous message. Mark, Is your concern is that the series of pstate related macro name in sysregs.h are lack of self-explanatory nature, which make it diffuclt to understand their functionality and purpose? If so, I daft some alternative macro names in the code below, looking forward to your feedback, or if you have any proposal for making these helpers discoverable. ----8<---- diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 643e2ad73cbd..4f514bdfb1bd 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -90,7 +90,7 @@ */ #define pstate_field(op1, op2) ((op1) << Op1_shift | (op2) << Op2_shift) #define PSTATE_Imm_shift CRm_shift -#define SET_PSTATE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) +#define MSR_PSTATE_ENCODE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) #define PSTATE_PAN pstate_field(0, 4) #define PSTATE_UAO pstate_field(0, 3) @@ -99,18 +99,18 @@ #define PSTATE_TCO pstate_field(3, 4) #define PSTATE_ALLINT pstate_field(1, 0) -#define SET_PSTATE_PAN(x) SET_PSTATE((x), PAN) -#define SET_PSTATE_UAO(x) SET_PSTATE((x), UAO) -#define SET_PSTATE_SSBS(x) SET_PSTATE((x), SSBS) -#define SET_PSTATE_DIT(x) SET_PSTATE((x), DIT) -#define SET_PSTATE_TCO(x) SET_PSTATE((x), TCO) -#define SET_PSTATE_ALLINT(x) SET_PSTATE((x), ALLINT) - -#define set_pstate_pan(x) asm volatile(SET_PSTATE_PAN(x)) -#define set_pstate_uao(x) asm volatile(SET_PSTATE_UAO(x)) -#define set_pstate_ssbs(x) asm volatile(SET_PSTATE_SSBS(x)) -#define set_pstate_dit(x) asm volatile(SET_PSTATE_DIT(x)) -#define set_pstate_allint(x) asm volatile(SET_PSTATE_ALLINT(x)) +#define MSR_PSTATE_PAN(x) MSR_PSTATE_ENCODE((x), PAN) +#define MSR_PSTATE_UAO(x) MSR_PSTATE_ENCODE((x), UAO) +#define MSR_PSTATE_SSBS(x) MSR_PSTATE_ENCODE((x), SSBS) +#define MSR_PSTATE_DIT(x) MSR_PSTATE_ENCODE((x), DIT) +#define MSR_PSTATE_TCO(x) MSR_PSTATE_ENCODE((x), TCO) +#define MSR_PSTATE_ALLINT(x) MSR_PSTATE_ENCODE((x), ALLINT) + +#define msr_pstate_pan(x) asm volatile(MSR_PSTATE_PAN(x)) +#define msr_pstate_uao(x) asm volatile(MSR_PSTATE_UAO(x)) +#define msr_pstate_ssbs(x) asm volatile(MSR_PSTATE_SSBS(x)) +#define msr_pstate_dit(x) asm volatile(MSR_PSTATE_DIT(x)) +#define msr_pstate_allint(x) asm volatile(MSR_PSTATE_ALLINT(x)) ---->8---- Thanks.
On Mon, Jun 03, 2024 at 11:26:39AM +0800, Liao, Chang wrote: > Mark, Is your concern is that the series of pstate related macro name in > sysregs.h are lack of self-explanatory nature, which make it diffuclt to > understand their functionality and purpose? If so, I daft some alternative > macro names in the code below, looking forward to your feedback, or if you > have any proposal for making these helpers discoverable. ... > -#define SET_PSTATE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) > +#define MSR_PSTATE_ENCODE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) Possibly, yes? TBH I was thinking of a comment but that does have "MSR" in it so might come up in greps. Not sure what others would prefer.
Hi, Mark 在 2024/6/4 21:29, Mark Brown 写道: > On Mon, Jun 03, 2024 at 11:26:39AM +0800, Liao, Chang wrote: > >> Mark, Is your concern is that the series of pstate related macro name in >> sysregs.h are lack of self-explanatory nature, which make it diffuclt to >> understand their functionality and purpose? If so, I daft some alternative >> macro names in the code below, looking forward to your feedback, or if you >> have any proposal for making these helpers discoverable. > > ... > >> -#define SET_PSTATE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) >> +#define MSR_PSTATE_ENCODE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) > > Possibly, yes? TBH I was thinking of a comment but that does have "MSR" > in it so might come up in greps. Not sure what others would prefer. I am going to push revision v4 of the interrupt masking patchset, this revision involves several improvements based on feedback and further testing: - Addressed feedback comes form Mark Brown. - Enrich the commit messages of patch includes major changes to provide more detailed explanations of the implemenations, purpose and potential result. This aims to improve developer understanding and reviewing efficiency. - Revising the function names of logical interrupt masking to better reflect their purpose. - Fixed bugs during local testing on platform with FEAT_NMI support. Additionally, i note you left a feedback as below. >...I've started looking at this in the series. There are some subtleties here, and >I don't think the helpers in this series are quite right as-is. I will try to get back to you next week with a description of those; it'll take a short while to write that up correctly and clearly... I appreciate your time and look forward your further feedback at your convenience. Thanks.
Hi, Mark 在 2024/6/5 17:52, Liao, Chang 写道: > Hi, Mark > > 在 2024/6/4 21:29, Mark Brown 写道: >> On Mon, Jun 03, 2024 at 11:26:39AM +0800, Liao, Chang wrote: >> >>> Mark, Is your concern is that the series of pstate related macro name in >>> sysregs.h are lack of self-explanatory nature, which make it diffuclt to >>> understand their functionality and purpose? If so, I daft some alternative >>> macro names in the code below, looking forward to your feedback, or if you >>> have any proposal for making these helpers discoverable. >> >> ... >> >>> -#define SET_PSTATE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) >>> +#define MSR_PSTATE_ENCODE(x, r) __emit_inst(0xd500401f | PSTATE_ ## r | ((!!x) << PSTATE_Imm_shift)) >> >> Possibly, yes? TBH I was thinking of a comment but that does have "MSR" >> in it so might come up in greps. Not sure what others would prefer. > > I am going to push revision v4 of the interrupt masking patchset, this revision > involves several improvements based on feedback and further testing: > Kindly ping, the new revsion is available at the link: https://lore.kernel.org/all/20240614034433.602622-1-liaochang1@huawei.com/
diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h new file mode 100644 index 000000000000..0c566c649485 --- /dev/null +++ b/arch/arm64/include/asm/nmi.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2022 ARM Ltd. + */ +#ifndef __ASM_NMI_H +#define __ASM_NMI_H + +#ifndef __ASSEMBLER__ + +#include <linux/cpumask.h> + +extern bool arm64_supports_nmi(void); + +#endif /* !__ASSEMBLER__ */ + +static __always_inline void _allint_clear(void) +{ + asm volatile(__msr_s(SYS_ALLINT_CLR, "xzr")); +} + +static __always_inline void _allint_set(void) +{ + asm volatile(__msr_s(SYS_ALLINT_SET, "xzr")); +} + +#endif + diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 9e8999592f3a..b105773c57ca 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -167,6 +167,8 @@ * System registers, organised loosely by encoding but grouped together * where the architected name contains an index. e.g. ID_MMFR<n>_EL1. */ +#define SYS_ALLINT_CLR sys_reg(0, 1, 4, 0, 0) +#define SYS_ALLINT_SET sys_reg(0, 1, 4, 1, 0) #define SYS_SVCR_SMSTOP_SM_EL0 sys_reg(0, 3, 4, 2, 3) #define SYS_SVCR_SMSTART_SM_EL0 sys_reg(0, 3, 4, 3, 3) #define SYS_SVCR_SMSTOP_SMZA_EL0 sys_reg(0, 3, 4, 6, 3)