| Message ID | 20240916153057.17319-4-witu@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add VFIO P2P support | expand |
On Mon, Sep 16, 2024 at 5:32 PM William Tu <witu@nvidia.com> wrote: > > From: Jason Gunthorpe <jgg@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@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 (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@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
Hi Bartlomiej, Thanks for raising the concern. And Yes, we do test the VFIO_DEVICE_RESET case, by using the test program here https://gitlab.com/Mellanox/spdk_team/vfio-dmabuf-test/ William 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() 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: 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@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 (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@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
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