LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()
@ 2021-10-28  8:01 Zhiyuan Dai
  2021-10-28  8:34 ` Marc Zyngier
  0 siblings, 1 reply; 3+ messages in thread
From: Zhiyuan Dai @ 2021-10-28  8:01 UTC (permalink / raw)
  Cc: daizhiyuan, Thomas Gleixner, Marc Zyngier, Hector Martin,
	Will Deacon, linux-kernel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()
  2021-10-28  8:01 [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init() Zhiyuan Dai
@ 2021-10-28  8:34 ` Marc Zyngier
  2021-10-28  9:43   ` 戴志远
  0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2021-10-28  8:34 UTC (permalink / raw)
  To: Zhiyuan Dai; +Cc: Thomas Gleixner, Hector Martin, Will Deacon, linux-kernel

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Re: [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init()
  2021-10-28  8:34 ` Marc Zyngier
@ 2021-10-28  9:43   ` 戴志远
  0 siblings, 0 replies; 3+ messages in thread
From: 戴志远 @ 2021-10-28  9:43 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: Thomas Gleixner, Hector Martin, Will Deacon, linux-kernel

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-28  9:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-28  8:01 [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg_init() Zhiyuan Dai
2021-10-28  8:34 ` Marc Zyngier
2021-10-28  9:43   ` 戴志远

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).