Re: [Intel-gfx] [PATCH v12 14/24] iommufd/device: Add iommufd_access_detach() API
On Fri, Jun 23, 2023 at 11:15:40AM -0300, Jason Gunthorpe wrote:
> > +static void __iommufd_access_detach(struct iommufd_access *access)
> > +{
> > + struct iommufd_ioas *cur_ioas = access->ioas;
> > +
> > + lockdep_assert_held(>ioas_lock);
> > + /*
> > +* Set ioas to NULL to block any further iommufd_access_pin_pages().
> > +* iommufd_access_unpin_pages() can continue using access->ioas_unpin.
> > +*/
> > + access->ioas = NULL;
> > +
> > + if (access->ops->unmap) {
> > + mutex_unlock(>ioas_lock);
> > + access->ops->unmap(access->data, 0, ULONG_MAX);
> > + mutex_lock(>ioas_lock);
> > + }
> > + iopt_remove_access(_ioas->iopt, access);
> > + refcount_dec(_ioas->obj.users);
> > +}
> > +
> > +void iommufd_access_detach(struct iommufd_access *access)
> > +{
> > + mutex_lock(>ioas_lock);
> > + if (WARN_ON(!access->ioas))
> > + goto out;
> > + __iommufd_access_detach(access);
> > +out:
> > + access->ioas_unpin = NULL;
> > + mutex_unlock(>ioas_lock);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);
>
> There is not really any benefit to make this two functions
The __iommufd_access_detach() will be used by replace() in the
following series. Yet, let's merge them here then. And I'll add
__iommufd_access_detach() back in the replace series.
> > int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> > {
> [..]
> > if (access->ioas) {
>
> if (access->ioas || access->ioas_unpin) {
Ack.
> But I wonder if it should be a WARN_ON? Does VFIO protect against
> the userspace racing detach and attach, or do we expect to do it here?
VFIO has a vdev->iommufd_attached flag to prevent a double call
of this function. And detach and attach there also have a mutex
protection. So it should be a WARN_ON here, I think.
> > @@ -579,8 +620,8 @@ void iommufd_access_notify_unmap(struct io_pagetable
> > *iopt, unsigned long iova,
> > void iommufd_access_unpin_pages(struct iommufd_access *access,
> > unsigned long iova, unsigned long length)
> > {
> > - struct io_pagetable *iopt = >ioas->iopt;
> > struct iopt_area_contig_iter iter;
> > + struct io_pagetable *iopt;
> > unsigned long last_iova;
> > struct iopt_area *area;
> >
> > @@ -588,6 +629,13 @@ void iommufd_access_unpin_pages(struct iommufd_access
> > *access,
> > WARN_ON(check_add_overflow(iova, length - 1, _iova)))
> > return;
> >
> > + mutex_lock(>ioas_lock);
> > + if (!access->ioas_unpin) {
>
> This should be WARN_ON(), the driver has done something wrong if we
> call this after the access has been detached.
Ack. Also adding a line of comments for that:
+ /*
+* The driver must be doing something wrong if it calls this before an
+* iommufd_access_attach() or after an iommufd_access_detach().
+*/
+ if (WARN_ON(!access->ioas_unpin)) {
Thanks
Nic
Re: [Intel-gfx] [PATCH v12 00/24] Add vfio_device cdev for iommufd support
On Fri, Jun 02, 2023 at 05:16:29AM -0700, Yi Liu wrote: > Existing VFIO provides group-centric user APIs for userspace. Userspace > opens the /dev/vfio/$group_id first before getting device fd and hence > getting access to device. This is not the desired model for iommufd. Per > the conclusion of community discussion[1], iommufd provides device-centric > kAPIs and requires its consumer (like VFIO) to be device-centric user > APIs. Such user APIs are used to associate device with iommufd and also > the I/O address spaces managed by the iommufd. > > This series first introduces a per device file structure to be prepared > for further enhancement and refactors the kvm-vfio code to be prepared > for accepting device file from userspace. After this, adds a mechanism for > blocking device access before iommufd bind. Then refactors the vfio to be > able to handle cdev path (e.g. iommufd binding, no-iommufd, [de]attach ioas). > This refactor includes making the device_open exclusive between the group > and the cdev path, only allow single device open in cdev path; vfio-iommufd > code is also refactored to support cdev. e.g. split the vfio_iommufd_bind() > into two steps. Eventually, adds the cdev support for vfio device and the > new ioctls, then makes group infrastructure optional as it is not needed > when vfio device cdev is compiled. > > This series is based on some preparation works done to vfio emulated > devices[2] > and vfio pci hot reset enhancements[3]. > > This series is a prerequisite for iommu nesting for vfio device[4] [5]. > > The complete code can be found in below branch, simple tests done to the > legacy group path and the cdev path. Draft QEMU branch can be found at[6] > However, the noiommu mode test is only done with some hacks in kernel and > qemu to check if qemu can boot with noiommu devices. > > https://github.com/yiliu1765/iommufd/tree/vfio_device_cdev_v12 > (config CONFIG_IOMMUFD=y CONFIG_VFIO_DEVICE_CDEV=y) Rebased our nesting branch, and tested with an updated QEMU branch on ARM64 (SMMUv3): https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-06052023-cdev-v12-nic https://github.com/nicolinc/qemu/commits/wip/iommufd_nesting-06062023 Tested-by: Nicolin Chen Thanks Nicolin
Re: [Intel-gfx] [PATCH v8 16/24] iommufd/device: Add iommufd_access_detach() API
On Mon, Mar 27, 2023 at 07:23:57PM -0700, Jon Pan-Doh wrote:
> External email: Use caution opening links or attachments
>
>
> On 2023/3/27 02:40, Yi Liu wrote:
> > diff --git a/drivers/iommu/iommufd/iommufd_private.h
> > b/drivers/iommu/iommufd/iommufd_private.h
> > index 2e6e8e217cce..ec2ce3ef187d 100644
> > --- a/drivers/iommu/iommufd/iommufd_private.h
> > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > @@ -263,6 +263,8 @@ struct iommufd_access {
> > struct iommufd_object obj;
> > struct iommufd_ctx *ictx;
> > struct iommufd_ioas *ioas;
> > + struct iommufd_ioas *ioas_unpin;
> > + struct mutex ioas_lock;
> > const struct iommufd_access_ops *ops;
> > void *data;
> > unsigned long iova_alignment;
>
> I think you may need to initialize ioas_lock. I got lockdep warnings running
> iommufd selftests against this patch. Those went away when I added
> mutex_init().
>
> ---
> drivers/iommu/iommufd/device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index f0522d80919d..0eaae60f3537 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -474,6 +474,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> iommufd_ctx_get(ictx);
> iommufd_object_finalize(ictx, >obj);
> *id = access->obj.id;
> + mutex_init(>ioas_lock);
> return access;
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
> --
> 2.40.0.348.gf938b09366-goog
Yes... I think I lost that when splitting the changes.
Yi, can you help add this in the next version?
Thanks!
Nicolin
Re: [Intel-gfx] [PATCH v8 00/24] Add vfio_device cdev for iommufd support
On Mon, Mar 27, 2023 at 02:40:23AM -0700, Yi Liu wrote: > External email: Use caution opening links or attachments > > > Existing VFIO provides group-centric user APIs for userspace. Userspace > opens the /dev/vfio/$group_id first before getting device fd and hence > getting access to device. This is not the desired model for iommufd. Per > the conclusion of community discussion[1], iommufd provides device-centric > kAPIs and requires its consumer (like VFIO) to be device-centric user > APIs. Such user APIs are used to associate device with iommufd and also > the I/O address spaces managed by the iommufd. > > This series first introduces a per device file structure to be prepared > for further enhancement and refactors the kvm-vfio code to be prepared > for accepting device file from userspace. Afte this, adds a mechanism for > blocking device access before iommufd bind. Then refactors the vfio to be > able to handle cdev path (e.g. iommufd binding, no-iommufd, [de]attach ioas). > This refactor includes making the device_open exclusive between group and > cdev path, only allow single device open in cdev path; vfio-iommufd code is > also refactored to support cdev. e.g. split the vfio_iommufd_bind() into > two steps. Eventually, adds the cdev support for vfio device and the new > ioctls, then makes group infrastructure optional as it is not needed when > vfio device cdev is compiled. > > This series is based on some preparation works done to vfio emulated > devices[2] > and vfio pci hot reset enhancements[3]. > > This series is a prerequisite for iommu nesting for vfio device[4] [5]. > > The complete code can be found in below branch, simple tests done to the > legacy group path and the cdev path. Draft QEMU branch can be found at[6] > > https://github.com/yiliu1765/iommufd/tree/vfio_device_cdev_v8 > (config CONFIG_IOMMUFD=y CONFIG_VFIO_DEVICE_CDEV=y) > > base-commit: 1d412cdf6cd17c347b5398416a60518671e13d37 > > [1] > https://lore.kernel.org/kvm/bn9pr11mb5433b1e4ae5b0480369f97178c...@bn9pr11mb5433.namprd11.prod.outlook.com/ > [2] https://lore.kernel.org/kvm/20230327093351.44505-1-yi.l@intel.com/ > [3] https://lore.kernel.org/kvm/20230327093458.44939-1-yi.l@intel.com/ > [4] > https://lore.kernel.org/linux-iommu/20230309080910.607396-1-yi.l@intel.com/ > [5] > https://lore.kernel.org/linux-iommu/20230309082207.612346-1-yi.l@intel.com/ > [6] https://github.com/yiliu1765/qemu/tree/iommufd_rfcv3 (it is based on > Eric's > QEMU iommufd rfcv3 > (https://lore.kernel.org/kvm/20230131205305.2726330-1-eric.au...@redhat.com/) > plus commits to align with vfio_device_cdev v8) > > Change log: > > v8: > - Add patch 18 to determine noiommu device at vfio_device registration > (Jason) > - Add patch 19 to name noiommu device with "noiommu-" prefix to be par with >group path > - Add r-b from Kevin > - Add t-b from Terrence This runs well with iommufd selftest on x86 and QEMU sanity on ARM64, applying nesting series on top of this series: https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03272023 Tested-by: Nicolin Chen
Re: [Intel-gfx] [PATCH v3 0/6] vfio: Make emulated devices prepared for vfio device cdev
On Mon, Mar 27, 2023 at 02:33:45AM -0700, Yi Liu wrote: > External email: Use caution opening links or attachments > > > The .bind_iommufd op of vfio emulated devices are either empty or does > nothing. This is different with the vfio physical devices, to add vfio > device cdev, need to make them act the same. > > This series first makes the .bind_iommufd op of vfio emulated devices > to create iommufd_access, this introduces a new iommufd API. Then let > the driver that does not provide .bind_iommufd op to use the vfio emulated > iommufd op set. This makes all vfio device drivers have consistent iommufd > operations, which is good for adding new device uAPIs in the device cdev > series. > > Change log: > > v3: > - Use iommufd_get_ioas() for ioas get, hence patch 01 is added to modify >the input parameter of iommufd_get_ioas(). (Jason) > - Add r-b from Jason and Kevin > - Add t-b from Terrence Xu This runs well with iommufd selftest on x86 and QEMU sanity on ARM64, applying nesting series on top of this and cdev series: https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03272023 Tested-by: Nicolin Chen
Re: [Intel-gfx] [PATCH v6 12/24] vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET
On Thu, Mar 16, 2023 at 01:22:58PM +, Liu, Yi L wrote: > > And regarding the new baseline for the replace series and the > > nesting series, it'd be nicer to have another one git-merging > > your cdev v7 branch on top of Jason's iommufd_hwpt branch. We > > could wait for him updating to 6.3-rc2, if that's necessary. > > Yes. I cherry-pick his iommufd_hwpt to 6.3-rc2 and then try a > merge and then cherry-pick the replace and nesting series from > your above branch. Though the order between cdev and > iommufd_hwpt not perfect, we may use it as a wip baseline > when we try to address the comments w.r.t. nesting and > replace series. > > https://github.com/yiliu1765/iommufd/tree/wip/iommufd_nesting-03162023 Nice. It looks like you integrated everything from my tree so it saves me some effort :) Regarding the order between cdev and iommufd_hwpt, I think it would be Jason's decision whether to merge his changes prior to the PR from the VFIO tree, or the other way around. One way or another, I think the replace v5 and the nesting v2 will be less impacted, unless Jason makes some huge changes to his branch. Let's use this tree this week to rework both series, and rebase after he comes back and updates his tree. Lemme know if you need a help for the nesting series or so. Thanks Nic
Re: [Intel-gfx] [PATCH v6 12/24] vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET
On Thu, Mar 16, 2023 at 06:28:28AM +, Liu, Yi L wrote: > > Anyway let's not wait here. Send your v7 and we can have more > > focused discussion in your split series about hot reset. > > Sure. Once Nicolin's patch is updated, I can send v7 with the hot > reset series as well. I've updated three commits and pushed here: https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03152023 Please pull the following commit to the emulated series: "iommufd: Create access in vfio_iommufd_emulated_bind()" https://github.com/nicolinc/iommufd/commit/6467e332584de62d1c4d5daab404a8c8d5a90a2d Please pull the following commit to the cdev series or a place that you feel it'd be better -- it's required by the change of adding vfio_iommufd_emulated_detach_ioas(): "iommufd/device: Add iommufd_access_detach() API" https://github.com/nicolinc/iommufd/commit/86346b5d06100640037cbb4a14bd249476072dec The other one adding replace() will go with the replace series. And regarding the new baseline for the replace series and the nesting series, it'd be nicer to have another one git-merging your cdev v7 branch on top of Jason's iommufd_hwpt branch. We could wait for him updating to 6.3-rc2, if that's necessary. Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Thu, Mar 16, 2023 at 05:49:20AM +, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen > > Sent: Thursday, March 16, 2023 1:44 PM > > > > On Thu, Mar 16, 2023 at 05:38:41AM +, Tian, Kevin wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > > From: Nicolin Chen > > > > Sent: Thursday, March 16, 2023 1:33 PM > > > > > > > > Hi Kevin, > > > > > > > > I've fixed the other two commits. Here is the one that I am > > > > not sure about: > > > > > > > > On Thu, Mar 16, 2023 at 02:53:50AM +, Tian, Kevin wrote: > > > > > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > > > 097e2c9d9e26af4 > > > > > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in > > attach. > > > > > > > > This is the commit adding detach(). And there's a check in it: > > > > if (WARN_ON(!access->ioas)) > > > > > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > > > in the commit adding attach()? > > > > > > if (access->ioas && access->ioas != new_ioas) > > > return -EBUSY; > > > > > > yes this is for attach. > > > > OK. For attach(), the access->ioas shouldn't be !NULL, I think. > > At the point of adding attach(), the uAPI doesn't support the > > replacement use case yet. And later we have a separate API for > > that. > > what about user calling attach twice in cdev? > > > > > So I think it'd be just: > > if (access->ioas) > > return -EBUSY; > > > > The reason why I didn't add it is actually because the caller > > vfio_iommufd_emulated_attach_ioas() has a check of "attached" > > already. Yet, it doesn't hurt to have one more in the API. > > > > but here the slight difference is that in physical path we allow > attach twice to the same hwpt. they should be consistent: > > if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) > return -EINVAL; I see. The point is to support duplicated calls: ATTACH (pt_id = ioas1) ATTACH (pt_id = ioas1) Then I will add this to keep the consistency: if (access->ioas != NULL && access->ioas != new_ioas) return -EINVAL; Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Thu, Mar 16, 2023 at 05:38:41AM +, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen > > Sent: Thursday, March 16, 2023 1:33 PM > > > > Hi Kevin, > > > > I've fixed the other two commits. Here is the one that I am > > not sure about: > > > > On Thu, Mar 16, 2023 at 02:53:50AM +, Tian, Kevin wrote: > > > > > > [2] This adds iommufd_access_detach() in the cdev series: > > > > "iommufd/device: Add iommufd_access_detach() API" > > > > > > > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > > > 097e2c9d9e26af4 > > > > > > also add a check if old_ioas exists it must equal to the new_ioas in > > > attach. > > > > This is the commit adding detach(). And there's a check in it: > > if (WARN_ON(!access->ioas)) > > > > Do you mean having an "if (access->ioas) return -EBUSY;" line > > in the commit adding attach()? > > if (access->ioas && access->ioas != new_ioas) > return -EBUSY; > > yes this is for attach. OK. For attach(), the access->ioas shouldn't be !NULL, I think. At the point of adding attach(), the uAPI doesn't support the replacement use case yet. And later we have a separate API for that. So I think it'd be just: if (access->ioas) return -EBUSY; The reason why I didn't add it is actually because the caller vfio_iommufd_emulated_attach_ioas() has a check of "attached" already. Yet, it doesn't hurt to have one more in the API. Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
Hi Kevin, I've fixed the other two commits. Here is the one that I am not sure about: On Thu, Mar 16, 2023 at 02:53:50AM +, Tian, Kevin wrote: > > [2] This adds iommufd_access_detach() in the cdev series: > > "iommufd/device: Add iommufd_access_detach() API" > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > 097e2c9d9e26af4 > > also add a check if old_ioas exists it must equal to the new_ioas in attach. This is the commit adding detach(). And there's a check in it: if (WARN_ON(!access->ioas)) Do you mean having an "if (access->ioas) return -EBUSY;" line in the commit adding attach()? And, how should we check in the detach() if it equals to the new_ioas in attach? Isn't the WARN_ON(!access->ioas) enough? Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Thu, Mar 16, 2023 at 02:53:50AM +, Tian, Kevin wrote: > > Please check if they look okay, so that Yi can integrate them > > accordingly to the emulated/cdev series. > > this split looks reasonable to me. Go ahead. Thanks for the review! I will address those comments and renew those commits by the end of the day. Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Wed, Mar 15, 2023 at 12:18:01PM +, Liu, Yi L wrote: > > OK. Basically I followed what Jason suggested by having three > > APIs and combined Kevin's inputs about the difference between > > the attach/replace(). I also updated the replace changes, and > > rebased all nesting (infrastructure, VT-d and SMMU): > > https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 > > > > The major three changes for those APIs: > > [1] This adds iommufd_access_attach() in this series: > > "iommufd: Create access in vfio_iommufd_emulated_bind()" > > > > https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dc > > ca5ceaeb40e22b5 > > [2] This adds iommufd_access_detach() in the cdev series: > > "iommufd/device: Add iommufd_access_detach() API" > > > > https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a > > 097e2c9d9e26af4 > > [3] This adds iommufd_access_replace() in the replace series: > > "iommufd: Add iommufd_access_replace() API" > > > > https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9 > > bc2bf319b7654a8 > > > > Please check if they look okay, so that Yi can integrate them > > accordingly to the emulated/cdev series. > > Thanks. I'll start to integrate after ack from Kevin or Jason. btw. > Below is my latest code (rebased on top of rc-2). > > https://github.com/yiliu1765/iommufd/tree/wip/vfio_device_cdev_v7_candidate Jason is travelling per his email in the iommufd group. Perhaps Kevin can help us here. After that, we can integrate a version and (if necessary) rework a bit after Jason comes back. Overall I think they are pretty close to what Jason suggested. Thanks Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Thu, Mar 16, 2023 at 12:17:11AM +, Tian, Kevin wrote: > > > > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct > > > > iommufd_ctx > > > > > > > > refcount_inc(>obj.users); > > > > > > > > + mutex_init(>ioas_lock); > > > > > > > > access->ictx = ictx; > > > > > > > > iommufd_ctx_get(ictx); > > > > > > > > > > > > > > this refcnt get should be moved to the start given next patch > > > > > > > removes the reference in the caller side. > > > > This change is ok but seems not necessary. > > > > Yes, vfio_iommufd_emulated_bind() will not have reference on the > > ictx after the next patch. However, it gets reference only because it > > wants to store it in vfio_device. Now, it does not store it. So no get. > > I think the caller of vfio_iommufd_emulated_bind() should ensure > > the ictx is valid. Also check the physical device bind. So maybe not > > necessary to get ictx before calling iommufd_access_create(). This is > > the same with the vfio_iommufd_physical_bind() which calls > > iommufd_device_bind() without ictx get, and iommufd_device_bind() > > also gets ictx in the end. > > > > You are right. I overlooked the fact that ictx is already held by the > caller of bind. I am dropping it then :) Nic
Re: [Intel-gfx] [PATCH v5 00/19] Add vfio_device cdev for iommufd support
On Tue, Mar 14, 2023 at 11:38:11AM +, Shameerali Kolothum Thodi wrote: > Hi Nicolin, > > I rebased your latest Qemu branch[1] on top of v7.2.0 and not observed > the above issue so far. However noticed couple of other issues when > we try to hot add/remove devices. > > (qemu) device_del net1 > qemu-system-aarch64-iommufd: Failed to free id: 4 Inappropriate ioctl for > device > qemu-system-aarch64-iommufd: IOMMU_IOAS_UNMAP failed: No such file or > directory > qemu-system-aarch64-iommufd: vfio_dma_unmap(0xf587a3d0, 0x8000101000, > 0xf000) = -2 (No such file or directory) > qemu-system-aarch64-iommufd: IOMMU_IOAS_UNMAP failed: No such file or > directory > qemu-system-aarch64-iommufd: vfio_dma_unmap(0xf587a3d0, 0x80, > 0x10) = -2 (No such file or directory) > qemu-system-aarch64-iommufd: Failed to free id:1 Device or resource busy > > Ignoring the MMIO UNMAP errors, it looks like the object free is > not proper on dev removal path. I have few quick fixes here > for this, > https://github.com/hisilicon/qemu/tree/private-v7.2.0-iommufd-nesting The smmuv3 change looks good to me. I will let Yi check the iommufd change. Yi, I wonder if this is the hot reset case that you asked me for, a couple of weeks ago. > With the above, it seems the HWPT/IOAS objects are destroyed properly > on dev detach path. But when the dev is added back, gets a Qemu seg fault > and so far I have no clue why that happens. > > (qemu) device_add vfio-pci,host=:7d:02.1,iommufd=iommufd0,bus=rp1,id=net1 > ./qemu_run-iommufd-nested: line 13: 7041 Segmentation fault > (core dumped) ./qemu-system-aarch64-iommufd > -machine virt,gic-version=3,iommu=nested-smmuv3,iommufd=iommufd0 > -enable-kvm -cpu host -m 1G -smp cpus=8,maxcpus=8 -object > iommufd,id=iommufd0 -bios QEMU_EFI_Dec2018.fd -kernel > Image-iommufd -initrd rootfs-iperf.cpio -device > ioh3420,id=rp1 -device > vfio-pci,host=:7d:02.1,iommufd=iommufd0,bus=rp1,id=net1 -append > "rdinit=init console=ttyAMA0 root=/dev/vda rw > earlycon=pl011,0x900" -net none -nographic -trace events=events -D > trace_iommufd > > There are no kernel log/crash and not much useful traces while this happens. > Understand these are early days and it is not robust in anyway, but please > let me know if you suspect anything. I will continue debugging and will update > if anything. Thanks! That'd be very helpful. Nicolin
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
Hi, On Wed, Mar 15, 2023 at 06:50:53AM +, Tian, Kevin wrote: > > So, this preparatory series will add a pair of simple attach() > > and detach() APIs. Then the cdev series will add the locking > > and the ioas_unpin stuff as a rework of the detach() API. > > I think they can be something mingled... the sample code that > > I sent previously could take care of those conditions. But, I > > am also thinking a bit that maybe attach() does not need the > > locking? I can do a separate replace() function in this case. > > > > w/o locking then you need smp_store_release() and its pair. > > anyway it's not in perf critical path. Keeping lock for attach > is simpler and safe. OK. Basically I followed what Jason suggested by having three APIs and combined Kevin's inputs about the difference between the attach/replace(). I also updated the replace changes, and rebased all nesting (infrastructure, VT-d and SMMU): https://github.com/nicolinc/iommufd/commits/wip/iommufd_nesting-03142023 The major three changes for those APIs: [1] This adds iommufd_access_attach() in this series: "iommufd: Create access in vfio_iommufd_emulated_bind()" https://github.com/nicolinc/iommufd/commit/34fba7509429380f828fb23dcca5ceaeb40e22b5 [2] This adds iommufd_access_detach() in the cdev series: "iommufd/device: Add iommufd_access_detach() API" https://github.com/nicolinc/iommufd/commit/4110522146ca1fc0d5321c04a097e2c9d9e26af4 [3] This adds iommufd_access_replace() in the replace series: "iommufd: Add iommufd_access_replace() API" https://github.com/nicolinc/iommufd/commit/36507fa9f0f42cf1a5bebe7c9bc2bf319b7654a8 Please check if they look okay, so that Yi can integrate them accordingly to the emulated/cdev series. [*] This is the patch that I posted in the other mail addressing Kevin's comments on iommufd_ctx_get(): "iommufd/device: Do iommufd_ctx_get() at the top of iommufd_access_create()" https://github.com/nicolinc/iommufd/commit/077b09bb83329dc046753f4ef672f5bf6386755c (I just saw Yi's reply concerning its necessity. Feel free to drop in that case.) Thanks Nicolin P.S. Attaching the list of changes with their locations: 3791dedf98e8 cover-letter: Add IO page table replacement support c8ebf51c3c9b vfio: Support IO page table replacement c5710f23e8f6 iommufd/selftest: Add IOMMU_TEST_OP_ACCESS_REPLACE_IOAS coverage [3] 36507fa9f0f4 iommufd: Add iommufd_access_replace() API 0263855d1e8b vfio: Do not allow !ops->dma_unmap in vfio_pin/unpin_pages() e39ed55e77a0 cover-letter: Add vfio_device cdev for iommufd support 26fd7fccaef3 docs: vfio: Add vfio device cdev description f10f3e3162bb vfio: Compile group optionally 9588ae4c4049 vfio: Add VFIO_DEVICE_AT[DE]TACH_IOMMUFD_PT 3e57108eac64 vfio: Add VFIO_DEVICE_BIND_IOMMUFD b925716dd226 vfio: Add cdev for vfio_device db309463ab92 vfio-iommufd: Add detach_ioas support for emulated VFIO devices [2] 4110522146ca iommufd/device: Add iommufd_access_detach() API abca7e1e063a vfio-iommufd: Add detach_ioas support for physical VFIO devices 9d368f7247c7 vfio: Record devid in vfio_device_file 683af0a471e1 vfio-iommufd: Split the compat_ioas attach out from vfio_iommufd_bind() 32a2e7de1d53 vfio-iommufd: Split the no-iommu support out from vfio_iommufd_bind() 8a1c042379f5 vfio: Make vfio_device_first_open() to accept NULL iommufd for noiommu fc6e0ed2aa44 vfio: Make vfio_device_open() single open for device cdev path 3f6821d507a4 vfio: Add cdev_device_open_cnt to vfio_group 896cde40a016 vfio: Block device access via device fd until device is opened f422c4216a19 vfio: Pass struct vfio_device_file * to vfio_device_open/close() b187f9980fed kvm/vfio: Accept vfio device file from userspace 721e2e60ff54 kvm/vfio: Rename kvm_vfio_group to prepare for accepting vfio device fd 8993c4c75c20 vfio: Accept vfio device file in the KVM facing kAPI a92c45ae0ce6 vfio: Remove vfio_file_is_group() fb586f783934 vfio: Refine vfio file kAPIs for KVM 50694af6f3c0 vfio: Allocate per device file structure df21c0737eef cover-letter: Make vfio-pci hot reset prepared for vfio device cdev 5c25c874d7e0 vfio/pci: Accept device fd in VFIO_DEVICE_PCI_HOT_RESET ioctl 7c30ce8b54db vfio: Accpet device file from vfio PCI hot reset path e3209342db44 vfio: Refine vfio file kAPIs for vfio PCI hot reset 8354fd79944e vfio/pci: Rename the helpers and data in hot reset path to accept device fd 54387efb858c vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET cd93ffb62c51 vfio/pci: Only need to check opened devices in the dev_set for hot reset 2a6fd7231cbf vfio/pci: Update comment around group_fd get in vfio_pci_ioctl_pci_hot_reset() 480abea5961e cover-letter: vfio: Make emulated devices prepared for vfio device cdev 46b6d1ae1754 vfio: Check the presence for iommufd callbacks in __vfio_register_dev() 6064b9f81817 Samples/mdev: Uses the vfio emulated iommufd ops set in the mdev sample drivers c20852af7291 vfio-iommufd: Make vfio_iommufd_emulated_bind() return
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Wed, Mar 15, 2023 at 06:15:23AM +, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Nicolin Chen > > Sent: Wednesday, March 15, 2023 9:01 AM > > > > Hi Jason/Kevin, > > > > > > > > Perhaps we can have iommufd_access_attach/detach in this series > > > along with a vfio_iommufd_emulated_detach_ioas, and the locking > > > will come with another patch in replace series? > > > > I recall that we previously concluded that the unbind() is safe > > to go without doing access->ops->unmap, because close_device() > > would be called prior to the unbind(). > > > > But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev > > series, we'd need the access->ops->unmap call, and the locking > > and "ioas_unpin" too in the detach and attach APIs, right? > > yes. We need locking since detach can happen any time with > cdev while driver is conducting pinning. So, this preparatory series will add a pair of simple attach() and detach() APIs. Then the cdev series will add the locking and the ioas_unpin stuff as a rework of the detach() API. > > I tried a bit of some update, across this series, cdev series, > > and the replace series. Though we might be able to simplify a > > bit of this patch/series, yet it doesn't seem to simplify the > > changes overall, particularly in the following cdev series for > > having an unmap() call and the locking. > > > > Then the replace API would mostly overlap with the attach API, > > by only having an additional detach(), which means actually we > > only need an iommufd_access_attach API to cover both cases... > > there is a subtle difference. to match the physical path: > > for attach we expect the existing access->ioas is either NULL or > same as the new ioas. > > for replace access->ioas must exist. > > they need different condition checks. I think they can be something mingled... the sample code that I sent previously could take care of those conditions. But, I am also thinking a bit that maybe attach() does not need the locking? I can do a separate replace() function in this case. Thanks Nic
Re: [Intel-gfx] [PATCH v6 19/24] vfio-iommufd: Add detach_ioas support for emulated VFIO devices
On Wed, Mar 15, 2023 at 06:15:02AM +, Liu, Yi L wrote:
> > > +void vfio_iommufd_emulated_detach_ioas(struct vfio_device *vdev)
> > > +{
> > > + lockdep_assert_held(>dev_set->lock);
> > > +
> > > + if (WARN_ON(!vdev->iommufd_access))
> > > + return;
> > > +
> > [...]
> > > + iommufd_access_destroy(vdev->iommufd_access);
> > > + vdev->iommufd_access = NULL;
> >
> > After moving access allocation/destroy to bind/unbind, here it
> > should be:
> > iommufd_access_set_ioas(vdev->iommufd_access, 0);
>
> You are right.
Yet...iommufd_access_set_ioas is getting reworked with my patch:
In another thread, Jason suggested to have iommufd_acces_detach
API, and I am trying to finalize it with Jason/Kevin.
https://lore.kernel.org/kvm/bn9pr11mb5276738dc59ac1b4a66ab3c38c...@bn9pr11mb5276.namprd11.prod.outlook.com/
Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Wed, Mar 15, 2023 at 06:16:37AM +, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
>
>
> > From: Nicolin Chen
> > Sent: Wednesday, March 15, 2023 2:51 AM
> >
> > On Fri, Mar 10, 2023 at 02:08:15AM +, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Liu, Yi L
> > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > >
> > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx
> > *ictx,
> > > > u32 ioas_id,
> > > > access->data = data;
> > > > access->ops = ops;
> > > >
> > > > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > - if (IS_ERR(obj)) {
> > > > - rc = PTR_ERR(obj);
> > > > - goto out_abort;
> > > > - }
> > > > - access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > - iommufd_ref_to_users(obj);
> > > > -
> > > > if (ops->needs_pin_pages)
> > > > access->iova_alignment = PAGE_SIZE;
> > > > else
> > > > access->iova_alignment = 1;
> > > > - rc = iopt_add_access(>ioas->iopt, access);
> > > > - if (rc)
> > > > - goto out_put_ioas;
> > > >
> > > > /* The calling driver is a user until iommufd_access_destroy() */
> > > > refcount_inc(>obj.users);
> > > > + mutex_init(>ioas_lock);
> > > > access->ictx = ictx;
> > > > iommufd_ctx_get(ictx);
> > >
> > > this refcnt get should be moved to the start given next patch
> > > removes the reference in the caller side.
> >
> > I don't feel quite convincing to justify for moving it in this
> > patch. Perhaps we should do that in the following patch, where
> > it needs this? Or another individual patch moving this alone?
> >
>
> Next patch. I just tried to point out the required change caused
> by next patch.
OK. I made a small individual patch. Posting here so Yi can just
squash it into the next patch:
>From dbfe7457d35ea9a4da9c8e6daa838b101bc8f621 Mon Sep 17 00:00:00 2001
From: Nicolin Chen
Date: Tue, 14 Mar 2023 12:51:07 -0700
Subject: [PATCH] iommufd/device: Do iommufd_ctx_get() at the top of
iommufd_access_create()
The following patch will remove the iommufd_ctx_get() call prior to the
iommufd_access_create() call in vfio_iommufd_emulated_bind(), expecting
iommufd_access_create() call covers the iommufd_ctx_get(). However, the
iommufd_access_create() only does iommufd_ctx_get() at the end. Thus,
move the iommufd_ctx_get() call to the top of iommufd_access_create().
Suggested-by: Kevin Tian
Signed-off-by: Nicolin Chen
---
drivers/iommu/iommufd/device.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 0132803449be..dc1015b02a53 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -649,13 +649,16 @@ iommufd_access_create(struct iommufd_ctx *ictx,
{
struct iommufd_access *access;
+ iommufd_ctx_get(ictx);
/*
* There is no uAPI for the access object, but to keep things symmetric
* use the object infrastructure anyhow.
*/
access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
- if (IS_ERR(access))
+ if (IS_ERR(access)) {
+ iommufd_ctx_put(ictx);
return access;
+ }
access->data = data;
access->ops = ops;
@@ -668,7 +671,6 @@ iommufd_access_create(struct iommufd_ctx *ictx,
/* The calling driver is a user until iommufd_access_destroy() */
refcount_inc(>obj.users);
access->ictx = ictx;
- iommufd_ctx_get(ictx);
iommufd_object_finalize(ictx, >obj);
return access;
}
--
2.39.2
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
Hi Jason/Kevin,
On Tue, Mar 14, 2023 at 01:20:52AM -0700, Nicolin Chen wrote:
> On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote:
> > On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:
> >
> > > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> > > +{
> > > + struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> > > + struct iommufd_ctx *ictx = access->ictx;
> > > + struct iommufd_object *obj;
> > > + int rc = 0;
> > > +
> > > + if (ioas_id) {
> > > + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > + if (IS_ERR(obj))
> > > + return PTR_ERR(obj);
> > > + new_ioas = container_of(obj, struct iommufd_ioas, obj);
> > > + }
> > > +
> > > + mutex_lock(>ioas_lock);
> > > + cur_ioas = access->ioas;
> > > + if (cur_ioas == new_ioas)
> > > + goto out_unlock;
> > > +
> > > + if (new_ioas) {
> > > + rc = iopt_add_access(_ioas->iopt, access);
> > > + if (rc)
> > > + goto out_unlock;
> > > + iommufd_ref_to_users(obj);
> > > + }
> > > +
> > > + if (cur_ioas) {
> > > + iopt_remove_access(_ioas->iopt, access);
> > > + refcount_dec(_ioas->obj.users);
> > > + }
> >
> > This should match the physical side with an add/remove/replace
> > API. Especially since remove is implicit in destroy this series only
> > needs the add API
>
> I assume that the API would be iommufd_access_attach,
> iommufd_access_detach, and iommufd_access_replace(). And there
> might be an iommufd_access_change_pt(access, pt, bool replace)?
>
> > And the locking shouldn't come in another patch that brings the
> > replace/remove since with just split add we don't need it.
>
> Hmm. The iommufd_access_detach would be needed in the following
> cdev series, while the iommufd_access_replace would be need in
> my replace series. So, that would make the API be divided into
> three series.
>
> Perhaps we can have iommufd_access_attach/detach in this series
> along with a vfio_iommufd_emulated_detach_ioas, and the locking
> will come with another patch in replace series?
I recall that we previously concluded that the unbind() is safe
to go without doing access->ops->unmap, because close_device()
would be called prior to the unbind().
But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
series, we'd need the access->ops->unmap call, and the locking
and "ioas_unpin" too in the detach and attach APIs, right?
I tried a bit of some update, across this series, cdev series,
and the replace series. Though we might be able to simplify a
bit of this patch/series, yet it doesn't seem to simplify the
changes overall, particularly in the following cdev series for
having an unmap() call and the locking.
Then the replace API would mostly overlap with the attach API,
by only having an additional detach(), which means actually we
only need an iommufd_access_attach API to cover both cases...
Can you please take a look at the final access APIs that I've
attached at the end of the email for things mentioned above?
Hopefully we can confirm and put them correctly into the next
version of the three series.
Thanks
Nic
---
static void __iommufd_access_detach(struct iommufd_access *access)
{
struct iommufd_ioas *cur_ioas = access->ioas;
lockdep_assert_held(>ioas_lock);
/*
* Set ioas to NULL to block any further iommufd_access_pin_pages().
* iommufd_access_unpin_pages() can continue using access->ioas_unpin.
*/
access->ioas = NULL;
if (access->ops->unmap) {
mutex_unlock(>ioas_lock);
access->ops->unmap(access->data, 0, ULONG_MAX);
mutex_lock(>ioas_lock);
}
iopt_remove_access(_ioas->iopt, access);
refcount_dec(_ioas->obj.users);
}
static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
{
struct iommufd_ioas *new_ioas, *cur_ioas;
struct iommufd_object *obj;
int rc = 0;
obj = iommufd_get_object(access->ictx, ioas_id, IOMMUFD_OBJ_IOAS);
if (IS_ERR(obj))
return PTR_ERR(obj);
new_ioas = container_of(obj, struct iommufd_ioas, obj);
mutex_lock(>ioas_lock);
cur_ioas = access->ioas;
if (cur_ioas == new_ioas)
goto out_unlock;
rc = iopt_add_access(_ioas->iopt, access);
if (rc)
goto out_unlock;
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Fri, Mar 10, 2023 at 02:08:15AM +, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
>
>
> > From: Liu, Yi L
> > Sent: Wednesday, March 8, 2023 9:14 PM
> >
> > @@ -449,33 +450,18 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> > u32 ioas_id,
> > access->data = data;
> > access->ops = ops;
> >
> > - obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > - if (IS_ERR(obj)) {
> > - rc = PTR_ERR(obj);
> > - goto out_abort;
> > - }
> > - access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > - iommufd_ref_to_users(obj);
> > -
> > if (ops->needs_pin_pages)
> > access->iova_alignment = PAGE_SIZE;
> > else
> > access->iova_alignment = 1;
> > - rc = iopt_add_access(>ioas->iopt, access);
> > - if (rc)
> > - goto out_put_ioas;
> >
> > /* The calling driver is a user until iommufd_access_destroy() */
> > refcount_inc(>obj.users);
> > + mutex_init(>ioas_lock);
> > access->ictx = ictx;
> > iommufd_ctx_get(ictx);
>
> this refcnt get should be moved to the start given next patch
> removes the reference in the caller side.
I don't feel quite convincing to justify for moving it in this
patch. Perhaps we should do that in the following patch, where
it needs this? Or another individual patch moving this alone?
Thanks
Nic
Re: [Intel-gfx] [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()
On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote:
> On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:
>
> > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> > +{
> > + struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> > + struct iommufd_ctx *ictx = access->ictx;
> > + struct iommufd_object *obj;
> > + int rc = 0;
> > +
> > + if (ioas_id) {
> > + obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > + if (IS_ERR(obj))
> > + return PTR_ERR(obj);
> > + new_ioas = container_of(obj, struct iommufd_ioas, obj);
> > + }
> > +
> > + mutex_lock(>ioas_lock);
> > + cur_ioas = access->ioas;
> > + if (cur_ioas == new_ioas)
> > + goto out_unlock;
> > +
> > + if (new_ioas) {
> > + rc = iopt_add_access(_ioas->iopt, access);
> > + if (rc)
> > + goto out_unlock;
> > + iommufd_ref_to_users(obj);
> > + }
> > +
> > + if (cur_ioas) {
> > + iopt_remove_access(_ioas->iopt, access);
> > + refcount_dec(_ioas->obj.users);
> > + }
>
> This should match the physical side with an add/remove/replace
> API. Especially since remove is implicit in destroy this series only
> needs the add API
I assume that the API would be iommufd_access_attach,
iommufd_access_detach, and iommufd_access_replace(). And there
might be an iommufd_access_change_pt(access, pt, bool replace)?
> And the locking shouldn't come in another patch that brings the
> replace/remove since with just split add we don't need it.
Hmm. The iommufd_access_detach would be needed in the following
cdev series, while the iommufd_access_replace would be need in
my replace series. So, that would make the API be divided into
three series.
Perhaps we can have iommufd_access_attach/detach in this series
along with a vfio_iommufd_emulated_detach_ioas, and the locking
will come with another patch in replace series?
Thanks
Nic
Re: [Intel-gfx] [PATCH v6 19/24] vfio-iommufd: Add detach_ioas support for emulated VFIO devices
On Wed, Mar 08, 2023 at 05:28:58AM -0800, Yi Liu wrote:
> External email: Use caution opening links or attachments
>
>
> this prepares for adding DETACH ioctl for emulated VFIO devices.
>
> Signed-off-by: Yi Liu
> Reviewed-by: Kevin Tian
> Tested-by: Terrence Xu
> Tested-by: Nicolin Chen
> Tested-by: Matthew Rosato
> ---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 +
> drivers/s390/cio/vfio_ccw_ops.c | 1 +
> drivers/s390/crypto/vfio_ap_ops.c | 1 +
> drivers/vfio/iommufd.c| 14 +-
> include/linux/vfio.h | 3 +++
> 5 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index de675d799c7d..9cd9e9da60dd 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -1474,6 +1474,7 @@ static const struct vfio_device_ops intel_vgpu_dev_ops
> = {
> .bind_iommufd = vfio_iommufd_emulated_bind,
> .unbind_iommufd = vfio_iommufd_emulated_unbind,
> .attach_ioas= vfio_iommufd_emulated_attach_ioas,
> + .detach_ioas= vfio_iommufd_emulated_detach_ioas,
> };
>
> static int intel_vgpu_probe(struct mdev_device *mdev)
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 5b53b94f13c7..cba4971618ff 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -632,6 +632,7 @@ static const struct vfio_device_ops vfio_ccw_dev_ops = {
> .bind_iommufd = vfio_iommufd_emulated_bind,
> .unbind_iommufd = vfio_iommufd_emulated_unbind,
> .attach_ioas = vfio_iommufd_emulated_attach_ioas,
> + .detach_ioas = vfio_iommufd_emulated_detach_ioas,
> };
>
> struct mdev_driver vfio_ccw_mdev_driver = {
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 72e10abb103a..9902e62e7a17 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1844,6 +1844,7 @@ static const struct vfio_device_ops
> vfio_ap_matrix_dev_ops = {
> .bind_iommufd = vfio_iommufd_emulated_bind,
> .unbind_iommufd = vfio_iommufd_emulated_unbind,
> .attach_ioas = vfio_iommufd_emulated_attach_ioas,
> + .detach_ioas = vfio_iommufd_emulated_detach_ioas,
> };
>
> static struct mdev_driver vfio_ap_matrix_driver = {
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index c06494e322f9..8a9457d0a33c 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -218,8 +218,20 @@ int vfio_iommufd_emulated_attach_ioas(struct vfio_device
> *vdev, u32 *pt_id)
> {
> lockdep_assert_held(>dev_set->lock);
>
> - if (!vdev->iommufd_access)
> + if (WARN_ON(!vdev->iommufd_access))
> return -ENOENT;
> return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id);
> }
> EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas);
> +
> +void vfio_iommufd_emulated_detach_ioas(struct vfio_device *vdev)
> +{
> + lockdep_assert_held(>dev_set->lock);
> +
> + if (WARN_ON(!vdev->iommufd_access))
> + return;
> +
[...]
> + iommufd_access_destroy(vdev->iommufd_access);
> + vdev->iommufd_access = NULL;
After moving access allocation/destroy to bind/unbind, here it
should be:
iommufd_access_set_ioas(vdev->iommufd_access, 0);
Thanks
Nic
Re: [Intel-gfx] [PATCH v5 00/19] Add vfio_device cdev for iommufd support
On Fri, Mar 03, 2023 at 03:01:03PM +, Shameerali Kolothum Thodi wrote: > External email: Use caution opening links or attachments > > > > -Original Message- > > From: Nicolin Chen [mailto:nicol...@nvidia.com] > > Sent: 02 March 2023 23:51 > > To: Shameerali Kolothum Thodi > > Cc: Xu, Terrence ; Liu, Yi L ; > > Jason Gunthorpe ; alex.william...@redhat.com; Tian, > > Kevin ; j...@8bytes.org; robin.mur...@arm.com; > > coh...@redhat.com; eric.au...@redhat.com; k...@vger.kernel.org; > > mjros...@linux.ibm.com; chao.p.p...@linux.intel.com; > > yi.y@linux.intel.com; pet...@redhat.com; jasow...@redhat.com; > > l...@redhat.com; suravee.suthikulpa...@amd.com; > > intel-gvt-...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; > > linux-s...@vger.kernel.org; Hao, Xudong ; Zhao, > > Yan Y > > Subject: Re: [PATCH v5 00/19] Add vfio_device cdev for iommufd support > > > > On Thu, Mar 02, 2023 at 09:43:00AM +, Shameerali Kolothum Thodi > > wrote: > > > > > Hi Nicolin, > > > > > > Thanks for the latest ARM64 branch. Do you have a working Qemu branch > > corresponding to the > > > above one? > > > > > > I tried the > > https://github.com/nicolinc/qemu/tree/wip/iommufd_rfcv3%2Bnesting%2B > > smmuv3 > > > but for some reason not able to launch the Guest. > > > > > > Please let me know. > > > > I do use that branch. It might not be that robust though as it > > went through a big rebase. > > Ok. The issue seems to be quite random in nature and only happens when there > are multiple vCPUs. Also doesn't look like related to VFIO device assignment > as I can reproduce Guest hang without it by only having nested-smmuv3 and > iommufd object. > > ./qemu-system-aarch64-iommuf -machine > virt,gic-version=3,iommu=nested-smmuv3,iommufd=iommufd0 \ > -enable-kvm -cpu host -m 1G -smp cpus=8,maxcpus=8 \ > -object iommufd,id=iommufd0 \ > -bios QEMU_EFI.fd \ > -kernel Image-6.2-iommufd \ > -initrd rootfs-iperf.cpio \ > -net none \ > -nographic \ > -append "rdinit=init console=ttyAMA0 root=/dev/vda rw > earlycon=pl011,0x900" \ > -trace events=events \ > -D trace_iommufd > > When the issue happens, no output on terminal as if Qemu is in a locked state. > > Can you try with the followings? > > > > --trace "iommufd*" --trace "smmu*" --trace "vfio_*" --trace "pci_*" --trace > > "msi_*" --trace "nvme_*" > > The only trace events with above are this, > > iommufd_backend_connect fd=22 owned=1 users=1 (0) > smmu_add_mr smmuv3-iommu-memory-region-0-0 > > I haven't debugged this further. Please let me know if issue is reproducible > with multiple vCPUs at your end. For now will focus on VFIO dev specific > tests. Oh. My test environment has been a single-core vCPU. So that doesn't happen to me. Can you try a vanilla QEMU branch that our nesting branch is rebased on? I took a branch from Yi as the baseline, while he might take from Eric for the rfcv3. I am guessing that it might be an issue in the common tree. Thanks Nicolin
Re: [Intel-gfx] [PATCH v5 00/19] Add vfio_device cdev for iommufd support
On Thu, Mar 02, 2023 at 09:43:00AM +, Shameerali Kolothum Thodi wrote: > Hi Nicolin, > > Thanks for the latest ARM64 branch. Do you have a working Qemu branch > corresponding to the > above one? > > I tried the > https://github.com/nicolinc/qemu/tree/wip/iommufd_rfcv3%2Bnesting%2Bsmmuv3 > but for some reason not able to launch the Guest. > > Please let me know. I do use that branch. It might not be that robust though as it went through a big rebase. Can you try with the followings? --trace "iommufd*" --trace "smmu*" --trace "vfio_*" --trace "pci_*" --trace "msi_*" --trace "nvme_*" Thanks Nicolin
Re: [Intel-gfx] [PATCH v5 00/19] Add vfio_device cdev for iommufd support
On Tue, Feb 28, 2023 at 04:58:06PM +, Xu, Terrence wrote: > Verified this series by "Intel GVT-g GPU device mediated passthrough" and > "Intel GVT-d GPU device direct passthrough" technologies. > Both passed VFIO legacy mode / compat mode / cdev mode, including negative > tests. > > Tested-by: Terrence Xu Sanity-tested this series on ARM64 with my wip branch: https://github.com/nicolinc/iommufd/commits/wip/iommufd-v6.2-nesting (Covering new iommufd and vfio-compat) Tested-by: Nicolin Chen
Re: [Intel-gfx] [PATCH v2 00/11] Connect VFIO to IOMMUFD
On Mon, Nov 07, 2022 at 08:52:44PM -0400, Jason Gunthorpe wrote: > This is on github: https://github.com/jgunthorpe/linux/commits/vfio_iommufd [...] > v2: > - Rebase to v6.1-rc3, v4 iommufd series > - Fixup comments and commit messages from list remarks > - Fix leaking of the iommufd for mdevs > - New patch to fix vfio modaliases when vfio container is disabled > - Add a dmesg once when the iommufd provided /dev/vfio/vfio is opened >to signal that iommufd is providing this I've redone my previous sanity tests. Except those reported bugs, things look fine. Once we fix those issues, GVT and other modules can run some more stressful tests, I think.
Re: [Intel-gfx] [PATCH v2 07/11] vfio-iommufd: Support iommufd for physical VFIO devices
On Mon, Nov 07, 2022 at 08:52:51PM -0400, Jason Gunthorpe wrote:
> @@ -795,6 +800,10 @@ static int vfio_device_first_open(struct vfio_device
> *device)
> ret = vfio_group_use_container(device->group);
> if (ret)
> goto err_module_put;
> + } else if (device->group->iommufd) {
> + ret = vfio_iommufd_bind(device, device->group->iommufd);
Here we check device->group->iommufd...
> + if (ret)
> + goto err_module_put;
> }
>
> device->kvm = device->group->kvm;
> @@ -812,6 +821,7 @@ static int vfio_device_first_open(struct vfio_device
> *device)
> device->kvm = NULL;
> if (device->group->container)
> vfio_group_unuse_container(device->group);
> + vfio_iommufd_unbind(device);
...yet, missing here, which could result in kernel oops.
Should probably add something similar:
+ if (device->group->iommufd)
+ vfio_iommufd_unbind(device);
Or should check !vdev->iommufd_device inside the ->unbind.
> err_module_put:
> mutex_unlock(>group->group_lock);
> module_put(device->dev->driver->owner);
> @@ -830,6 +840,7 @@ static void vfio_device_last_close(struct vfio_device
> *device)
> device->kvm = NULL;
> if (device->group->container)
> vfio_group_unuse_container(device->group);
> + vfio_iommufd_unbind(device);
Ditto
Re: [Intel-gfx] [PATCH 06/10] vfio-iommufd: Allow iommufd to be used in place of a container fd
On Tue, Nov 01, 2022 at 08:09:52AM +, Tian, Kevin wrote: > > From: Jason Gunthorpe > > Sent: Wednesday, October 26, 2022 2:51 AM > > > > menuconfig VFIO > > tristate "VFIO Non-Privileged userspace driver framework" > > select IOMMU_API > > + depends on IOMMUFD || !IOMMUFD > > Out of curiosity. What is the meaning of this dependency claim? "is it a module or not" -- from https://lwn.net/Articles/683476/
Re: [Intel-gfx] [PATCH 00/10] Connect VFIO to IOMMUFD
On Tue, Nov 01, 2022 at 11:04:38AM +0800, Yi Liu wrote:
> On 2022/11/1 07:24, Jason Gunthorpe wrote:
> > On Mon, Oct 31, 2022 at 08:25:39PM +0800, Yi Liu wrote:
> > > > There is something wrong with the test suite that it isn't covering
> > > > the above, I'm going to look into that today.
> > >
> > > sounds to be the cause. I didn't see any significant change in vfio_main.c
> > > that may fail gvt. So should the iommufd changes. Then we will re-run the
> > > test after your update.:-)
> >
> > I updated the github with all the changes made so far, it is worth
> > trying again!
>
> gvt is still failing with below call trace in host side. vfio_unpin_pages()
> is still in problem. Any idea on it?
> [ 206.464318] WARNING: CPU: 9 PID: 3362 at
> drivers/iommu/iommufd/device.c:591 iommufd_access_pin_pages+0x337/0x360
Judging from this WARNING, and since gvt (mdev) needs pin_pages(),
I assume this might be a fix, though Jason's latest change for the
iova_alignment seems to be added for CONFIG_IOMMUFD_TEST only.
--
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index 72a289c5f8c9..185075528d5e 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -120,6 +120,7 @@ static void vfio_emulated_unmap(void *data, unsigned long
iova,
}
static const struct iommufd_access_ops vfio_user_ops = {
+ .needs_pin_pages = 1,
.unmap = vfio_emulated_unmap,
};
--
Perhaps you can try it first to see if we can test the rest part of
the routine for now, till Jason acks tomorrow.
Thanks
Nic
Re: [Intel-gfx] [PATCH 00/10] Connect VFIO to IOMMUFD
On Tue, Oct 25, 2022 at 03:17:06PM -0300, Jason Gunthorpe wrote: > This series provides an alternative container layer for VFIO implemented > using iommufd. This is optional, if CONFIG_IOMMUFD is not set then it will > not be compiled in. > > At this point iommufd can be injected by passing in a iommfd FD to > VFIO_GROUP_SET_CONTAINER which will use the VFIO compat layer in iommufd > to obtain the compat IOAS and then connect up all the VFIO drivers as > appropriate. > > This is temporary stopping point, a following series will provide a way to > directly open a VFIO device FD and directly connect it to IOMMUFD using > native ioctls that can expose the IOMMUFD features like hwpt, future > vPASID and dynamic attachment. > > This series, in compat mode, has passed all the qemu tests we have > available, including the test suites for the Intel GVT mdev. Aside from > the temporary limitation with P2P memory this is belived to be fully > compatible with VFIO. > > This is on github: https://github.com/jgunthorpe/linux/commits/vfio_iommufd Tested-by: Nicolin Chen Tested this branch on ARM64+SMMUv3 with the iommufd selftest and QEMU passthrough sanity using noiommu and virtio-iommu setups by combining with both CONFIG_VFIO_CONTAINER=y and =n.
Re: [Intel-gfx] [PATCH 00/10] Connect VFIO to IOMMUFD
On Fri, Oct 28, 2022 at 04:53:21PM -0700, Nicolin Chen wrote: > On Tue, Oct 25, 2022 at 03:17:06PM -0300, Jason Gunthorpe wrote: > > This series provides an alternative container layer for VFIO implemented > > using iommufd. This is optional, if CONFIG_IOMMUFD is not set then it will > > not be compiled in. > > > > At this point iommufd can be injected by passing in a iommfd FD to > > VFIO_GROUP_SET_CONTAINER which will use the VFIO compat layer in iommufd > > to obtain the compat IOAS and then connect up all the VFIO drivers as > > appropriate. > > > > This is temporary stopping point, a following series will provide a way to > > directly open a VFIO device FD and directly connect it to IOMMUFD using > > native ioctls that can expose the IOMMUFD features like hwpt, future > > vPASID and dynamic attachment. > > > > This series, in compat mode, has passed all the qemu tests we have > > available, including the test suites for the Intel GVT mdev. Aside from > > the temporary limitation with P2P memory this is belived to be fully > > compatible with VFIO. > > > > This is on github: https://github.com/jgunthorpe/linux/commits/vfio_iommufd > > Tested-by: Nicolin Chen Sorry, wrong email -- should be: Tested-by: Nicolin Chen > Tested this branch on ARM64+SMMUv3 with the iommufd selftest and > QEMU passthrough sanity using noiommu and virtio-iommu setups by > combining with both CONFIG_VFIO_CONTAINER=y and =n.
