* [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call
@ 2022-09-29 17:48 Jason Gunthorpe
2022-09-29 20:04 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Jason Gunthorpe @ 2022-09-29 17:48 UTC (permalink / raw)
To: David Airlie, Daniel Vetter, dri-devel, intel-gfx, intel-gvt-dev,
Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
Zhenyu Wang
Cc: Christoph Hellwig, stable
When converting to directly create the vfio_device the mdev driver has to
put a vfio_register_emulated_iommu_dev() in the probe() and a pairing
vfio_unregister_group_dev() in the remove.
This was missed for gvt, add it.
Cc: stable@vger.kernel.org
Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use vfio_register_emulated_iommu_dev")
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/gpu/drm/i915/gvt/kvmgt.c | 1 +
1 file changed, 1 insertion(+)
Should go through Alex's tree.
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 41bba40feef8f4..9003145adb5a93 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1615,6 +1615,7 @@ static void intel_vgpu_remove(struct mdev_device *mdev)
if (WARN_ON_ONCE(vgpu->attached))
return;
+ vfio_unregister_group_dev(&vgpu->vfio_device);
vfio_put_device(&vgpu->vfio_device);
}
base-commit: c72e0034e6d4c36322d958b997d11d2627c6056c
--
2.37.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe @ 2022-09-29 20:04 ` Patchwork 2022-09-30 1:47 ` [Intel-gfx] [PATCH] " Tian, Kevin ` (5 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2022-09-29 20:04 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: intel-gfx == Series Details == Series: drm/i915/gvt: Add missing vfio_unregister_group_dev() call URL : https://patchwork.freedesktop.org/series/109264/ State : failure == Summary == Error: patch https://patchwork.freedesktop.org/api/1.0/series/109264/revisions/1/mbox/ not applied Applying: drm/i915/gvt: Add missing vfio_unregister_group_dev() call Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/gvt/kvmgt.c Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/gvt/kvmgt.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gvt/kvmgt.c error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 drm/i915/gvt: Add missing vfio_unregister_group_dev() call When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe 2022-09-29 20:04 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork @ 2022-09-30 1:47 ` Tian, Kevin 2022-09-30 6:46 ` Christoph Hellwig ` (4 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Tian, Kevin @ 2022-09-30 1:47 UTC (permalink / raw) To: Jason Gunthorpe, David Airlie, Daniel Vetter, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, intel-gvt-dev@lists.freedesktop.org, Jani Nikula, Joonas Lahtinen, Vivi, Rodrigo, Tvrtko Ursulin, Zhenyu Wang Cc: stable@vger.kernel.org, Christoph Hellwig > From: Jason Gunthorpe > Sent: Friday, September 30, 2022 1:49 AM > > When converting to directly create the vfio_device the mdev driver has to > put a vfio_register_emulated_iommu_dev() in the probe() and a pairing > vfio_unregister_group_dev() in the remove. > > This was missed for gvt, add it. > > Cc: stable@vger.kernel.org > Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use > vfio_register_emulated_iommu_dev") > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe 2022-09-29 20:04 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork 2022-09-30 1:47 ` [Intel-gfx] [PATCH] " Tian, Kevin @ 2022-09-30 6:46 ` Christoph Hellwig 2022-09-30 21:31 ` Alex Williamson ` (3 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2022-09-30 6:46 UTC (permalink / raw) To: Jason Gunthorpe Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig Oops. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe ` (2 preceding siblings ...) 2022-09-30 6:46 ` Christoph Hellwig @ 2022-09-30 21:31 ` Alex Williamson 2022-10-05 20:17 ` Alex Williamson ` (2 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Alex Williamson @ 2022-09-30 21:31 UTC (permalink / raw) To: Jason Gunthorpe Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Thu, 29 Sep 2022 14:48:35 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > When converting to directly create the vfio_device the mdev driver has to > put a vfio_register_emulated_iommu_dev() in the probe() and a pairing > vfio_unregister_group_dev() in the remove. > > This was missed for gvt, add it. > > Cc: stable@vger.kernel.org > Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use vfio_register_emulated_iommu_dev") > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > 1 file changed, 1 insertion(+) > > Should go through Alex's tree. Applied to vfio next branch for v6.1. Thanks for the quick fix! Alex > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > index 41bba40feef8f4..9003145adb5a93 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -1615,6 +1615,7 @@ static void intel_vgpu_remove(struct mdev_device *mdev) > if (WARN_ON_ONCE(vgpu->attached)) > return; > > + vfio_unregister_group_dev(&vgpu->vfio_device); > vfio_put_device(&vgpu->vfio_device); > } > > > base-commit: c72e0034e6d4c36322d958b997d11d2627c6056c ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe ` (3 preceding siblings ...) 2022-09-30 21:31 ` Alex Williamson @ 2022-10-05 20:17 ` Alex Williamson 2022-10-05 22:03 ` Alex Williamson 2022-10-06 11:35 ` Jason Gunthorpe 2022-10-05 21:16 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) Patchwork 2022-10-05 21:38 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 6 siblings, 2 replies; 15+ messages in thread From: Alex Williamson @ 2022-10-05 20:17 UTC (permalink / raw) To: Jason Gunthorpe Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Thu, 29 Sep 2022 14:48:35 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > When converting to directly create the vfio_device the mdev driver has to > put a vfio_register_emulated_iommu_dev() in the probe() and a pairing > vfio_unregister_group_dev() in the remove. > > This was missed for gvt, add it. > > Cc: stable@vger.kernel.org > Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use vfio_register_emulated_iommu_dev") > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > 1 file changed, 1 insertion(+) > > Should go through Alex's tree. > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > index 41bba40feef8f4..9003145adb5a93 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -1615,6 +1615,7 @@ static void intel_vgpu_remove(struct mdev_device *mdev) > if (WARN_ON_ONCE(vgpu->attached)) > return; > > + vfio_unregister_group_dev(&vgpu->vfio_device); > vfio_put_device(&vgpu->vfio_device); > } > > > base-commit: c72e0034e6d4c36322d958b997d11d2627c6056c This is marked for stable, but I think the stable backport for existing kernels is actually: diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index e3cd58946477..de89946c4817 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1595,6 +1595,9 @@ static void intel_vgpu_remove(struct mdev_device *mdev) if (WARN_ON_ONCE(vgpu->attached)) return; + + vfio_unregister_group_dev(&vgpu->vfio_device); + vfio_uninit_group_dev(&vgpu->vfio_device); intel_gvt_destroy_vgpu(vgpu); } ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-05 20:17 ` Alex Williamson @ 2022-10-05 22:03 ` Alex Williamson 2022-10-06 11:37 ` Jason Gunthorpe 2022-10-06 11:35 ` Jason Gunthorpe 1 sibling, 1 reply; 15+ messages in thread From: Alex Williamson @ 2022-10-05 22:03 UTC (permalink / raw) To: Jason Gunthorpe Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Wed, 5 Oct 2022 14:17:17 -0600 Alex Williamson <alex.williamson@redhat.com> wrote: > On Thu, 29 Sep 2022 14:48:35 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > When converting to directly create the vfio_device the mdev driver has to > > put a vfio_register_emulated_iommu_dev() in the probe() and a pairing > > vfio_unregister_group_dev() in the remove. > > > > This was missed for gvt, add it. > > > > Cc: stable@vger.kernel.org > > Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use vfio_register_emulated_iommu_dev") > > Reported-by: Alex Williamson <alex.williamson@redhat.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > > 1 file changed, 1 insertion(+) > > > > Should go through Alex's tree. > > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > > index 41bba40feef8f4..9003145adb5a93 100644 > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > > @@ -1615,6 +1615,7 @@ static void intel_vgpu_remove(struct mdev_device *mdev) > > if (WARN_ON_ONCE(vgpu->attached)) > > return; Actually, what's the purpose of this ^^^^ ? We can't have a .remove callback that does nothing, this breaks removing the device while it's in use. Once we have the vfio_unregister_group_dev() fix below, we'll block until the device is unused, at which point vgpu->attached becomes false. Unless I'm missing something, I think we should also follow-up with a patch to remove that bogus warn-on branch, right? Thanks, Alex > > > > + vfio_unregister_group_dev(&vgpu->vfio_device); > > vfio_put_device(&vgpu->vfio_device); > > } > > > > > > base-commit: c72e0034e6d4c36322d958b997d11d2627c6056c > > This is marked for stable, but I think the stable backport for > existing kernels is actually: > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > index e3cd58946477..de89946c4817 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -1595,6 +1595,9 @@ static void intel_vgpu_remove(struct mdev_device *mdev) > > if (WARN_ON_ONCE(vgpu->attached)) > return; > + > + vfio_unregister_group_dev(&vgpu->vfio_device); > + vfio_uninit_group_dev(&vgpu->vfio_device); > intel_gvt_destroy_vgpu(vgpu); > } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-05 22:03 ` Alex Williamson @ 2022-10-06 11:37 ` Jason Gunthorpe 2022-10-06 18:31 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Jason Gunthorpe @ 2022-10-06 11:37 UTC (permalink / raw) To: Alex Williamson Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Wed, Oct 05, 2022 at 04:03:56PM -0600, Alex Williamson wrote: > We can't have a .remove callback that does nothing, this breaks > removing the device while it's in use. Once we have the > vfio_unregister_group_dev() fix below, we'll block until the device is > unused, at which point vgpu->attached becomes false. Unless I'm > missing something, I think we should also follow-up with a patch to > remove that bogus warn-on branch, right? Thanks, Yes, looks right to me. I question all the logical arround attached, where is the locking? Jason ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-06 11:37 ` Jason Gunthorpe @ 2022-10-06 18:31 ` Alex Williamson 2022-10-10 23:14 ` Tian, Kevin 2022-10-19 9:40 ` Wang, Zhi A 0 siblings, 2 replies; 15+ messages in thread From: Alex Williamson @ 2022-10-06 18:31 UTC (permalink / raw) To: Zhenyu Wang, Zhi Wang, Tian, Kevin Cc: Jason Gunthorpe, David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Thu, 6 Oct 2022 08:37:09 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Wed, Oct 05, 2022 at 04:03:56PM -0600, Alex Williamson wrote: > > We can't have a .remove callback that does nothing, this breaks > > removing the device while it's in use. Once we have the > > vfio_unregister_group_dev() fix below, we'll block until the device is > > unused, at which point vgpu->attached becomes false. Unless I'm > > missing something, I think we should also follow-up with a patch to > > remove that bogus warn-on branch, right? Thanks, > > Yes, looks right to me. > > I question all the logical arround attached, where is the locking? Zhenyu, Zhi, Kevin, Could someone please take a look at use of vgpu->attached in the GVT-g driver? It's use in intel_vgpu_remove() is bogus, the .release callback needs to use vfio_unregister_group_dev() to wait for the device to be unused. The WARN_ON/return here breaks all future use of the device. I assume @attached has something to do with the page table interface with KVM, but it all looks racy anyway. Also, whatever purpose vgpu->released served looks unnecessary now. Thanks, Alex ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-06 18:31 ` Alex Williamson @ 2022-10-10 23:14 ` Tian, Kevin 2022-10-19 9:40 ` Wang, Zhi A 1 sibling, 0 replies; 15+ messages in thread From: Tian, Kevin @ 2022-10-10 23:14 UTC (permalink / raw) To: Alex Williamson, Zhenyu Wang, Wang, Zhi A Cc: Jason Gunthorpe, David Airlie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Daniel Vetter, Vivi, Rodrigo, stable@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org, Christoph Hellwig > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Friday, October 7, 2022 2:31 AM > > On Thu, 6 Oct 2022 08:37:09 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Wed, Oct 05, 2022 at 04:03:56PM -0600, Alex Williamson wrote: > > > We can't have a .remove callback that does nothing, this breaks > > > removing the device while it's in use. Once we have the > > > vfio_unregister_group_dev() fix below, we'll block until the device is > > > unused, at which point vgpu->attached becomes false. Unless I'm > > > missing something, I think we should also follow-up with a patch to > > > remove that bogus warn-on branch, right? Thanks, > > > > Yes, looks right to me. > > > > I question all the logical arround attached, where is the locking? > > Zhenyu, Zhi, Kevin, > > Could someone please take a look at use of vgpu->attached in the GVT-g > driver? It's use in intel_vgpu_remove() is bogus, the .release > callback needs to use vfio_unregister_group_dev() to wait for the > device to be unused. The WARN_ON/return here breaks all future use of > the device. I assume @attached has something to do with the page table > interface with KVM, but it all looks racy anyway. > > Also, whatever purpose vgpu->released served looks unnecessary now. > Thanks, > Zhi is looking at it. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-06 18:31 ` Alex Williamson 2022-10-10 23:14 ` Tian, Kevin @ 2022-10-19 9:40 ` Wang, Zhi A 2022-10-19 10:13 ` Tian, Kevin 1 sibling, 1 reply; 15+ messages in thread From: Wang, Zhi A @ 2022-10-19 9:40 UTC (permalink / raw) To: Alex Williamson, Zhenyu Wang, Tian, Kevin Cc: Jason Gunthorpe, David Airlie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Daniel Vetter, Vivi, Rodrigo, stable@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org, Christoph Hellwig On 10/6/22 18:31, Alex Williamson wrote: > On Thu, 6 Oct 2022 08:37:09 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > >> On Wed, Oct 05, 2022 at 04:03:56PM -0600, Alex Williamson wrote: >>> We can't have a .remove callback that does nothing, this breaks >>> removing the device while it's in use. Once we have the >>> vfio_unregister_group_dev() fix below, we'll block until the device is >>> unused, at which point vgpu->attached becomes false. Unless I'm >>> missing something, I think we should also follow-up with a patch to >>> remove that bogus warn-on branch, right? Thanks, >> >> Yes, looks right to me. >> >> I question all the logical arround attached, where is the locking? > > Zhenyu, Zhi, Kevin, > > Could someone please take a look at use of vgpu->attached in the GVT-g > driver? It's use in intel_vgpu_remove() is bogus, the .release > callback needs to use vfio_unregister_group_dev() to wait for the > device to be unused. The WARN_ON/return here breaks all future use of > the device. I assume @attached has something to do with the page table > interface with KVM, but it all looks racy anyway. > Thanks for pointing this out. It was introduced in the GVT-g refactor patch series and Christoph might not want to touch the vgpu->released while he needed a new state. I dig it a bit. vgpu->attached would be used for preventing multiple open on a single vGPU and indicate the kvm_get_kvm() has been done. vgpu->released was to prevent the release before close, which is now handled by the vfio_device_*. What I would like to do are: 1) Remove the vgpu->released. 2) Use alock to protect vgpu->attached. After those were solved, the WARN_ON/return in the intel_vgpu_remove() should be safely removed as the .release will be called after .close_device deceases the vfio_device->refcnt to zero. Thanks, Zhi. > Also, whatever purpose vgpu->released served looks unnecessary now. > Thanks, > > Alex > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-19 9:40 ` Wang, Zhi A @ 2022-10-19 10:13 ` Tian, Kevin 0 siblings, 0 replies; 15+ messages in thread From: Tian, Kevin @ 2022-10-19 10:13 UTC (permalink / raw) To: Wang, Zhi A, Alex Williamson, Zhenyu Wang Cc: Jason Gunthorpe, David Airlie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Daniel Vetter, Vivi, Rodrigo, stable@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org, Christoph Hellwig > From: Wang, Zhi A <zhi.a.wang@intel.com> > Sent: Wednesday, October 19, 2022 5:41 PM > > On 10/6/22 18:31, Alex Williamson wrote: > > On Thu, 6 Oct 2022 08:37:09 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > >> On Wed, Oct 05, 2022 at 04:03:56PM -0600, Alex Williamson wrote: > >>> We can't have a .remove callback that does nothing, this breaks > >>> removing the device while it's in use. Once we have the > >>> vfio_unregister_group_dev() fix below, we'll block until the device is > >>> unused, at which point vgpu->attached becomes false. Unless I'm > >>> missing something, I think we should also follow-up with a patch to > >>> remove that bogus warn-on branch, right? Thanks, > >> > >> Yes, looks right to me. > >> > >> I question all the logical arround attached, where is the locking? > > > > Zhenyu, Zhi, Kevin, > > > > Could someone please take a look at use of vgpu->attached in the GVT-g > > driver? It's use in intel_vgpu_remove() is bogus, the .release > > callback needs to use vfio_unregister_group_dev() to wait for the > > device to be unused. The WARN_ON/return here breaks all future use of > > the device. I assume @attached has something to do with the page table > > interface with KVM, but it all looks racy anyway. > > > Thanks for pointing this out. > > It was introduced in the GVT-g refactor patch series and Christoph might > not want to touch the vgpu->released while he needed a new state. > > I dig it a bit. vgpu->attached would be used for preventing multiple open > on a single vGPU and indicate the kvm_get_kvm() has been done. vfio core already ensures that .open_device() is called only once: vfio_device_open() { ... mutex_lock(&device->dev_set->lock); device->open_count++; if (device->open_count == 1) { ... if (device->ops->open_device) { ret = device->ops->open_device(device); ... } > vgpu->released was to prevent the release before close, which is now > handled by the vfio_device_*. > > What I would like to do are: > 1) Remove the vgpu->released. 2) Use alock to protect vgpu->attached. > > After those were solved, the WARN_ON/return in the intel_vgpu_remove() > should be safely removed as the .release will be called after .close_device > deceases the vfio_device->refcnt to zero. > > Thanks, > Zhi. > > > Also, whatever purpose vgpu->released served looks unnecessary now. > > Thanks, > > > > Alex > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call 2022-10-05 20:17 ` Alex Williamson 2022-10-05 22:03 ` Alex Williamson @ 2022-10-06 11:35 ` Jason Gunthorpe 1 sibling, 0 replies; 15+ messages in thread From: Jason Gunthorpe @ 2022-10-06 11:35 UTC (permalink / raw) To: Alex Williamson Cc: David Airlie, intel-gfx, dri-devel, Daniel Vetter, Rodrigo Vivi, stable, intel-gvt-dev, Christoph Hellwig On Wed, Oct 05, 2022 at 02:17:17PM -0600, Alex Williamson wrote: > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > > index 41bba40feef8f4..9003145adb5a93 100644 > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > > @@ -1615,6 +1615,7 @@ static void intel_vgpu_remove(struct mdev_device *mdev) > > if (WARN_ON_ONCE(vgpu->attached)) > > return; > > > > + vfio_unregister_group_dev(&vgpu->vfio_device); > > vfio_put_device(&vgpu->vfio_device); > > } > > > > > > base-commit: c72e0034e6d4c36322d958b997d11d2627c6056c > > This is marked for stable, but I think the stable backport for > existing kernels is actually: Yes probably, this patch won't apply so if anyone wants to see it in the stable series they need to follow the process to send the reworked version. Jason ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe ` (4 preceding siblings ...) 2022-10-05 20:17 ` Alex Williamson @ 2022-10-05 21:16 ` Patchwork 2022-10-05 21:38 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 6 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2022-10-05 21:16 UTC (permalink / raw) To: Alex Williamson; +Cc: intel-gfx == Series Details == Series: drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) URL : https://patchwork.freedesktop.org/series/109264/ State : warning == Summary == Error: dim checkpatch failed d1cb20a63051 drm/i915/gvt: Add missing vfio_unregister_group_dev() call -:16: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line) #16: > Fixes: 978cf586ac35 ("drm/i915/gvt: convert to use vfio_register_emulated_iommu_dev") -:56: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s) total: 1 errors, 1 warnings, 0 checks, 9 lines checked ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe ` (5 preceding siblings ...) 2022-10-05 21:16 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) Patchwork @ 2022-10-05 21:38 ` Patchwork 6 siblings, 0 replies; 15+ messages in thread From: Patchwork @ 2022-10-05 21:38 UTC (permalink / raw) To: Alex Williamson; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 11015 bytes --] == Series Details == Series: drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) URL : https://patchwork.freedesktop.org/series/109264/ State : failure == Summary == CI Bug Log - changes from CI_DRM_12219 -> Patchwork_109264v2 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_109264v2 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_109264v2, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/index.html Participating hosts (42 -> 42) ------------------------------ Additional (2): fi-hsw-4770 fi-rkl-11600 Missing (2): fi-ctg-p8600 fi-hsw-4200u Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_109264v2: ### IGT changes ### #### Possible regressions #### * igt@gem_render_tiled_blits@basic: - fi-apl-guc: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/fi-apl-guc/igt@gem_render_tiled_blits@basic.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-apl-guc/igt@gem_render_tiled_blits@basic.html Known issues ------------ Here are the changes found in Patchwork_109264v2 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_huc_copy@huc-copy: - fi-rkl-11600: NOTRUN -> [SKIP][3] ([i915#2190]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html * igt@gem_lmem_swapping@basic: - fi-rkl-11600: NOTRUN -> [SKIP][4] ([i915#4613]) +3 similar issues [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@gem_lmem_swapping@basic.html * igt@gem_softpin@allocator-basic-reserve: - fi-hsw-4770: NOTRUN -> [SKIP][5] ([fdo#109271]) +9 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-hsw-4770/igt@gem_softpin@allocator-basic-reserve.html * igt@gem_tiled_pread_basic: - fi-rkl-11600: NOTRUN -> [SKIP][6] ([i915#3282]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@gem_tiled_pread_basic.html * igt@i915_pm_backlight@basic-brightness: - fi-hsw-4770: NOTRUN -> [SKIP][7] ([fdo#109271] / [i915#3012]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-hsw-4770/igt@i915_pm_backlight@basic-brightness.html - fi-rkl-11600: NOTRUN -> [SKIP][8] ([i915#3012]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html * igt@i915_pm_rpm@module-reload: - fi-cfl-8109u: [PASS][9] -> [DMESG-FAIL][10] ([i915#62]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html * igt@i915_selftest@live@late_gt_pm: - fi-cfl-8109u: [PASS][11] -> [DMESG-WARN][12] ([i915#5904]) +30 similar issues [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html * igt@i915_suspend@basic-s2idle-without-i915: - fi-cfl-8109u: [PASS][13] -> [DMESG-WARN][14] ([i915#5904] / [i915#62]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/fi-cfl-8109u/igt@i915_suspend@basic-s2idle-without-i915.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-cfl-8109u/igt@i915_suspend@basic-s2idle-without-i915.html * igt@i915_suspend@basic-s3-without-i915: - fi-rkl-11600: NOTRUN -> [INCOMPLETE][15] ([i915#5982]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html * igt@kms_chamelium@dp-crc-fast: - fi-hsw-4770: NOTRUN -> [SKIP][16] ([fdo#109271] / [fdo#111827]) +8 similar issues [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-hsw-4770/igt@kms_chamelium@dp-crc-fast.html * igt@kms_chamelium@hdmi-edid-read: - fi-rkl-11600: NOTRUN -> [SKIP][17] ([fdo#111827]) +7 similar issues [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@kms_chamelium@hdmi-edid-read.html * igt@kms_cursor_legacy@basic-busy-flip-before-cursor: - fi-rkl-11600: NOTRUN -> [SKIP][18] ([i915#4103]) [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html * igt@kms_force_connector_basic@force-load-detect: - fi-rkl-11600: NOTRUN -> [SKIP][19] ([fdo#109285] / [i915#4098]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html * igt@kms_frontbuffer_tracking@basic: - fi-cfl-8109u: [PASS][20] -> [DMESG-WARN][21] ([i915#62]) +12 similar issues [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html * igt@kms_psr@primary_page_flip: - fi-rkl-11600: NOTRUN -> [SKIP][22] ([i915#1072]) +3 similar issues [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@kms_psr@primary_page_flip.html * igt@kms_psr@sprite_plane_onoff: - fi-hsw-4770: NOTRUN -> [SKIP][23] ([fdo#109271] / [i915#1072]) +3 similar issues [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-hsw-4770/igt@kms_psr@sprite_plane_onoff.html * igt@kms_setmode@basic-clone-single-crtc: - fi-rkl-11600: NOTRUN -> [SKIP][24] ([i915#3555] / [i915#4098]) [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html * igt@prime_vgem@basic-read: - fi-rkl-11600: NOTRUN -> [SKIP][25] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@prime_vgem@basic-read.html * igt@prime_vgem@basic-userptr: - fi-rkl-11600: NOTRUN -> [SKIP][26] ([fdo#109295] / [i915#3301] / [i915#3708]) [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/fi-rkl-11600/igt@prime_vgem@basic-userptr.html #### Possible fixes #### * igt@gem_exec_suspend@basic-s0@smem: - {bat-rplp-1}: [DMESG-WARN][27] ([i915#2867]) -> [PASS][28] [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/bat-rplp-1/igt@gem_exec_suspend@basic-s0@smem.html - {bat-adlm-1}: [DMESG-WARN][29] ([i915#2867]) -> [PASS][30] [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/bat-adlm-1/igt@gem_exec_suspend@basic-s0@smem.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/bat-adlm-1/igt@gem_exec_suspend@basic-s0@smem.html * igt@i915_module_load@reload: - {bat-rpls-2}: [DMESG-WARN][31] ([i915#5537]) -> [PASS][32] +1 similar issue [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/bat-rpls-2/igt@i915_module_load@reload.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/bat-rpls-2/igt@i915_module_load@reload.html * igt@i915_selftest@live@requests: - {bat-rpls-1}: [INCOMPLETE][33] ([i915#6257]) -> [PASS][34] [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/bat-rpls-1/igt@i915_selftest@live@requests.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/bat-rpls-1/igt@i915_selftest@live@requests.html * igt@i915_selftest@live@reset: - {bat-rpls-2}: [DMESG-FAIL][35] ([i915#4983]) -> [PASS][36] [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12219/bat-rpls-2/igt@i915_selftest@live@reset.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/bat-rpls-2/igt@i915_selftest@live@reset.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285 [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295 [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827 [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072 [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190 [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582 [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867 [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012 [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282 [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291 [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301 [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555 [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708 [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098 [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5537]: https://gitlab.freedesktop.org/drm/intel/issues/5537 [i915#5904]: https://gitlab.freedesktop.org/drm/intel/issues/5904 [i915#5982]: https://gitlab.freedesktop.org/drm/intel/issues/5982 [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62 [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6559]: https://gitlab.freedesktop.org/drm/intel/issues/6559 [i915#6818]: https://gitlab.freedesktop.org/drm/intel/issues/6818 Build changes ------------- * Linux: CI_DRM_12219 -> Patchwork_109264v2 CI-20190529: 20190529 CI_DRM_12219: a6282eed0f613f3230a53a1826ac913edc0a65e4 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_6683: 5de4065922af981366357df5a94caf4e8d15c52a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_109264v2: a6282eed0f613f3230a53a1826ac913edc0a65e4 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits c959da3572d3 drm/i915/gvt: Add missing vfio_unregister_group_dev() call == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109264v2/index.html [-- Attachment #2: Type: text/html, Size: 12867 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-10-19 10:14 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-29 17:48 [Intel-gfx] [PATCH] drm/i915/gvt: Add missing vfio_unregister_group_dev() call Jason Gunthorpe 2022-09-29 20:04 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork 2022-09-30 1:47 ` [Intel-gfx] [PATCH] " Tian, Kevin 2022-09-30 6:46 ` Christoph Hellwig 2022-09-30 21:31 ` Alex Williamson 2022-10-05 20:17 ` Alex Williamson 2022-10-05 22:03 ` Alex Williamson 2022-10-06 11:37 ` Jason Gunthorpe 2022-10-06 18:31 ` Alex Williamson 2022-10-10 23:14 ` Tian, Kevin 2022-10-19 9:40 ` Wang, Zhi A 2022-10-19 10:13 ` Tian, Kevin 2022-10-06 11:35 ` Jason Gunthorpe 2022-10-05 21:16 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gvt: Add missing vfio_unregister_group_dev() call (rev2) Patchwork 2022-10-05 21:38 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox