linux-kernel - Re: Re: [PATCH] irqchip/gic-v3: Fix EOImode semantics in git_cpu_sys_reg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5c537930.226.17cc6485afa.Coremail.daizhiyuan@phytium.com.cn>
Date:   Thu, 28 Oct 2021 17:43:23 +0800 (GMT+08:00)
From:   戴志远 <daizhiyuan@...tium.com.cn>
To:     "Marc Zyngier" <maz@...nel.org>
Cc:     "Thomas Gleixner" <tglx@...utronix.de>,
        "Hector Martin" <marcan@...can.st>,
        "Will Deacon" <will@...nel.org>, linux-kernel@...r.kernel.org
Subject: 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@...tium.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@...tium.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.