<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word"> <div class="WordSection1"> Hi Bartlomiej,<o:p></o:p> Thanks for raising the concern. And Yes, we do test the VFIO_DEVICE_RESET case, by using the test program here<o:p></o:p> <a href="https://gitlab.com/Mellanox/spdk_team/vfio-dmabuf-test/">https://gitlab.com/Mellanox/spdk_team/vfio-dmabuf-test/</a><o:p></o:p> William<o:p></o:p> <o:p> </o:p> <div id="mail-editor-reference-message-container"> <div> <div> <div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in"> From: Bartlomiej Zolnierkiewicz <bartlomiej.zolnierkiewicz@canonical.com> Date: Monday, September 23, 2024 at 6:30 AM To: William Tu <witu@nvidia.com> Cc: kernel-team@lists.ubuntu.com <kernel-team@lists.ubuntu.com>, Vladimir Sokolovsky <vlad@nvidia.com>, Bodong Wang <bodong@nvidia.com>, Sergey Gorenko <sergeygo@nvidia.com>, Jason Gunthorpe <jgg@nvidia.com>, Ziv Waksman <zwaksman@nvidia.com> Subject: Re: [SRU][J:linux-bluefield][PATCH v5 3/6] UBUNTU: SAUCE: vfio_pci: Do not open code pci_try_reset_function()<o:p></o:p> </div> <div> External email: Use caution opening links or attachments On Mon, Sep 16, 2024 at 5:32 PM William Tu <witu@nvidia.com> wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > BugLink: <a href="https://bugs.launchpad.net/bugs/2077887">https://bugs.launchpad.net/bugs/2077887</a> > > 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@nvidia.com> > Signed-off-by: William Tu <witu@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 (<a href="https://lore.kernel.org/all/20240624065552.1572580-1-vivek.kasireddy@intel.com/">https://lore.kernel.org/all/20240624065552.1572580-1-vivek.kasireddy@intel.com/</a>) 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@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<o:p></o:p> </div> </div> </div> </div> </div> </body> </html>