[SRU][J:linux-bluefield][PATCH v5 3/6] UBUNTU: SAUCE: vfio_pci: Do not open code pci_try_reset_function()
Bartlomiej Zolnierkiewicz
bartlomiej.zolnierkiewicz at canonical.com
Mon Sep 23 13:30:16 UTC 2024
On Mon, Sep 16, 2024 at 5:32 PM William Tu <witu at nvidia.com> wrote:
>
> From: Jason Gunthorpe <jgg at nvidia.com>
>
> BugLink: https://bugs.launchpad.net/bugs/2077887
>
> FLR triggered by an emulated config space write should not behave
> differently from a FLR triggered by VFIO_DEVICE_RESET, currently the
> config space path misses the power management.
>
> Consolidate all the call sites to invoke a single function.
>
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
> Signed-off-by: William Tu <witu at nvidia.com>
I did some more research and this patch seems to introduce user-space
visible changes in behaviour for VFIO PCI devices with the
vdev->reset_works flag set. Also the recent upstream (re)submission of
this feature from Vivek Kasireddy
(https://lore.kernel.org/all/20240624065552.1572580-1-vivek.kasireddy@intel.com/)
has dropped this change. Maybe I'm worrying too much but has this been
actually tested with vdev->reset_works == 1 devices? If not and the
patch served only a cleanup purpose then I think that it should be
dropped (vfio_pci_set_power_state() call was re-added in v5 only so it
also doesn't seem that it was actually needed/tested for devices which
have NoSoftRst-).
--
Best regards,
Bartlomiej
> ---
> drivers/vfio/pci/vfio_pci_config.c | 14 +++-----
> drivers/vfio/pci/vfio_pci_core.c | 54 +++++++++++++++++-------------
> drivers/vfio/pci/vfio_pci_priv.h | 7 ++++
> 3 files changed, 41 insertions(+), 34 deletions(-)
> create mode 100644 drivers/vfio/pci/vfio_pci_priv.h
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 6e58b4bf7a60..78c626b8907d 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -857,11 +857,8 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
> pos - offset + PCI_EXP_DEVCAP,
> &cap);
>
> - if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
> - vfio_pci_zap_and_down_write_memory_lock(vdev);
> - pci_try_reset_function(vdev->pdev);
> - up_write(&vdev->memory_lock);
> - }
> + if (!ret && (cap & PCI_EXP_DEVCAP_FLR))
> + vfio_pci_try_reset_function(vdev);
> }
>
> /*
> @@ -939,11 +936,8 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
> pos - offset + PCI_AF_CAP,
> &cap);
>
> - if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
> - vfio_pci_zap_and_down_write_memory_lock(vdev);
> - pci_try_reset_function(vdev->pdev);
> - up_write(&vdev->memory_lock);
> - }
> + if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP))
> + vfio_pci_try_reset_function(vdev);
> }
>
> return count;
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 4f1aceadd4fb..a45cdd3cfb84 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -29,6 +29,8 @@
>
> #include <linux/vfio_pci_core.h>
>
> +#include "vfio_pci_priv.h"
> +
> #define DRIVER_AUTHOR "Alex Williamson <alex.williamson at redhat.com>"
> #define DRIVER_DESC "core driver for VFIO based PCI devices"
>
> @@ -648,6 +650,33 @@ int vfio_pci_register_dev_region(struct vfio_pci_core_device *vdev,
> }
> EXPORT_SYMBOL_GPL(vfio_pci_register_dev_region);
>
> +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev)
> +{
> + int ret;
> +
> + if (!vdev->reset_works)
> + return -EINVAL;
> +
> + vfio_pci_zap_and_down_write_memory_lock(vdev);
> +
> + /*
> + * This function can be invoked while the power state is non-D0.
> + * If pci_try_reset_function() has been called while the power
> + * state is non-D0, then pci_try_reset_function() will
> + * internally set the power state to D0 without vfio driver
> + * involvement. For the devices which have NoSoftRst-, the
> + * reset function can cause the PCI config space reset without
> + * restoring the original state (saved locally in
> + * 'vdev->pm_save').
> + */
> + vfio_pci_set_power_state(vdev, PCI_D0);
> +
> + ret = pci_try_reset_function(vdev->pdev);
> + up_write(&vdev->memory_lock);
> +
> + return ret;
> +}
> +
> long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
> unsigned long arg)
> {
> @@ -929,30 +958,7 @@ long vfio_pci_core_ioctl(struct vfio_device *core_vdev, unsigned int cmd,
> return ret;
>
> } else if (cmd == VFIO_DEVICE_RESET) {
> - int ret;
> -
> - if (!vdev->reset_works)
> - return -EINVAL;
> -
> - vfio_pci_zap_and_down_write_memory_lock(vdev);
> -
> - /*
> - * This function can be invoked while the power state is non-D0.
> - * If pci_try_reset_function() has been called while the power
> - * state is non-D0, then pci_try_reset_function() will
> - * internally set the power state to D0 without vfio driver
> - * involvement. For the devices which have NoSoftRst-, the
> - * reset function can cause the PCI config space reset without
> - * restoring the original state (saved locally in
> - * 'vdev->pm_save').
> - */
> - vfio_pci_set_power_state(vdev, PCI_D0);
> -
> - ret = pci_try_reset_function(vdev->pdev);
> - up_write(&vdev->memory_lock);
> -
> - return ret;
> -
> + return vfio_pci_try_reset_function(vdev);
> } else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
> struct vfio_pci_hot_reset_info hdr;
> struct vfio_pci_fill_info fill = { 0 };
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> new file mode 100644
> index 000000000000..4971cd95b431
> --- /dev/null
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef VFIO_PCI_PRIV_H
> +#define VFIO_PCI_PRIV_H
> +
> +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev);
> +
> +#endif
More information about the kernel-team
mailing list