linux-kernel - Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual function

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: <20210121153043.GA2654954@bjorn-Precision-5520>
Date:   Thu, 21 Jan 2021 09:30:43 -0600
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Chiqijun <chiqijun@...wei.com>,
        Alex Williamson <alex.williamson@...hat.com>
Cc:     "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yinshi (Stone)" <yin.yinshi@...wei.com>,
        "Wangxiaoyun (Cloud)" <cloud.wangxiaoyun@...wei.com>,
        zengweiliang zengweiliang <zengweiliang.zengweiliang@...wei.com>,
        "Chenlizhong (IT Chip)" <chenlizhong@...wei.com>
Subject: Re: [v3] PCI: Add pci reset quirk for Huawei Intelligent NIC virtual
 function

[Alex is a reset expert, hoping he can chime in]

On Thu, Jan 21, 2021 at 08:53:12PM +0800, Chiqijun wrote:
> On 2021/1/9 6:25, Bjorn Helgaas wrote:
> > On Fri, Dec 25, 2020 at 05:25:30PM +0800, Chiqijun wrote:
> > > When multiple VFs do FLR at the same time, the firmware is
> > > processed serially, resulting in some VF FLRs being delayed more
> > > than 100ms, when the virtual machine restarts and the device
> > > driver is loaded, the firmware is doing the corresponding VF
> > > FLR, causing the driver to fail to load.
> > > 
> > > To solve this problem, add host and firmware status synchronization
> > > during FLR.
> > > 
> > > Signed-off-by: Chiqijun <chiqijun@...wei.com>
> > > ...

> > > +	 * Get and check firmware capabilities.
> > > +	 */
> > > +	val = readl(bar + HINIC_VF_FLR_TYPE);
> > > +	if (!(val & (1UL << HINIC_VF_FLR_CAP_BIT_SHIFT))) {
> > > +		pci_iounmap(pdev, bar);
> > > +		return -ENOTTY;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Set the processing bit for the start of FLR, which will be cleared
> > > +	 * by the firmware after FLR is completed.
> > > +	 */
> > > +	val = readl(bar + HINIC_VF_OP);
> > > +	val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
> > > +	writel(val, bar + HINIC_VF_OP);
> > > +
> > > +	/* Perform the actual device function reset */
> > > +	pcie_flr(pdev);
> > > +
> > > +	/*
> > > +	 * The device must learn BDF after FLR in order to respond to BAR's
> > > +	 * read request, therefore, we issue a configure write request to let
> > > +	 * the device capture BDF.
> > > +	 */
> > > +	pci_read_config_word(pdev, PCI_COMMAND, &command);
> > > +	pci_write_config_word(pdev, PCI_COMMAND, command);
> > 
> > I assume this is because of this requirement from PCIe r5.0, sec
> > 2.2.9:
> > 
> >    Functions must capture the Bus and Device Numbers supplied with all
> >    Type 0 Configuration Write Requests completed by the Function, and
> >    supply these numbers in the Bus and Device Number fields of the
> >    Completer ID for all Completions generated by the Device/Function.
> > 
> > I'm a little concerned because it seems like this requirement should
> > apply to *all* resets, and I don't see where we do a similar write
> > following other resets.  Can you help me out?  Do we need this in
> > other cases?  Do we do it?
> 
> This depends on the hardware device. The HINIC device clears the BDF
> information of the VF during FLR, so it relies on Configuration
> Write Requests to capture BDF. If other devices do not clear the DBF
> information during FLR, this operation is not required.

If the spec says devices must keep the latched BDF during FLR, and the
HINIC doesn't comply with that, then it makes sense to do a config
write here in HINIC-specific code.

But if devices are allowed to clear the BDF during FLR, the OS has to
assume they all do, and the generic code for FLR (and probably other
resets) should do a config write so devices can latch the BDF again.

> In addition, I did not find other devices directly access the BAR register
> after FLR in resets.

I didn't catch your meaning here.

If a device loses the BDF during FLR and we don't do something to
allow it to latch the BDF again, any completions from the device will
have the wrong information.  We will likely do *some* config write to
the device eventually, which will fix this, but we can't rely on some
unknown future write to do this.  If it's a problem, we need to
explicitly do a write for this purpose.

Bjorn