From: Baolu Lu <baolu.lu@linux.intel.com>
To: Robin Murphy <robin.murphy@arm.com>,
alex.williamson@redhat.com, cohuck@redhat.com
Cc: baolu.lu@linux.intel.com, kvm@vger.kernel.org,
iommu@lists.linux.dev, iommu@lists.linux-foundation.org,
linux-kernel@vger.kernel.org, jgg@nvidia.com
Subject: Re: [PATCH v2 1/2] vfio/type1: Simplify bus_type determination
Date: Thu, 23 Jun 2022 09:46:16 +0800 [thread overview]
Message-ID: <e18cca4c-c324-c1ab-7a2f-0f97c6387475@linux.intel.com> (raw)
In-Reply-To: <b1d13cade281a7d8acbfd0f6a33dcd086207952c.1655898523.git.robin.murphy@arm.com>
On 2022/6/22 20:04, Robin Murphy wrote:
> Since IOMMU groups are mandatory for drivers to support, it stands to
> reason that any device which has been successfully be added to a group
> must be on a bus supported by that IOMMU driver, and therefore a domain
> viable for any device in the group must be viable for all devices in
> the group. This already has to be the case for the IOMMU API's internal
> default domain, for instance. Thus even if the group contains devices on
> different buses, that can only mean that the IOMMU driver actually
> supports such an odd topology, and so without loss of generality we can
> expect the bus type of any device in a group to be suitable for IOMMU
> API calls.
Ideally we could remove bus->iommu_ops and all IOMMU APIs go through the
dev_iommu_ops().
>
> Replace vfio_bus_type() with a simple call to resolve an appropriate
> member device from which to then derive a bus type. This is also a step
> towards removing the vague bus-based interfaces from the IOMMU API, when
> we can subsequently switch to using this device directly.
>
> Furthermore, scrutiny reveals a lack of protection for the bus being
> removed while vfio_iommu_type1_attach_group() is using it; the reference
> that VFIO holds on the iommu_group ensures that data remains valid, but
> does not prevent the group's membership changing underfoot. Holding the
> vfio_device for as long as we need here also neatly solves this.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Best regards,
baolu
> ---
>
> After sleeping on it, I decided to type up the helper function approach
> to see how it looked in practice, and in doing so realised that with one
> more tweak it could also subsume the locking out of the common paths as
> well, so end up being a self-contained way for type1 to take care of its
> own concern, which I rather like.
>
> drivers/vfio/vfio.c | 18 +++++++++++++++++-
> drivers/vfio/vfio.h | 3 +++
> drivers/vfio/vfio_iommu_type1.c | 30 +++++++++++-------------------
> 3 files changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 61e71c1154be..73bab04880d0 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -448,7 +448,7 @@ static void vfio_group_get(struct vfio_group *group)
> * Device objects - create, release, get, put, search
> */
> /* Device reference always implies a group reference */
> -static void vfio_device_put(struct vfio_device *device)
> +void vfio_device_put(struct vfio_device *device)
> {
> if (refcount_dec_and_test(&device->refcount))
> complete(&device->comp);
> @@ -475,6 +475,22 @@ static struct vfio_device *vfio_group_get_device(struct vfio_group *group,
> return NULL;
> }
>
> +struct vfio_device *vfio_device_get_from_iommu(struct iommu_group *iommu_group)
> +{
> + struct vfio_group *group = vfio_group_get_from_iommu(iommu_group);
> + struct vfio_device *device;
> +
> + mutex_lock(&group->device_lock);
> + list_for_each_entry(device, &group->device_list, group_next) {
> + if (vfio_device_try_get(device)) {
> + mutex_unlock(&group->device_lock);
> + return device;
> + }
> + }
> + mutex_unlock(&group->device_lock);
> + return NULL;
> +}
> +
> /*
> * VFIO driver API
> */
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a67130221151..e8f21e64541b 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -70,3 +70,6 @@ struct vfio_iommu_driver_ops {
>
> int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
> void vfio_unregister_iommu_driver(const struct vfio_iommu_driver_ops *ops);
> +
> +struct vfio_device *vfio_device_get_from_iommu(struct iommu_group *iommu_group);
> +void vfio_device_put(struct vfio_device *device);
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c13b9290e357..e38b8bfde677 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1679,18 +1679,6 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> return ret;
> }
>
> -static int vfio_bus_type(struct device *dev, void *data)
> -{
> - struct bus_type **bus = data;
> -
> - if (*bus && *bus != dev->bus)
> - return -EINVAL;
> -
> - *bus = dev->bus;
> -
> - return 0;
> -}
> -
> static int vfio_iommu_replay(struct vfio_iommu *iommu,
> struct vfio_domain *domain)
> {
> @@ -2159,7 +2147,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> struct vfio_iommu *iommu = iommu_data;
> struct vfio_iommu_group *group;
> struct vfio_domain *domain, *d;
> - struct bus_type *bus = NULL;
> + struct vfio_device *iommu_api_dev;
> bool resv_msi, msi_remap;
> phys_addr_t resv_msi_base = 0;
> struct iommu_domain_geometry *geo;
> @@ -2192,18 +2180,19 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> goto out_unlock;
> }
>
> - /* Determine bus_type in order to allocate a domain */
> - ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
> - if (ret)
> + /* Resolve the group back to a member device for IOMMU API ops */
> + ret = -ENODEV;
> + iommu_api_dev = vfio_device_get_from_iommu(iommu_group);
> + if (!iommu_api_dev)
> goto out_free_group;
>
> ret = -ENOMEM;
> domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> if (!domain)
> - goto out_free_group;
> + goto out_put_dev;
>
> ret = -EIO;
> - domain->domain = iommu_domain_alloc(bus);
> + domain->domain = iommu_domain_alloc(iommu_api_dev->dev->bus);
> if (!domain->domain)
> goto out_free_domain;
>
> @@ -2258,7 +2247,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> list_add(&group->next, &domain->group_list);
>
> msi_remap = irq_domain_check_msi_remap() ||
> - iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
> + iommu_capable(iommu_api_dev->dev->bus, IOMMU_CAP_INTR_REMAP);
>
> if (!allow_unsafe_interrupts && !msi_remap) {
> pr_warn("%s: No interrupt remapping support. Use the module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support on this platform\n",
> @@ -2331,6 +2320,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> iommu->num_non_pinned_groups++;
> mutex_unlock(&iommu->lock);
> vfio_iommu_resv_free(&group_resv_regions);
> + vfio_device_put(iommu_api_dev);
>
> return 0;
>
> @@ -2342,6 +2332,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> vfio_iommu_resv_free(&group_resv_regions);
> out_free_domain:
> kfree(domain);
> +out_put_dev:
> + vfio_device_put(iommu_api_dev);
> out_free_group:
> kfree(group);
> out_unlock:
next prev parent reply other threads:[~2022-06-23 1:46 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 12:04 [PATCH v2 1/2] vfio/type1: Simplify bus_type determination Robin Murphy
2022-06-22 12:04 ` [PATCH v2 2/2] vfio: Use device_iommu_capable() Robin Murphy
2022-06-23 1:47 ` Baolu Lu
2022-06-22 22:17 ` [PATCH v2 1/2] vfio/type1: Simplify bus_type determination Alex Williamson
2022-06-23 8:46 ` Tian, Kevin
2022-06-23 20:35 ` Alex Williamson
2022-06-23 12:23 ` Robin Murphy
2022-06-23 20:50 ` Jason Gunthorpe
2022-06-23 23:00 ` Alex Williamson
2022-06-24 1:50 ` Jason Gunthorpe
2022-06-24 14:11 ` Alex Williamson
2022-06-24 14:18 ` Jason Gunthorpe
2022-06-24 14:28 ` Alex Williamson
2022-06-24 14:56 ` Jason Gunthorpe
2022-06-24 15:12 ` Robin Murphy
2022-06-24 16:04 ` Alex Williamson
2022-06-23 1:46 ` Baolu Lu [this message]
2022-06-23 4:32 ` kernel test robot
2022-06-24 1:52 ` Jason Gunthorpe
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=e18cca4c-c324-c1ab-7a2f-0f97c6387475@linux.intel.com \
--to=baolu.lu@linux.intel.com \
--cc=alex.williamson@redhat.com \
--cc=cohuck@redhat.com \
--cc=iommu@lists.linux-foundation.org \
--cc=iommu@lists.linux.dev \
--cc=jgg@nvidia.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robin.murphy@arm.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 NNTP newsgroup(s).