On Sun, Sep 19, 2021 at 02:38:38PM +0800, Liu Yi L wrote:
> After a device is bound to the iommufd, userspace can use this interface
> to query the underlying iommu capability and format info for this device.
> Based on this information the user then creates I/O address space in a
> compatible format with the to-be-attached devices.
>
> Device cookie which is registered at binding time is used to mark the
> device which is being queried here.
>
> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> drivers/iommu/iommufd/iommufd.c | 68 +++++++++++++++++++++++++++++++++
> include/uapi/linux/iommu.h | 49 ++++++++++++++++++++++++
> 2 files changed, 117 insertions(+)
>
> diff --git a/drivers/iommu/iommufd/iommufd.c b/drivers/iommu/iommufd/iommufd.c
> index e16ca21e4534..641f199f2d41 100644
> +++ b/drivers/iommu/iommufd/iommufd.c
> @@ -117,6 +117,71 @@ static int iommufd_fops_release(struct inode *inode, struct file *filep)
> return 0;
> }
>
> +static struct device *
> +iommu_find_device_from_cookie(struct iommufd_ctx *ictx, u64 dev_cookie)
> +{
We have an xarray ID for the device, why are we allowing userspace to
use the dev_cookie as input?
Userspace should always pass in the ID. The only place dev_cookie
should appear is if the kernel generates an event back to
userspace. Then the kernel should return both the ID and the
dev_cookie in the event to allow userspace to correlate it.
> +static void iommu_device_build_info(struct device *dev,
> + struct iommu_device_info *info)
> +{
> + bool snoop;
> + u64 awidth, pgsizes;
> +
> + if (!iommu_device_get_info(dev, IOMMU_DEV_INFO_FORCE_SNOOP, &snoop))
> + info->flags |= snoop ? IOMMU_DEVICE_INFO_ENFORCE_SNOOP : 0;
> +
> + if (!iommu_device_get_info(dev, IOMMU_DEV_INFO_PAGE_SIZE, &pgsizes)) {
> + info->pgsize_bitmap = pgsizes;
> + info->flags |= IOMMU_DEVICE_INFO_PGSIZES;
> + }
> +
> + if (!iommu_device_get_info(dev, IOMMU_DEV_INFO_ADDR_WIDTH, &awidth)) {
> + info->addr_width = awidth;
> + info->flags |= IOMMU_DEVICE_INFO_ADDR_WIDTH;
> + }
Another good option is to push the iommu_device_info uAPI struct down
through to the iommu driver to fill it in and forget about the crazy
enum.
A big part of thinking of this iommu interface is a way to bind the HW
IOMMU driver to a uAPI and allow the HW driver to expose its unique
functionalities.
> +static int iommufd_get_device_info(struct iommufd_ctx *ictx,
> + unsigned long arg)
> +{
> + struct iommu_device_info info;
> + unsigned long minsz;
> + struct device *dev;
> +
> + minsz = offsetofend(struct iommu_device_info, addr_width);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
All of these patterns everywhere are wrongly coded for forward/back
compatibility.
static int iommufd_get_device_info(struct iommufd_ctx *ictx,
struct iommu_device_info __user *arg, size_t usize)
{
struct iommu_device_info info;
int ret;
if (usize < offsetofend(struct iommu_device_info, addr_flags))
return -EINVAL;
ret = copy_struct_from_user(&info, sizeof(info), arg, usize);
if (ret)
return ret;
'usize' should be in a 'common' header extracted by the main ioctl handler.
> +struct iommu_device_info {
> + __u32 argsz;
> + __u32 flags;
> +#define IOMMU_DEVICE_INFO_ENFORCE_SNOOP (1 << 0) /* IOMMU enforced snoop */
> +#define IOMMU_DEVICE_INFO_PGSIZES (1 << 1) /* supported page sizes */
> +#define IOMMU_DEVICE_INFO_ADDR_WIDTH (1 << 2) /* addr_wdith field valid */
> + __u64 dev_cookie;
> + __u64 pgsize_bitmap;
> + __u32 addr_width;
> +};
Be explicit with padding here too.
Jason
