From: "Tian, Kevin" <kevin.tian@intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>,
Jason Gunthorpe <jgg@ziepe.ca>, Joerg Roedel <joro@8bytes.org>,
Will Deacon <will@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
Jean-Philippe Brucker <jean-philippe@linaro.org>,
Nicolin Chen <nicolinc@nvidia.com>,
"Liu, Yi L" <yi.l.liu@intel.com>,
"Jacob Pan" <jacob.jun.pan@linux.intel.com>,
Joel Granados <j.granados@samsung.com>
Cc: "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v5 5/9] iommufd: Add iommufd fault object
Date: Wed, 15 May 2024 08:37:09 +0000 [thread overview]
Message-ID: <BN9PR11MB52769AC595B6BDA8FB4639258CEC2@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20240430145710.68112-6-baolu.lu@linux.intel.com>
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, April 30, 2024 10:57 PM
>
> @@ -131,6 +131,9 @@ struct iopf_group {
> struct iommu_attach_handle *attach_handle;
> /* The device's fault data parameter. */
> struct iommu_fault_param *fault_param;
> + /* Used by handler provider to hook the group on its own lists. */
> + struct list_head node;
> + u32 cookie;
better put together with attach_handle.
rename 'node' to 'handle_node'
> @@ -128,6 +128,7 @@ enum iommufd_object_type {
> IOMMUFD_OBJ_HWPT_NESTED,
> IOMMUFD_OBJ_IOAS,
> IOMMUFD_OBJ_ACCESS,
> + IOMMUFD_OBJ_FAULT,
Agree with Jason that 'FAULT_QUEUE' sounds a clearer object name.
> @@ -395,6 +396,8 @@ struct iommufd_device {
> /* always the physical device */
> struct device *dev;
> bool enforce_cache_coherency;
> + /* outstanding faults awaiting response indexed by fault group id */
> + struct xarray faults;
this...
> +struct iommufd_fault {
> + struct iommufd_object obj;
> + struct iommufd_ctx *ictx;
> + struct file *filep;
> +
> + /* The lists of outstanding faults protected by below mutex. */
> + struct mutex mutex;
> + struct list_head deliver;
> + struct list_head response;
...and here worth a discussion.
First the response list is not used. If continuing the choice of queueing
faults per device it should be removed.
But I wonder whether it makes more sense to keep this response
queue per fault object. sounds simpler to me.
Also it's unclear why we need the response message to carry the
same info as the request while only id/code/cookie are used.
+struct iommu_hwpt_page_response {
+ __u32 size;
+ __u32 flags;
+ __u32 dev_id;
+ __u32 pasid;
+ __u32 grpid;
+ __u32 code;
+ __u32 cookie;
+ __u32 reserved;
+};
If we keep the response queue in the fault object, the response message
only needs to carry size/flags/code/cookie and cookie can identify the
pending message uniquely in the response queue.
> +static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user
> *buf,
> + size_t count, loff_t *ppos)
> +{
> + size_t response_size = sizeof(struct iommu_hwpt_page_response);
> + struct iommufd_fault *fault = filep->private_data;
> + struct iommu_hwpt_page_response response;
> + struct iommufd_device *idev = NULL;
> + struct iopf_group *group;
> + size_t done = 0;
> + int rc;
> +
> + if (*ppos || count % response_size)
> + return -ESPIPE;
> +
> + mutex_lock(&fault->mutex);
> + while (count > done) {
> + rc = copy_from_user(&response, buf + done, response_size);
> + if (rc)
> + break;
> +
> + if (!idev || idev->obj.id != response.dev_id)
> + idev = container_of(iommufd_get_object(fault->ictx,
> + response.dev_id,
> +
> IOMMUFD_OBJ_DEVICE),
> + struct iommufd_device, obj);
> + if (IS_ERR(idev))
> + break;
> +
> + group = xa_erase(&idev->faults, response.cookie);
> + if (!group)
> + break;
is 'continue' better?
> +
> + iopf_group_response(group, response.code);
PCIe spec states that a response failure disables the PRI interface. For SR-IOV
it'd be dangerous allowing user to trigger such code to VF to close the entire
shared PRI interface.
Just another example lacking of coordination for shared capabilities between
PF/VF. But exposing such gap to userspace makes it worse.
I guess we don't want to make this work depending on that cleanup. The
minimal correct thing is to disallow attaching VF to a fault-capable hwpt
with a note here that once we turn on support for VF the response failure
code should not be forwarded to the hardware. Instead it's an indication
that the user cannot serve more requests and such situation waits for
a vPRI reset to recover.
> + iopf_free_group(group);
> + done += response_size;
> +
> + iommufd_put_object(fault->ictx, &idev->obj);
get/put is unpaired:
if (!idev || idev->obj.id != response.dev_id)
idev = iommufd_get_object();
...
iommufd_put_object(idev);
The intention might be reusing idev if multiple fault responses are
for a same idev. But idev is always put in each iteration then following
messages will access the idev w/o holding the reference.
next prev parent reply other threads:[~2024-05-15 8:37 UTC|newest]
Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 14:57 [PATCH v5 0/9] IOMMUFD: Deliver IO page faults to user space Lu Baolu
2024-04-30 14:57 ` [PATCH v5 1/9] iommu: Introduce domain attachment handle Lu Baolu
2024-05-15 7:17 ` Tian, Kevin
2024-05-19 10:07 ` Baolu Lu
2024-04-30 14:57 ` [PATCH v5 2/9] iommu: Replace sva_iommu with iommu_attach_handle Lu Baolu
2024-05-07 23:58 ` Jason Gunthorpe
2024-05-15 7:21 ` Tian, Kevin
2024-05-19 10:14 ` Baolu Lu
2024-05-20 3:18 ` Tian, Kevin
2024-04-30 14:57 ` [PATCH v5 3/9] iommu: Add attachment handle to struct iopf_group Lu Baolu
2024-05-08 0:04 ` Jason Gunthorpe
2024-05-10 3:14 ` Baolu Lu
2024-05-10 13:38 ` Jason Gunthorpe
2024-05-10 14:30 ` Baolu Lu
2024-05-10 16:28 ` Jason Gunthorpe
2024-05-15 7:28 ` Tian, Kevin
2024-05-15 7:31 ` Tian, Kevin
2024-05-19 14:03 ` Baolu Lu
2024-05-20 3:20 ` Tian, Kevin
2024-04-30 14:57 ` [PATCH v5 4/9] iommufd: Add fault and response message definitions Lu Baolu
2024-05-15 7:43 ` Tian, Kevin
2024-05-19 14:37 ` Baolu Lu
2024-05-20 3:24 ` Tian, Kevin
2024-05-20 3:33 ` Baolu Lu
2024-05-20 4:59 ` Tian, Kevin
2024-05-24 14:15 ` Jason Gunthorpe
2024-05-27 1:27 ` Tian, Kevin
2024-04-30 14:57 ` [PATCH v5 5/9] iommufd: Add iommufd fault object Lu Baolu
2024-05-08 0:11 ` Jason Gunthorpe
2024-05-08 10:05 ` Baolu Lu
2024-05-15 7:57 ` Tian, Kevin
2024-05-20 0:41 ` Baolu Lu
2024-05-20 3:26 ` Tian, Kevin
2024-05-20 3:28 ` Baolu Lu
2024-05-08 0:22 ` Jason Gunthorpe
2024-05-10 9:13 ` Baolu Lu
2024-05-15 8:37 ` Tian, Kevin [this message]
2024-05-20 1:15 ` Baolu Lu
2024-05-20 1:24 ` Baolu Lu
2024-05-24 14:16 ` Jason Gunthorpe
2024-05-20 1:33 ` Baolu Lu
2024-05-20 3:33 ` Tian, Kevin
2024-05-20 1:38 ` Baolu Lu
2024-04-30 14:57 ` [PATCH v5 6/9] iommufd: Fault-capable hwpt attach/detach/replace Lu Baolu
2024-05-08 0:18 ` Jason Gunthorpe
2024-05-10 3:20 ` Baolu Lu
2024-05-10 13:39 ` Jason Gunthorpe
2024-05-15 8:43 ` Tian, Kevin
2024-05-20 2:10 ` Baolu Lu
2024-05-20 3:35 ` Tian, Kevin
2024-05-20 3:55 ` Baolu Lu
2024-04-30 14:57 ` [PATCH v5 7/9] iommufd: Associate fault object with iommufd_hw_pgtable Lu Baolu
2024-05-08 0:25 ` Jason Gunthorpe
2024-05-10 3:23 ` Baolu Lu
2024-05-15 8:50 ` Tian, Kevin
2024-05-20 2:18 ` Baolu Lu
2024-05-20 3:39 ` Tian, Kevin
2024-05-20 4:00 ` Baolu Lu
2024-05-24 14:24 ` Jason Gunthorpe
2024-05-27 1:33 ` Tian, Kevin
2024-05-27 3:16 ` Baolu Lu
2024-04-30 14:57 ` [PATCH v5 8/9] iommufd/selftest: Add IOPF support for mock device Lu Baolu
2024-04-30 14:57 ` [PATCH v5 9/9] iommufd/selftest: Add coverage for IOPF test Lu Baolu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN9PR11MB52769AC595B6BDA8FB4639258CEC2@BN9PR11MB5276.namprd11.prod.outlook.com \
--to=kevin.tian@intel.com \
--cc=baolu.lu@linux.intel.com \
--cc=iommu@lists.linux.dev \
--cc=j.granados@samsung.com \
--cc=jacob.jun.pan@linux.intel.com \
--cc=jean-philippe@linaro.org \
--cc=jgg@ziepe.ca \
--cc=joro@8bytes.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolinc@nvidia.com \
--cc=robin.murphy@arm.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=will@kernel.org \
--cc=yi.l.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).