On 2021/3/19 4:54, Bjorn Helgaas wrote:
On Tue, Mar 16, 2021 at 10:08:47PM +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.
Nit: VFs do not do FLR; *software* does FLR on a VF. And I think this
is a spec compliance issue specific to the Huawei NIC. I would say
something like "When we do an FLR on several VFs at the same time, the
Huawei Intelligent NIC processes them serially, ..."
OK，will update it at next patch.
"VF FLRs being delayed more than 100ms" does not by itself explain
what the problem is. I'm guessing the problem is that it exceeds the
"msleep(100)" in pcie_flr(), which is based on PCIe r5.0, sec 6.6.2,
which requires:
After an FLR has been initiated by writing a 1b to the Initiate
Function Level Reset bit, the Function must complete the FLR within
100 ms.
So this device is apparently out of spec. Is there an erratum for
this? Please cite it and quote the relevant part here. I want to
avoid having to update this quirk with future device IDs.
We added a description of this problem to the document on the Huawei
support website:
https://support.huawei.com/enterprise/en/doc/EDOC1100063073/87950645/vm-oss-occasionally-fail-to-load-the-in200-driver-when-the-vf-performs-flr
IIUC, VFIO is initiating the FLR, probably as part of assigning the VF
to a VM?
Yes, any process that can trigger multiple VF parallel FLRs has this
problem.
To solve this problem, add host and firmware status synchronization
during FLR.
Signed-off-by: Chiqijun <chiqijun@xxxxxxxxxx>
---
v5:
- Fix build warning reported by kernel test robot
v4:
- Addressed Bjorn's review comments
v3:
- The MSE bit in the VF configuration space is hardwired to zero,
remove the setting of PCI_COMMAND_MEMORY bit. Add comment for
set PCI_COMMAND register.
v2:
- Update comments
- Use the HINIC_VF_FLR_CAP_BIT_SHIFT and HINIC_VF_FLR_PROC_BIT_SHIFT
macro instead of the magic number
---
drivers/pci/quirks.c | 69 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..343890432ba8 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3913,6 +3913,73 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
return 0;
}
+#define PCI_DEVICE_ID_HINIC_VF 0x375E
+#define HINIC_VF_FLR_TYPE 0x1000
+#define HINIC_VF_FLR_CAP_BIT_SHIFT 30
+#define HINIC_VF_OP 0xE80
+#define HINIC_VF_FLR_PROC_BIT_SHIFT 18
+#define HINIC_OPERATION_TIMEOUT 15000 /* 15 seconds */
If you did this:
#define HINIC_VF_FLR_CAP_BIT (1UL << 30)
#define HINIC_VF_FLR_PROC_BIT (1UL << 18)
the code below could be a little more readable, e.g,:
if (!(val & HINIC_VF_FLR_CAP_BIT))
...
val |= HINIC_VF_FLR_PROC_BIT;
Will fix.
+/* Device-specific reset method for Huawei Intelligent NIC virtual functions */
+static int reset_hinic_vf_dev(struct pci_dev *pdev, int probe)
+{
+ unsigned long timeout;
+ void __iomem *bar;
+ u32 val;
+
+ if (probe)
+ return 0;
+
+ bar = pci_iomap(pdev, 0, 0);
+ if (!bar)
+ return -ENOTTY;
+
+ /* Get and check firmware capabilities. */
+ val = ioread32be(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 = ioread32be(bar + HINIC_VF_OP);
+ val = val | (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT);
+ iowrite32be(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.
Will this device capture the bus/device here even though it hasn't
completed the reset? Or does this write need to happen below, after
the device has cleared HINIC_VF_FLR_PROC_BIT?
The FLR processing of Huawei NIC is completed by the cooperation of
hardware and firmware. The hardware can be executed in parallel and
ensured within 100ms, but the firmware processing can only be executed
serially; bus/device clearing and capturing are all completed by
hardware, so it can be directly complete the config write operation here.
+ */
+ pci_write_config_word(pdev, PCI_VENDOR_ID, 0);
+
+ /* Waiting for device reset complete */
+ timeout = jiffies + msecs_to_jiffies(HINIC_OPERATION_TIMEOUT);
+ do {
+ val = ioread32be(bar + HINIC_VF_OP);
+ if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
+ goto reset_complete;
+ msleep(20);
+ } while (time_before(jiffies, timeout));
+
+ val = ioread32be(bar + HINIC_VF_OP);
+ if (!(val & (1UL << HINIC_VF_FLR_PROC_BIT_SHIFT)))
+ goto reset_complete;
+
+ pci_warn(pdev, "Reset dev timeout, flr ack reg: %#010x\n", val);
s/flr/FLR/
Will fix.
+reset_complete:
+ pci_iounmap(pdev, bar);
+
+ return 0;
You return 0 (success) even if the reset timed out. Is that what you
want?
I'd consider adding an "int err" local variable, then doing this so
there's a single cleanup place that does the pci_iounmap() and the
straight-line main path is the non-error one:
int err = 0;
if (!(val & HINIC_VF_FLR_CAP_BIT)) {
err = -ENOTTY;
goto reset_complete;
}
do {
...
} while (time_before(jiffies, timeout));
val = ioread32be(bar + HINIC_VF_OP);
if (val & HINIC_VF_FLR_PROC_BIT) {
pci_warn(pdev, "Reset dev timeout, FLR ack reg: %#010x\n", val);
err = -EBUSY; /* if you want error here; I dunno */
goto reset_complete;
}
/* Let device capture bus/device, per PCIe r5.0, sec 2.2.9 */
pci_write_config_word(pdev, PCI_VENDOR_ID, 0); /* if it goes here? */
reset_complete:
pci_iounmap(pdev, bar);
return err;
Waiting for FLR timeout here means that the firmware has not executed
FLR for a period of time, but eventually FLR will be executed. We expect
PCIE to be initialized after the timeout to ensure that PCIE is normal;
even if VF initialization fails at the beginning, we do not restart the
virtual machine, But after the remove/insmod driver, it can still be
used normally.
Thanks.
+}
+
static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
reset_intel_82599_sfp_virtfn },
@@ -3924,6 +3991,8 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
reset_chelsio_generic_dev },
+ { PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
+ reset_hinic_vf_dev },
{ 0 }
};
--
2.17.1
.
