Re: [PATCH v1 04/13] PCI/IOV: Allow VF drivers to handle resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI/IOV: bus and slot resets fail to lock VFs on virtfn bus or other slots
- [High] PCI/IOV: VF reset_prepare() before PF reset_prepare() deadlocks hisi_qm FLR
--

commit f7e0f3b0fa0caf7e168b1c2965897910aa159a0a
Author: Michał Winiarski <michal.winiarski@intel.com>

PCI/IOV: Allow VF drivers to handle resets

This commit saves and restores PCI configuration state for all subordinate
VFs across PF resets and notifies VF drivers via reset_prepare() and
reset_done(). It also updates bus and slot resets so that VFs are managed
through their parent PF rather than walked independently on the bus.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 95452c11dea55..ba8553011daea 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5164,6 +5195,17 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  		return;
>  
>  	pci_save_state(dev);
> +}
> +
> +static void pci_dev_save_and_disable(struct pci_dev *dev)
> +{
> +	struct pci_dev *vf;
> +
> +	for_each_pci_vf(dev, vf)
> +		pci_dev_reset_prepare_and_save(vf);
> +
> +	pci_dev_reset_prepare_and_save(dev);

[Severity: High]
Does calling pci_dev_reset_prepare_and_save(vf) on every VF before calling
pci_dev_reset_prepare_and_save(dev) on the PF deadlock HiSilicon QM resets?

During pci_reset_function(pf), pci_dev_save_and_disable() now runs
reset_prepare() on all VFs and then on the PF before pci_dev_restore() calls
reset_done() on any of them.

In drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c,
hisi_acc_vf_pci_reset_prepare() sets QM_RESETTING on the parent PF's
qm->misc_ctl (hisi_acc_vdev->pf_qm) and only clears it in
hisi_acc_vf_pci_aer_reset_done():

    while (test_and_set_bit(QM_RESETTING, &qm->misc_ctl)) {
        msleep(1);
        if (++delay > QM_RESET_WAIT_TIMEOUT) {
            dev_err(dev, "reset prepare failed\n");
            return;
        }
    }

When the first VF sets QM_RESETTING on pf_qm->misc_ctl, any subsequent VF's
hisi_acc_vf_pci_reset_prepare() spins for 60 seconds and fails, and the PF's
hisi_qm_reset_prepare() -> qm_reset_prepare_ready(pf_qm) ->
qm_wait_reset_finish(pf_qm) in drivers/crypto/hisilicon/qm.c also spins on
pf_qm->misc_ctl for ~80 seconds before timing out with -EBUSY ("FLR not
ready!") without stopping the PF.

A similar self-deadlock occurs when VFs are bound to the host hisi_qm drivers
(hisi_hpre, hisi_sec2, hisi_zip) in drivers/crypto/hisilicon/qm.c:

    static int qm_reset_prepare_ready(struct hisi_qm *qm)
    {
        struct pci_dev *pdev = qm->pdev;
        struct hisi_qm *pf_qm = pci_get_drvdata(pci_physfn(pdev));

        if (qm->ver < QM_HW_V3)
            return qm_wait_reset_finish(pf_qm);

        return qm_wait_reset_finish(qm);
    }

On Kunpeng920 (qm->ver < QM_HW_V3), the first VF's hisi_qm_reset_prepare()
sets QM_RESETTING on pf_qm->misc_ctl, blocking subsequent VFs and the PF's
own hisi_qm_reset_prepare(). On Kunpeng930+ (qm->ver >= QM_HW_V3), each VF's
hisi_qm_reset_prepare() sets QM_RESETTING on vf_qm->misc_ctl; when the PF's
hisi_qm_reset_prepare() then calls qm_try_stop_vfs(qm, QM_PF_FLR_PREPARE,
QM_DOWN), each VF's mailbox handler qm_pf_reset_vf_prepare() calls
qm_reset_prepare_ready(vf_qm) -> qm_wait_reset_finish(vf_qm), which waits for
QM_RESETTING on vf_qm->misc_ctl to clear and times out.

[ ... ]

> @@ -5551,6 +5631,11 @@ static int pci_slot_trylock(struct pci_slot *slot)
>  	return __pci_bus_trylock(slot->bus, slot);
>  }
>  
> +static bool pci_dev_skip_bus_reset(struct pci_dev *dev)
> +{
> +	return dev->is_virtfn; /* VFs are managed by their PF */
> +}
> +
>  /*
>   * Save and disable devices from the top of the tree down while holding
>   * the @dev mutex lock for the entire tree.
> @@ -5560,6 +5645,8 @@ static void pci_bus_save_and_disable_locked(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		if (pci_dev_skip_bus_reset(dev))
> +			continue;
>  		pci_dev_save_and_disable(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable_locked(dev->subordinate);
> @@ -5576,6 +5663,8 @@ static void pci_bus_restore_locked(struct pci_bus *bus)
>  	struct pci_dev *dev;
>  
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		if (pci_dev_skip_bus_reset(dev))
> +			continue;
>  		pci_dev_restore(dev);
>  		if (dev->subordinate) {
>  			pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> @@ -5595,6 +5684,8 @@ static void pci_slot_save_and_disable_locked(struct pci_slot *slot)
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> +		if (pci_dev_skip_bus_reset(dev))
> +			continue;
>  		pci_dev_save_and_disable(dev);
>  		if (dev->subordinate)
>  			pci_bus_save_and_disable_locked(dev->subordinate);
> @@ -5613,6 +5704,8 @@ static void pci_slot_restore_locked(struct pci_slot *slot)
>  	list_for_each_entry(dev, &slot->bus->devices, bus_list) {
>  		if (!dev->slot || dev->slot != slot)
>  			continue;
> +		if (pci_dev_skip_bus_reset(dev))
> +			continue;
>  		pci_dev_restore(dev);
>  		if (dev->subordinate) {
>  			pci_bridge_wait_for_secondary_bus(dev, "slot reset");

[Severity: High]
Can pci_bus_save_and_disable_locked(), pci_bus_restore_locked(),
pci_slot_save_and_disable_locked(), and pci_slot_restore_locked() operate on
unlocked VFs when VFs reside on an SR-IOV virtual bus or do not match the
PF's struct pci_slot?

When pci_dev_save_and_disable() and pci_dev_restore() run on a PF, they
iterate over all VFs with for_each_pci_vf(dev, vf) and call
pci_dev_reset_prepare_and_save(vf) and pci_dev_reset_done(vf), both of which
require the VF lock to be held and assert device_lock_assert(&vf->dev).

However, __pci_bus_lock(), __pci_bus_trylock(), and __pci_bus_unlock() in
drivers/pci/pci.c only walk bus->devices (filtered by dev->slot == slot for
slot resets) and recurse into dev->subordinate:

    list_for_each_entry(dev, &bus->devices, bus_list) {
        if (slot && (!dev->slot || dev->slot != slot))
            continue;
        if (dev->subordinate) {
            if (!pci_bus_trylock(dev->subordinate))
                goto unlock;
        } else if (!pci_dev_trylock(dev))
            goto unlock;
    }

First, if any VFs reside on an SR-IOV virtual bus created by virtfn_add_bus()
in drivers/pci/iov.c:

    child = pci_add_new_bus(bus, NULL, busnr);

that virtual bus has bridge == NULL, so pf->subordinate is NULL and those VFs
are on the virtual bus's devices list rather than pf->bus->devices. As a
result, __pci_bus_lock() and __pci_bus_trylock() never lock VFs on the
virtual bus.

Second, during a slot reset (pci_slot_trylock() / pci_slot_lock()),
pci_dev_matches_slot() in drivers/pci/slot.c only assigns dev->slot = slot
when the device matches the slot number:

    static bool pci_dev_matches_slot(struct pci_dev *dev, struct pci_slot *slot)
    {
        if (slot->per_func_slot)
            return dev->devfn == slot->number;

        return slot->number == PCI_SLOT_ALL_DEVICES ||
            PCI_SLOT(dev->devfn) == slot->number;
    }

Any VF with a different PCI_SLOT(vf->devfn) (when slot->number !=
PCI_SLOT_ALL_DEVICES) or a different devfn (when slot->per_func_slot is set)
has vf->slot != slot and is skipped by __pci_bus_trylock(slot->bus, slot),
while pci_slot_save_and_disable_locked() and pci_slot_restore_locked() still
visit all VFs of the PF via for_each_pci_vf(dev, vf) without holding their
device or config access locks.

Should __pci_bus_lock(), __pci_bus_trylock(), and __pci_bus_unlock() also
skip VFs with pci_dev_skip_bus_reset(dev) and lock/unlock VFs via their PF
using pci_dev_lock_vfs(), pci_dev_trylock_vfs(), and pci_dev_unlock_vfs()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019083514.838640-1-lukasz.laguna@intel.com?part=4