Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 21 Sep 2021 14:40:49 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Liu Yi L <yi.l.liu@intel.com>
Cc:     alex.williamson@redhat.com, hch@lst.de, jasowang@redhat.com,
        joro@8bytes.org, jean-philippe@linaro.org, kevin.tian@intel.com,
        parav@mellanox.com, lkml@metux.net, pbonzini@redhat.com,
        lushenming@huawei.com, eric.auger@redhat.com, corbet@lwn.net,
        ashok.raj@intel.com, yi.l.liu@linux.intel.com,
        jun.j.tian@intel.com, hao.wu@intel.com, dave.jiang@intel.com,
        jacob.jun.pan@linux.intel.com, kwankhede@nvidia.com,
        robin.murphy@arm.com, kvm@vger.kernel.org,
        iommu@lists.linux-foundation.org, dwmw2@infradead.org,
        linux-kernel@vger.kernel.org, baolu.lu@linux.intel.com,
        david@gibson.dropbear.id.au, nicolinc@nvidia.com
Subject: Re: [RFC 10/20] iommu/iommufd: Add IOMMU_DEVICE_GET_INFO
Message-ID: <20210921174049.GV327412@nvidia.com>
References: <20210919063848.1476776-1-yi.l.liu@intel.com>
 <20210919063848.1476776-11-yi.l.liu@intel.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210919063848.1476776-11-yi.l.liu@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?heINA7s3zuIgTP4zUmXkD9P8K/srZAJEq9L1BrCgnvNJWx5Nm7buTW2s5KC/?=
 =?us-ascii?Q?KmXazhCxIpdECddk4YaQs14zxMuKUdvZuuO7F0oxW9wtH+TC5WybddaXnTgS?=
 =?us-ascii?Q?WXci4AoBzMGoZ20B5fsDEpj1Ku6l/bK+2HWDQGOKHJEienc0lND+tZlChYLu?=
 =?us-ascii?Q?uxu3dqqbHqK+JWG5TGjS7VZVN8k6WTTIG07qsTJVKj1SaxyiXRHlFSkGVjQM?=
 =?us-ascii?Q?gn62j/MZ4LTp3IEgsvUkinV538MjJ/WyMuSuAxv5Rm0R7fxbDSJmo1WI2u6x?=
 =?us-ascii?Q?OoQs5NDze+REkzD+KO/TL2oLpBpOym6GKej4VTR0a8K8AILIchnUieBPo4Bu?=
 =?us-ascii?Q?OQF4d8urQdqEX99qwxbzQSxolEwPaELXG2LIEKmbJnMr/zVx+YLnFjKsLvV6?=
 =?us-ascii?Q?JCdsaPQ82el1yrlifGd4W6uoxwjMqeEAHZRKgPb1bRsDSac62x4s5k1KNLt9?=
 =?us-ascii?Q?LvqR9P75BfA1tfPBmGazm6VvPzGnmIfQLjfAHJU267OPyF6JFzjRuLdEYdOV?=
 =?us-ascii?Q?h6uE7ZshKh+EgZIpdQegaFCzYwHpotVLh69rdO7KrarY1BAlbtt2o4u/1S+p?=
 =?us-ascii?Q?Xtjp8nMjE3VID8o7SSQU/ogvz6mCJmuxXw1menI9Qkuj9TpbXPxTRB+iscDw?=
 =?us-ascii?Q?2VUMYDWW2xBZlyrve+ePXwc7Oq9Ejixy6Zs45Xnv3vXwNQQyYQ2b3guSYMQ2?=
 =?us-ascii?Q?Abm9NSYS5fkCXtuh3RD4W3FBjl606oVIE7aEh0ETcPAmdhAF9+aVfBzavNPn?=
 =?us-ascii?Q?LaHp46YPt02XUNv3RPeDEIwosUOkuf2wrzYMICAgkAR69XoQrm6C/tAKqHiL?=
 =?us-ascii?Q?3TmKJh+emW4eU0Gc7MZYGsdXn38UB9BbUro5AODRf8DLPdKfMQF1MGwn2qXK?=
 =?us-ascii?Q?0vI74vWLX8J+rIy/P3rWq/l8dB+5is8H79p/b+fTuNWudadBSzS803Gl0hM6?=
 =?us-ascii?Q?vpQXAFjZNR4WMZMidw6cXlXui1PojpJiktnSZh+esfrimj+6iCoC1wmdAkUs?=
 =?us-ascii?Q?upGSfznnh3iWc0TnSNzY+Jg/7F5gaejZTn7Bfm5UlY61mfFBW25nITlbfnH1?=
 =?us-ascii?Q?IQEson+xQP6JzP5zyVoQX3z8MC+4jISVtFCZWcrnF7BarLIbWNcjSddfYV0/?=
 =?us-ascii?Q?feHcDZppZCIZDb3dlJ8DV/DCCjs28bix8Owi4Ha8B3Sx+mFMTZcrYtiVxtgp?=
 =?us-ascii?Q?/1sHLEtdgCTmho/TEfFDXgNJ5hf1gUggvB+TA+aYGjckQ16hB9BUSJXYb8B3?=
 =?us-ascii?Q?wjW7IAYb77VfRJEaw4/Q+AdLBNS/dEk+S3xpX7c3YkCtlYKW2Db9vIsgXAmO?=
 =?us-ascii?Q?kEDfCJYQ4p4kPmnfKzX7cMP/?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a271b1c3-bbf8-4e64-cf4a-08d97d26f38e
X-MS-Exchange-CrossTenant-AuthSource: BL0PR12MB5506.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Sep 2021 17:40:50.0576
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XueyFe4nxjdYjIyaDTPkJdMXTDve45vQTGSULLgppeg/XHa0btLGLnOGw2nouHdY
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5144
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@intel.com>
>  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