[PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()

[PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()

[Zhiyuan Dai]

ICC_CTLR_EL1 is a 64-bit register.EOImode, bit [1] EOI mode
for the current Security state.

current code semantics is set ICC_CTLR_EL1 register to zero.
This patch only set the EOImode Bit to zero.

refs: See Arm IHI 0069G, page 12-229.

Signed-off-by: Zhiyuan Dai <daizhiyuan@phytium.com.cn>
---
 drivers/irqchip/irq-gic-v3.c       | 9 +++++++--
 include/linux/irqchip/arm-gic-v3.h | 3 +--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index fd4e9a3..96466fc0 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -967,6 +967,7 @@ static void gic_cpu_sys_reg_init(void)
 	u64 need_rss = MPIDR_RS(mpidr);
 	bool group0;
 	u32 pribits;
+	u32 val;
 
 	/*
 	 * Need to check that the SRE bit has actually been set. If
@@ -1009,12 +1010,16 @@ static void gic_cpu_sys_reg_init(void)
 	 */
 	gic_write_bpr1(0);
 
+	val = gic_read_ctlr();
+
 	if (static_branch_likely(&supports_deactivate_key)) {
 		/* EOI drops priority only (mode 1) */
-		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop);
+		val |= ICC_CTLR_EL1_EOImode;
+		gic_write_ctlr(val);
 	} else {
 		/* EOI deactivates interrupt too (mode 0) */
-		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
+		val &= ~ICC_CTLR_EL1_EOImode;
+		gic_write_ctlr(val);
 	}
 
 	/* Always whack Group0 before Group1 */
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 81cbf85..1a35b24 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -544,8 +544,7 @@
  * CPU interface registers
  */
 #define ICC_CTLR_EL1_EOImode_SHIFT	(1)
-#define ICC_CTLR_EL1_EOImode_drop_dir	(0U << ICC_CTLR_EL1_EOImode_SHIFT)
-#define ICC_CTLR_EL1_EOImode_drop	(1U << ICC_CTLR_EL1_EOImode_SHIFT)
+#define ICC_CTLR_EL1_EOImode	(1U << ICC_CTLR_EL1_EOImode_SHIFT)
 #define ICC_CTLR_EL1_EOImode_MASK	(1 << ICC_CTLR_EL1_EOImode_SHIFT)
 #define ICC_CTLR_EL1_CBPR_SHIFT		0
 #define ICC_CTLR_EL1_CBPR_MASK		(1 << ICC_CTLR_EL1_CBPR_SHIFT)
-- 
1.8.3.1

Re: [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()

[Marc Zyngier]

Hi Zhiuyan,

On Thu, 28 Oct 2021 09:01:31 +0100,
Zhiyuan Dai <daizhiyuan@phytium.com.cn> wrote:
> 
> ICC_CTLR_EL1 is a 64-bit register.EOImode, bit [1] EOI mode
> for the current Security state.
> 
> current code semantics is set ICC_CTLR_EL1 register to zero.
> This patch only set the EOImode Bit to zero.
> 
> refs: See Arm IHI 0069G, page 12-229.
> 
> Signed-off-by: Zhiyuan Dai <daizhiyuan@phytium.com.cn>
> ---
>  drivers/irqchip/irq-gic-v3.c       | 9 +++++++--
>  include/linux/irqchip/arm-gic-v3.h | 3 +--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index fd4e9a3..96466fc0 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -967,6 +967,7 @@ static void gic_cpu_sys_reg_init(void)
>  	u64 need_rss = MPIDR_RS(mpidr);
>  	bool group0;
>  	u32 pribits;
> +	u32 val;
>  
>  	/*
>  	 * Need to check that the SRE bit has actually been set. If
> @@ -1009,12 +1010,16 @@ static void gic_cpu_sys_reg_init(void)
>  	 */
>  	gic_write_bpr1(0);
>  
> +	val = gic_read_ctlr();
> +
>  	if (static_branch_likely(&supports_deactivate_key)) {
>  		/* EOI drops priority only (mode 1) */
> -		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop);
> +		val |= ICC_CTLR_EL1_EOImode;
> +		gic_write_ctlr(val);
>  	} else {
>  		/* EOI deactivates interrupt too (mode 0) */
> -		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
> +		val &= ~ICC_CTLR_EL1_EOImode;
> +		gic_write_ctlr(val);

I really wonder why you would need a read-modify-write sequence. There
are no bits in ICC_CTLR_EL1 that we would want to preserve:

- PHME: if it is writable, we really want it to be 0, as we don't use
  1:N distribution

- CBPR: We only use G1 interrupts, and we use ICC_BPR1_EL1 for
  preemption, hence the value being 0

All the other fields (apart from EOImode, obviously) are read-only or
RES0, as per the architecture.

Can you explain what you are trying to achieve here?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: Re: [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()

[戴志远]

Hi, Marc.

Sorry, I was careless, I didn't look carefully at the access properties of each field. 

Thank you for your correction.

> 
> Hi Zhiuyan,
> 
> On Thu, 28 Oct 2021 09:01:31 +0100,
> Zhiyuan Dai <daizhiyuan@phytium.com.cn> wrote:
> > 
> > ICC_CTLR_EL1 is a 64-bit register.EOImode, bit [1] EOI mode
> > for the current Security state.
> > 
> > current code semantics is set ICC_CTLR_EL1 register to zero.
> > This patch only set the EOImode Bit to zero.
> > 
> > refs: See Arm IHI 0069G, page 12-229.
> > 
> > Signed-off-by: Zhiyuan Dai <daizhiyuan@phytium.com.cn>
> > ---
> >  drivers/irqchip/irq-gic-v3.c       | 9 +++++++--
> >  include/linux/irqchip/arm-gic-v3.h | 3 +--
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index fd4e9a3..96466fc0 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -967,6 +967,7 @@ static void gic_cpu_sys_reg_init(void)
> >  	u64 need_rss = MPIDR_RS(mpidr);
> >  	bool group0;
> >  	u32 pribits;
> > +	u32 val;
> >  
> >  	/*
> >  	 * Need to check that the SRE bit has actually been set. If
> > @@ -1009,12 +1010,16 @@ static void gic_cpu_sys_reg_init(void)
> >  	 */
> >  	gic_write_bpr1(0);
> >  
> > +	val = gic_read_ctlr();
> > +
> >  	if (static_branch_likely(&supports_deactivate_key)) {
> >  		/* EOI drops priority only (mode 1) */
> > -		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop);
> > +		val |= ICC_CTLR_EL1_EOImode;
> > +		gic_write_ctlr(val);
> >  	} else {
> >  		/* EOI deactivates interrupt too (mode 0) */
> > -		gic_write_ctlr(ICC_CTLR_EL1_EOImode_drop_dir);
> > +		val &= ~ICC_CTLR_EL1_EOImode;
> > +		gic_write_ctlr(val);
> 
> I really wonder why you would need a read-modify-write sequence. There
> are no bits in ICC_CTLR_EL1 that we would want to preserve:
> 
> - PHME: if it is writable, we really want it to be 0, as we don't use
>   1:N distribution
> 
> - CBPR: We only use G1 interrupts, and we use ICC_BPR1_EL1 for
>   preemption, hence the value being 0
> 
> All the other fields (apart from EOImode, obviously) are read-only or
> RES0, as per the architecture.
> 
> Can you explain what you are trying to achieve here?
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.



信息安全声明：本邮件包含信息归发件人所在组织所有,发件人所在组织对该邮件拥有所有权利。请接收者注意保密,未经发件人书面许可,不得向任何第三方组织和个人透露本邮件所含信息。
Information Security Notice: The information contained in this mail is solely property of the sender's organization.This mail communication is confidential.Recipients named above are obligated to maintain secrecy and are not permitted to disclose the contents of this communication to others.