linux-kernel - Re: [PATCH v10 12/15] cxl/pci: Disable root port interrupts in RCH mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6513515526995_bf9129471@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Tue, 26 Sep 2023 14:47:01 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Terry Bowman <Terry.Bowman@....com>,
        Dan Williams <dan.j.williams@...el.com>,
        <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
        <ira.weiny@...el.com>, <bwidawsk@...nel.org>,
        <dave.jiang@...el.com>, <Jonathan.Cameron@...wei.com>,
        <linux-cxl@...r.kernel.org>
CC:     <rrichter@....com>, <linux-kernel@...r.kernel.org>,
        <bhelgaas@...gle.com>
Subject: Re: [PATCH v10 12/15] cxl/pci: Disable root port interrupts in RCH
 mode

Terry Bowman wrote:
> Hi Dan,
> 
> I added comments below.
> 
> On 9/15/23 13:43, Dan Williams wrote:
> > Terry Bowman wrote:
> >> The RCH root port contains root command AER registers that should not be
> >> enabled.[1] Disable these to prevent root port interrupts.
> >>
> >> [1] CXL 3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
> >>
> >> Signed-off-by: Terry Bowman <terry.bowman@....com>
> >> Signed-off-by: Robert Richter <rrichter@....com>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> >> Reviewed-by: Dave Jiang <dave.jiang@...el.com>
> > [..]
> >> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> >> index 2a22a7ed4704..d195af72ed65 100644
> >> --- a/drivers/cxl/core/port.c
> >> +++ b/drivers/cxl/core/port.c
> >> @@ -1042,6 +1042,9 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
> >>  
> >>  	cxl_dport_map_regs(dport);
> >>  
> >> +	if (dport->rch)
> >> +		cxl_disable_rch_root_ints(dport);
> >> +
> > 
> > Similar to the comment about cxl_dport_map_regs() not being appropriate
> > in an enumeration routine, this also needs to move out of _add_dport. It
> > occurs to me that it should also be undone on driver detach just like
> > other device "enables".
> 
> Ok. I will move out of enumeration. 
> 
> Per the 'undo' request: This is a RCH downstream port (dport) with PCIe root port 
> capability. PCI spec states root port error reporting is disabled by default at 
> powerup. And SW does *not* enable the root port errors because the RCH dport is *not* 
> bound to a root port driver (missing BDF, etc). This mask is added to follow the 
> CXL spec precisely and if the rest of the system behaves as expected should not 
> be necessary. 

Ah, got it perhaps add a comment to sanity check that the hardware is in
the per-spec state. Are you certain that even in firmware-first error
handling it is safe for the driver to unconditionally disable these
interrupts?

> I don't believe masking should be 'undone' in driver detach or elsewhere. Adding 
> the 'undo' masking would potentially introduce RCH dport root port interrupt 
> reporting which is incorrect for the RCH/RCD mode. Only CXL components (device, 
> uport, switch) may reside under the RCH dport and never want RCH dport reporting 
> root port errors. RCEC reports the root complex errors in RCH/RCD mode.

Ok, that also seems to suggest that even in the firmware-first case the
driver should make sure they are off per-spec.