| Message ID | 20221110014027.28780-4-ajderossi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Check the device set open count on reset | expand |
Hi DeRossi, On 2022/11/10 09:40, Anthony DeRossi wrote: > vfio_pci_dev_set_needs_reset() inspects the open_count of every device > in the set to determine whether a reset is allowed. The current device > always has open_count == 1 within vfio_pci_core_disable(), effectively > disabling the reset logic. This field is also documented as private in > vfio_device, so it should not be used to determine whether other devices > in the set are open. haven't went through the prior version. maybe may question has been already answered. My question is: the major reason is the order problem in vfio_main.c. close_device() is always called before decreasing open_count to be 0. So even other device has no open fd, the current vfio_device still have one open count. So why can't we just switch the order of open_count-- and close_device()? > Checking for vfio_device_set_open_count() > 1 on the device set fixes > both issues tbh. it's weird to me that a driver needs to know the internal logic of vfio core before knowing it needs to check the vfio_device_set_open_count() in this way. Is vfio-pci the only driver that needs to do this check or there are other drivers? If there are other drivers, maybe fixing the order in core is better. > After commit 2cd8b14aaa66 ("vfio/pci: Move to the device set > infrastructure"), failure to create a new file for a device would cause > the reset to be skipped due to open_count being decremented after > calling close_device() in the error path. > > After commit eadd86f835c6 ("vfio: Remove calls to > vfio_group_add_container_user()"), releasing a device would always skip > the reset due to an ordering change in vfio_device_fops_release(). > > Failing to reset the device leaves it in an unknown state, potentially > causing errors when it is accessed later or bound to a different driver. > > This issue was observed with a Radeon RX Vega 56 [1002:687f] (rev c3) > assigned to a Windows guest. After shutting down the guest, unbinding > the device from vfio-pci, and binding the device to amdgpu: > > [ 548.007102] [drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed! > [ 548.027174] [drm:psp_hw_init [amdgpu]] *ERROR* PSP firmware loading failed > [ 548.027242] [drm:amdgpu_device_fw_loading [amdgpu]] *ERROR* hw_init of IP block <psp> failed -22 > [ 548.027306] amdgpu 0000:0a:00.0: amdgpu: amdgpu_device_ip_init failed > [ 548.027308] amdgpu 0000:0a:00.0: amdgpu: Fatal error during GPU init > > Fixes: 2cd8b14aaa66 ("vfio/pci: Move to the device set infrastructure") > Fixes: eadd86f835c6 ("vfio: Remove calls to vfio_group_add_container_user()") > Signed-off-by: Anthony DeRossi <ajderossi@gmail.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > drivers/vfio/pci/vfio_pci_core.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c > index badc9d828cac..e030c2120183 100644 > --- a/drivers/vfio/pci/vfio_pci_core.c > +++ b/drivers/vfio/pci/vfio_pci_core.c > @@ -2488,12 +2488,12 @@ static bool vfio_pci_dev_set_needs_reset(struct vfio_device_set *dev_set) > struct vfio_pci_core_device *cur; > bool needs_reset = false; > > - list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { > - /* No VFIO device in the set can have an open device FD */ > - if (cur->vdev.open_count) > - return false; > + /* No other VFIO device in the set can be open. */ > + if (vfio_device_set_open_count(dev_set) > 1) > + return false; > + > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > needs_reset |= cur->needs_reset; > - } > return needs_reset; > } >
On Thu, 10 Nov 2022 11:03:29 +0800 Yi Liu <yi.l.liu@intel.com> wrote: > Hi DeRossi, > > On 2022/11/10 09:40, Anthony DeRossi wrote: > > vfio_pci_dev_set_needs_reset() inspects the open_count of every device > > in the set to determine whether a reset is allowed. The current device > > always has open_count == 1 within vfio_pci_core_disable(), effectively > > disabling the reset logic. This field is also documented as private in > > vfio_device, so it should not be used to determine whether other devices > > in the set are open. > > haven't went through the prior version. maybe may question has been already > answered. My question is: > > the major reason is the order problem in vfio_main.c. close_device() is > always called before decreasing open_count to be 0. So even other device > has no open fd, the current vfio_device still have one open count. So why > can't we just switch the order of open_count-- and close_device()? This is what was originally proposed and Jason shot it down: https://lore.kernel.org/all/Y1kY0I4lr7KntbWp@ziepe.ca/ > > Checking for vfio_device_set_open_count() > 1 on the device set fixes > > both issues > tbh. it's weird to me that a driver needs to know the internal logic of > vfio core before knowing it needs to check the vfio_device_set_open_count() > in this way. Is vfio-pci the only driver that needs to do this check or > there are other drivers? If there are other drivers, maybe fixing the order > in core is better. Please see the evolution of reflck into device sets. Both PCI and FSL can have multiple devices in a set, AIUI. The driver defines the set. This ability to test for the last close among devices in the set is a fundamental feature of the original reflck. Thanks, Alex
On 2022/11/10 12:17, Alex Williamson wrote: > On Thu, 10 Nov 2022 11:03:29 +0800 > Yi Liu <yi.l.liu@intel.com> wrote: > >> Hi DeRossi, >> >> On 2022/11/10 09:40, Anthony DeRossi wrote: >>> vfio_pci_dev_set_needs_reset() inspects the open_count of every device >>> in the set to determine whether a reset is allowed. The current device >>> always has open_count == 1 within vfio_pci_core_disable(), effectively >>> disabling the reset logic. This field is also documented as private in >>> vfio_device, so it should not be used to determine whether other devices >>> in the set are open. >> >> haven't went through the prior version. maybe may question has been already >> answered. My question is: >> >> the major reason is the order problem in vfio_main.c. close_device() is >> always called before decreasing open_count to be 0. So even other device >> has no open fd, the current vfio_device still have one open count. So why >> can't we just switch the order of open_count-- and close_device()? > > This is what was originally proposed and Jason shot it down: > > https://lore.kernel.org/all/Y1kY0I4lr7KntbWp@ziepe.ca/ got it. :-) >>> Checking for vfio_device_set_open_count() > 1 on the device set fixes >>> both issues >> tbh. it's weird to me that a driver needs to know the internal logic of >> vfio core before knowing it needs to check the vfio_device_set_open_count() >> in this way. Is vfio-pci the only driver that needs to do this check or >> there are other drivers? If there are other drivers, maybe fixing the order >> in core is better. > > Please see the evolution of reflck into device sets. Both PCI and FSL > can have multiple devices in a set, AIUI. The driver defines the set. > This ability to test for the last close among devices in the set is a > fundamental feature of the original reflck. Thanks, thanks for the info.
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index badc9d828cac..e030c2120183 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -2488,12 +2488,12 @@ static bool vfio_pci_dev_set_needs_reset(struct vfio_device_set *dev_set) struct vfio_pci_core_device *cur; bool needs_reset = false; - list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) { - /* No VFIO device in the set can have an open device FD */ - if (cur->vdev.open_count) - return false; + /* No other VFIO device in the set can be open. */ + if (vfio_device_set_open_count(dev_set) > 1) + return false; + + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) needs_reset |= cur->needs_reset; - } return needs_reset; }
vfio_pci_dev_set_needs_reset() inspects the open_count of every device in the set to determine whether a reset is allowed. The current device always has open_count == 1 within vfio_pci_core_disable(), effectively disabling the reset logic. This field is also documented as private in vfio_device, so it should not be used to determine whether other devices in the set are open. Checking for vfio_device_set_open_count() > 1 on the device set fixes both issues. After commit 2cd8b14aaa66 ("vfio/pci: Move to the device set infrastructure"), failure to create a new file for a device would cause the reset to be skipped due to open_count being decremented after calling close_device() in the error path. After commit eadd86f835c6 ("vfio: Remove calls to vfio_group_add_container_user()"), releasing a device would always skip the reset due to an ordering change in vfio_device_fops_release(). Failing to reset the device leaves it in an unknown state, potentially causing errors when it is accessed later or bound to a different driver. This issue was observed with a Radeon RX Vega 56 [1002:687f] (rev c3) assigned to a Windows guest. After shutting down the guest, unbinding the device from vfio-pci, and binding the device to amdgpu: [ 548.007102] [drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed! [ 548.027174] [drm:psp_hw_init [amdgpu]] *ERROR* PSP firmware loading failed [ 548.027242] [drm:amdgpu_device_fw_loading [amdgpu]] *ERROR* hw_init of IP block <psp> failed -22 [ 548.027306] amdgpu 0000:0a:00.0: amdgpu: amdgpu_device_ip_init failed [ 548.027308] amdgpu 0000:0a:00.0: amdgpu: Fatal error during GPU init Fixes: 2cd8b14aaa66 ("vfio/pci: Move to the device set infrastructure") Fixes: eadd86f835c6 ("vfio: Remove calls to vfio_group_add_container_user()") Signed-off-by: Anthony DeRossi <ajderossi@gmail.com> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> --- drivers/vfio/pci/vfio_pci_core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)