From: Alex Williamson <alex.williamson@redhat.com>
To: Yi Liu <yi.l.liu@intel.com>
Cc: jgg@nvidia.com, kevin.tian@intel.com, joro@8bytes.org,
robin.murphy@arm.com, cohuck@redhat.com, eric.auger@redhat.com,
nicolinc@nvidia.com, kvm@vger.kernel.org, mjrosato@linux.ibm.com,
chao.p.peng@linux.intel.com, yi.y.sun@linux.intel.com,
peterx@redhat.com, jasowang@redhat.com,
shameerali.kolothum.thodi@huawei.com, lulu@redhat.com,
suravee.suthikulpanit@amd.com,
intel-gvt-dev@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, linux-s390@vger.kernel.org,
xudong.hao@intel.com, yan.y.zhao@intel.com,
terrence.xu@intel.com, yanting.jiang@intel.com,
zhenzhong.duan@intel.com, clegoate@redhat.com
Subject: Re: [PATCH v11 21/23] vfio: Determine noiommu device in __vfio_register_dev()
Date: Mon, 22 May 2023 17:04:29 -0600 [thread overview]
Message-ID: <20230522170429.2d5ca274.alex.williamson@redhat.com> (raw)
In-Reply-To: <20230513132827.39066-22-yi.l.liu@intel.com>
On Sat, 13 May 2023 06:28:25 -0700
Yi Liu <yi.l.liu@intel.com> wrote:
> This is to make the cdev path and group path consistent for the noiommu
> devices registration. If vfio_noiommu is disabled, such registration
> should fail. However, this check is vfio_device_set_group() which is part
> of the vfio_group code. If the vfio_group code is compiled out, noiommu
> devices would be registered even vfio_noiommu is disabled.
>
> This adds vfio_device_set_noiommu() which can fail and calls it in the
> device registration. For now, it never fails as long as
> vfio_device_set_group() is successful. But when the vfio_group code is
> compiled out, vfio_device_set_noiommu() would fail the noiommu devices
> when vfio_noiommu is disabled.
I'm lost. After the next patch we end up with the following when
CONFIG_VFIO_GROUP is set:
static inline int vfio_device_set_noiommu(struct vfio_device *device)
{
device->noiommu = IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
device->group->type == VFIO_NO_IOMMU;
return 0;
}
I think this is relying on the fact that vfio_device_set_group() which
is called immediately prior to this function would have performed the
testing for noiommu and failed prior to this function being called and
therefore there is no error return here.
Note also here that I think CONFIG_VFIO_NOIOMMU was only being tested
here previously so that a smart enough compiler would optimize out the
entire function, we can never set a VFIO_NO_IOMMU type when
!CONFIG_VFIO_NOIOMMU. That's no longer the case if the function is
refactored like this.
When !CONFIG_VFIO_GROUP:
static inline int vfio_device_set_noiommu(struct vfio_device *device)
{
struct iommu_group *iommu_group;
iommu_group = iommu_group_get(device->dev);
if (!iommu_group) {
if (!IS_ENABLED(CONFIG_VFIO_NOIOMMU) || !vfio_noiommu)
return -EINVAL;
device->noiommu = true;
} else {
iommu_group_put(iommu_group);
device->noiommu = false;
}
return 0;
}
Here again, the NOIOMMU config option is irrelevant, vfio_noiommu can
only be true if the config option is enabled. Therefore if there's no
IOMMU group and the module option to enable noiommu is not set, return
an error.
It's really quite ugly that in one mode we rely on this function to
generate the error and in the other mode it happens prior to getting
here.
The above could be simplified to something like:
iommu_group = iommu_group_get(device->dev);
if (!iommu_group && !vfio_iommu)
return -EINVAL;
device->noiommu = !iommu_group;
iommu_group_put(iommu_group); /* Accepts NULL */
return 0;
Which would actually work regardless of CONFIG_VFIO_GROUP, where maybe
this could then be moved before vfio_device_set_group() and become the
de facto exit point for invalid noiommu configurations and maybe we
could remove the test from the group code (with a comment to note that
it's been tested prior)? Thanks,
Alex
> As noiommu devices is checked and there are multiple places which needs
> to test noiommu devices, this also adds a flag to mark noiommu devices.
> Hence the callers of vfio_device_is_noiommu() can be converted to test
> vfio_device->noiommu.
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Yanting Jiang <yanting.jiang@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
> drivers/vfio/device_cdev.c | 4 ++--
> drivers/vfio/group.c | 2 +-
> drivers/vfio/iommufd.c | 10 +++++-----
> drivers/vfio/vfio.h | 7 ++++---
> drivers/vfio/vfio_main.c | 6 +++++-
> include/linux/vfio.h | 1 +
> 6 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index 3f14edb80a93..6d7f50ee535d 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -111,7 +111,7 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> if (df->group)
> return -EINVAL;
>
> - if (vfio_device_is_noiommu(device) && !capable(CAP_SYS_RAWIO))
> + if (device->noiommu && !capable(CAP_SYS_RAWIO))
> return -EPERM;
>
> ret = vfio_device_block_group(device);
> @@ -157,7 +157,7 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
> device->cdev_opened = true;
> mutex_unlock(&device->dev_set->lock);
>
> - if (vfio_device_is_noiommu(device))
> + if (device->noiommu)
> dev_warn(device->dev, "noiommu device is bound to iommufd by user "
> "(%s:%d)\n", current->comm, task_pid_nr(current));
> return 0;
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index 7aacbd9d08c9..bf4335bce892 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -192,7 +192,7 @@ static int vfio_device_group_open(struct vfio_device_file *df)
> vfio_device_group_get_kvm_safe(device);
>
> df->iommufd = device->group->iommufd;
> - if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count == 0) {
> + if (df->iommufd && device->noiommu && device->open_count == 0) {
> ret = vfio_iommufd_compat_probe_noiommu(device,
> df->iommufd);
> if (ret)
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index 799ea322a7d4..dfe706f1e952 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -71,7 +71,7 @@ int vfio_iommufd_bind(struct vfio_device_file *df)
>
> lockdep_assert_held(&vdev->dev_set->lock);
>
> - if (vfio_device_is_noiommu(vdev))
> + if (vdev->noiommu)
> return vfio_iommufd_noiommu_bind(vdev, ictx, &df->devid);
>
> return vdev->ops->bind_iommufd(vdev, ictx, &df->devid);
> @@ -86,7 +86,7 @@ int vfio_iommufd_compat_attach_ioas(struct vfio_device *vdev,
> lockdep_assert_held(&vdev->dev_set->lock);
>
> /* compat noiommu does not need to do ioas attach */
> - if (vfio_device_is_noiommu(vdev))
> + if (vdev->noiommu)
> return 0;
>
> ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> @@ -103,7 +103,7 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
>
> lockdep_assert_held(&vdev->dev_set->lock);
>
> - if (vfio_device_is_noiommu(vdev)) {
> + if (vdev->noiommu) {
> vfio_iommufd_noiommu_unbind(vdev);
> return;
> }
> @@ -116,7 +116,7 @@ int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> {
> lockdep_assert_held(&vdev->dev_set->lock);
>
> - if (vfio_device_is_noiommu(vdev))
> + if (vdev->noiommu)
> return 0;
>
> return vdev->ops->attach_ioas(vdev, pt_id);
> @@ -126,7 +126,7 @@ void vfio_iommufd_detach(struct vfio_device *vdev)
> {
> lockdep_assert_held(&vdev->dev_set->lock);
>
> - if (!vfio_device_is_noiommu(vdev))
> + if (!vdev->noiommu)
> vdev->ops->detach_ioas(vdev);
> }
>
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 50553f67600f..c8579d63b2b9 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -106,10 +106,11 @@ bool vfio_device_has_container(struct vfio_device *device);
> int __init vfio_group_init(void);
> void vfio_group_cleanup(void);
>
> -static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
> +static inline int vfio_device_set_noiommu(struct vfio_device *device)
> {
> - return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> - vdev->group->type == VFIO_NO_IOMMU;
> + device->noiommu = IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> + device->group->type == VFIO_NO_IOMMU;
> + return 0;
> }
>
> #if IS_ENABLED(CONFIG_VFIO_CONTAINER)
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 8c3f26b4929b..8979f320d620 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -289,8 +289,12 @@ static int __vfio_register_dev(struct vfio_device *device,
> if (ret)
> return ret;
>
> + ret = vfio_device_set_noiommu(device);
> + if (ret)
> + goto err_out;
> +
> ret = dev_set_name(&device->device, "%svfio%d",
> - vfio_device_is_noiommu(device) ? "noiommu-" : "", device->index);
> + device->noiommu ? "noiommu-" : "", device->index);
> if (ret)
> goto err_out;
>
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index cf9d082a623c..fa13889e763f 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -68,6 +68,7 @@ struct vfio_device {
> bool iommufd_attached;
> #endif
> bool cdev_opened:1;
> + bool noiommu:1;
> };
>
> /**
next prev parent reply other threads:[~2023-05-22 23:05 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-13 13:28 [PATCH v11 00/23] Add vfio_device cdev for iommufd support Yi Liu
2023-05-13 13:28 ` [PATCH v11 01/23] vfio: Allocate per device file structure Yi Liu
2023-05-13 13:28 ` [PATCH v11 02/23] vfio: Refine vfio file kAPIs for KVM Yi Liu
2023-05-13 13:28 ` [PATCH v11 03/23] vfio: Accept vfio device file in the KVM facing kAPI Yi Liu
2023-05-22 19:42 ` Alex Williamson
2023-05-13 13:28 ` [PATCH v11 04/23] kvm/vfio: Rename kvm_vfio_group to prepare for accepting vfio device fd Yi Liu
2023-05-13 13:28 ` [PATCH v11 05/23] kvm/vfio: Accept vfio device file from userspace Yi Liu
2023-05-13 13:28 ` [PATCH v11 06/23] vfio: Pass struct vfio_device_file * to vfio_device_open/close() Yi Liu
2023-05-13 13:28 ` [PATCH v11 07/23] vfio: Block device access via device fd until device is opened Yi Liu
2023-05-13 13:28 ` [PATCH v11 08/23] vfio: Add cdev_device_open_cnt to vfio_group Yi Liu
2023-05-13 13:28 ` [PATCH v11 09/23] vfio: Make vfio_device_open() single open for device cdev path Yi Liu
2023-05-13 13:28 ` [PATCH v11 10/23] vfio-iommufd: Move noiommu compat probe out of vfio_iommufd_bind() Yi Liu
2023-05-22 20:24 ` Alex Williamson
2023-05-23 0:45 ` Liu, Yi L
2023-05-13 13:28 ` [PATCH v11 11/23] vfio-iommufd: Split bind/attach into two steps Yi Liu
2023-05-13 13:28 ` [PATCH v11 12/23] vfio: Record devid in vfio_device_file Yi Liu
2023-05-13 13:28 ` [PATCH v11 13/23] vfio-iommufd: Add detach_ioas support for physical VFIO devices Yi Liu
2023-05-22 20:46 ` Alex Williamson
2023-05-22 20:59 ` Alex Williamson
2023-05-23 1:34 ` Liu, Yi L
2023-05-13 13:28 ` [PATCH v11 14/23] iommufd/device: Add iommufd_access_detach() API Yi Liu
2023-05-13 13:28 ` [PATCH v11 15/23] vfio-iommufd: Add detach_ioas support for emulated VFIO devices Yi Liu
2023-05-13 13:28 ` [PATCH v11 16/23] vfio: Name noiommu vfio_device with "noiommu-" prefix Yi Liu
2023-05-13 13:28 ` [PATCH v11 17/23] vfio: Move vfio_device_group_unregister() to be the first operation in unregister Yi Liu
2023-05-13 13:28 ` [PATCH v11 18/23] vfio: Add cdev for vfio_device Yi Liu
2023-05-13 13:28 ` [PATCH v11 19/23] vfio: Add VFIO_DEVICE_BIND_IOMMUFD Yi Liu
2023-05-22 22:01 ` Alex Williamson
2023-05-23 1:41 ` Liu, Yi L
2023-05-23 15:51 ` Alex Williamson
2023-05-24 2:20 ` Liu, Yi L
2023-05-24 2:39 ` Tian, Kevin
2023-05-24 2:40 ` Liu, Yi L
2023-05-24 8:31 ` Liu, Yi L
2023-05-13 13:28 ` [PATCH v11 20/23] vfio: Add VFIO_DEVICE_[AT|DE]TACH_IOMMUFD_PT Yi Liu
2023-05-22 22:15 ` Alex Williamson
2023-05-23 1:20 ` Liu, Yi L
2023-05-23 15:50 ` Alex Williamson
2023-05-24 2:12 ` Liu, Yi L
2023-05-24 15:31 ` Alex Williamson
2023-05-25 3:03 ` Liu, Yi L
2023-05-25 15:59 ` Alex Williamson
2023-05-26 8:38 ` Liu, Yi L
2023-06-06 14:42 ` Jason Gunthorpe
2023-06-06 14:40 ` Jason Gunthorpe
2023-05-13 13:28 ` [PATCH v11 21/23] vfio: Determine noiommu device in __vfio_register_dev() Yi Liu
2023-05-22 23:04 ` Alex Williamson [this message]
2023-05-23 2:13 ` Liu, Yi L
2023-05-24 8:14 ` Liu, Yi L
2023-05-13 13:28 ` [PATCH v11 22/23] vfio: Compile vfio_group infrastructure optionally Yi Liu
2023-05-13 13:28 ` [PATCH v11 23/23] docs: vfio: Add vfio device cdev description Yi Liu
2023-05-18 5:39 ` [PATCH v11 00/23] Add vfio_device cdev for iommufd support Xu, Terrence
2023-05-23 7:42 ` Duan, Zhenzhong
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=20230522170429.2d5ca274.alex.williamson@redhat.com \
--to=alex.williamson@redhat.com \
--cc=chao.p.peng@linux.intel.com \
--cc=clegoate@redhat.com \
--cc=cohuck@redhat.com \
--cc=eric.auger@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-gvt-dev@lists.freedesktop.org \
--cc=jasowang@redhat.com \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=lulu@redhat.com \
--cc=mjrosato@linux.ibm.com \
--cc=nicolinc@nvidia.com \
--cc=peterx@redhat.com \
--cc=robin.murphy@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=suravee.suthikulpanit@amd.com \
--cc=terrence.xu@intel.com \
--cc=xudong.hao@intel.com \
--cc=yan.y.zhao@intel.com \
--cc=yanting.jiang@intel.com \
--cc=yi.l.liu@intel.com \
--cc=yi.y.sun@linux.intel.com \
--cc=zhenzhong.duan@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