diff mbox series

[v5] PCI: Add reset quirk for Huawei Intelligent NIC virtual function

Message ID 20210316140847.3326-1-chiqijun@huawei.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series [v5] PCI: Add reset quirk for Huawei Intelligent NIC virtual function | expand

Commit Message

Chiqijun March 16, 2021, 2:08 p.m. UTC
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@huawei.com>
---
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(+)

Comments

Bjorn Helgaas March 18, 2021, 8:54 p.m. UTC | #1
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, ..."

"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.

IIUC, VFIO is initiating the FLR, probably as part of assigning the VF
to a VM?

> To solve this problem, add host and firmware status synchronization
> during FLR.
> 
> Signed-off-by: Chiqijun <chiqijun@huawei.com>
> ---
> 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;

> +/* 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?

> +	 */
> +	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/

> +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;

> +}
> +
>  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
>
Chiqijun March 30, 2021, 12:48 p.m. UTC | #2
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@huawei.com>
>> ---
>> 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
>>
> .
>
diff mbox series

Patch

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 */
+
+/* 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.
+	 */
+	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);
+
+reset_complete:
+	pci_iounmap(pdev, bar);
+
+	return 0;
+}
+
 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 }
 };