Received-SPF: pass (google.com: domain of linux-kernel+bounces-176906-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 12 May 2024 11:58:27 -0300
From: Jason Gunthorpe <jgg@nvidia.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: will@kernel.org, robin.murphy@arm.com, kevin.tian@intel.com,
	suravee.suthikulpanit@amd.com, joro@8bytes.org,
	linux-kernel@vger.kernel.org, iommu@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org,
	yi.l.liu@intel.com, eric.auger@redhat.com, vasant.hegde@amd.com,
	jon.grimm@amd.com, santosh.shukla@amd.com, Dhaval.Giani@amd.com,
	shameerali.kolothum.thodi@huawei.com
Subject: Re: [PATCH RFCv1 08/14] iommufd: Add IOMMU_VIOMMU_SET_DEV_ID ioctl
Message-ID: <ZkDZE32JFyKprmpi@nvidia.com>
References: <cover.1712978212.git.nicolinc@nvidia.com>
 <c97a98a72ee3498c587e5898d6b899553ccd9b27.1712978212.git.nicolinc@nvidia.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c97a98a72ee3498c587e5898d6b899553ccd9b27.1712978212.git.nicolinc@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?tuovzTTkf2C5By54+w7SelM02DqzO3CEtF84URTzT6Dhb1YCklPihDgbbYg8?=
 =?us-ascii?Q?MnJwciACcvcj2391u13mh7mx9Me3gMCagVuC0GTldeTXl7pVhX673+Oq52bO?=
 =?us-ascii?Q?eavXAuG0jBtKxDIOi+f4kEhSWEU8tJx9fwn6cOCb5FvJk5KuvGN2+5CCE1hV?=
 =?us-ascii?Q?HrBKqwz5t89w9ZQ4PIGsz/ZtlRStKDA9K/63X7O5DhH7tyUVA9H8BArO509s?=
 =?us-ascii?Q?SJeOI8bNe8iIiwrNXDLW1ZOzc6+bA0Nw2IfgnzbibjVmk/VQpeqo8xYWz7zw?=
 =?us-ascii?Q?KGbkNsFCu9r4PuRCuqs8S5i1Cz8YawDuwtDEhGXUt/exwgVlTPeqTFyZdrLD?=
 =?us-ascii?Q?XbpUboBWs6YrTeXivoOgJ2461mbATu7fSJ5X9CPLWmc6YDJZ0riB/lBWT4bN?=
 =?us-ascii?Q?0rCLv+XaP+Vk//cvqeXOhILb3Nf3Fcu6m0ZQLL7fGWVUQnxztLbdj6dVgCxY?=
 =?us-ascii?Q?ehPQUDIYc7HMHQLU0HMZTsZfOORTFCuCxSAU7q08I+fsCnX35sKMd/odUsSN?=
 =?us-ascii?Q?BKjEzxwMAyzWFff4Aki3yEv/VGugpbmGg1AuW1EcbxPgbJ8xGNcoILb8a7a5?=
 =?us-ascii?Q?udSmtkIb4Xhek7IjmNK3BXFMmJCEu5B8MvXUn5lBjstvUxFLbRECQGqzR7Qq?=
 =?us-ascii?Q?BAs4/lNbIv9pNEeSrvQSnK8uN79Wcg244SBjjfoPrg/NzKUnEK5j9mIjvRoE?=
 =?us-ascii?Q?5P2dJMLT8NQ8p2jY3BPOfS5XIRJtYjgVY25jUbcycQpBpVaYXX2lSL8HcXRg?=
 =?us-ascii?Q?w/zYHMOWzKy6MZgEbHYQ9kinUp1td9uYOe5UaxAmi4dZYLSLI5qUn9ebGDEJ?=
 =?us-ascii?Q?x6yxwY5WG2TkjWRwiETijpy8BQNGTHPLjgkHTOUQnGjop9ARF9CPYKO/nIt1?=
 =?us-ascii?Q?dGcRhm/nlWqXHZMCxvGqphpps32pQWwODYCAxJZVML8UX67Sxr1cf7jBWDLM?=
 =?us-ascii?Q?hXH0tX9gukmFX6lKv1WsIygrlcn7FOmQNAN7Su6tuScXT/yXsKqPPhn3mWQr?=
 =?us-ascii?Q?REKUHg81k3Ygy11KfpwGnNUmwXZxibSSVaSXAcBrcgYOQstDW9UOfvZEnFpn?=
 =?us-ascii?Q?4oFDuu/+pYOvjrl/5tCn9XGpBSFMeS458faxUhb/RWxoAx6ZenC3ylu7Khyh?=
 =?us-ascii?Q?NpLozcWh4S0wwpFXDHr4mgrGwQVpXQ/mNilhz50jG+51ztQ1CT9BFdnn9o60?=
 =?us-ascii?Q?PFnJgGnIqgp9MB8TNoNshDPm+uQMuEK9W9Au6Gjg/ppue7TcOnQZRdaIuDOr?=
 =?us-ascii?Q?ZzoTRM9ql7/dzm1dvuobHrB3UlYmjF7XTcSHU6aVYQlfri/4rk6x2lbzxqWf?=
 =?us-ascii?Q?GGsPEytEmlfyeyUEEL6jJMtqb5KGONRsM4zQx4JuhFDgP+oRxD/85P1iZXs3?=
 =?us-ascii?Q?LbM/pxs8ATOEH89D3OEH8YGhU7HwM1UfRrc6JdG7YG6KIIxWwxn/5Zij5rXd?=
 =?us-ascii?Q?eM/BwOSgqrNuzGT5yRm//er5ykZxH1oumSMwaM8GrIF/TlTk4x1yotYLhlhW?=
 =?us-ascii?Q?2noNBZlKSD4zjXVWx2PnPLmsfmfBNUjvH+ljd16vtw5JlSlSAmTXDj8g2rXV?=
 =?us-ascii?Q?fKisDuOMS3tc8X9mQGiL6ceN9xsDWu91MxK1bGUn?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fe0156cd-6b41-4eb1-80a0-08dc729400f7
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2024 14:58:37.8751
 (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: Pa1viGjYNeWTWDB69omCLBCH8mZuYZdUo4xKydeQzpXJHSkkbxLnn6jKFrKCmSo8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7643

On Fri, Apr 12, 2024 at 08:47:05PM -0700, Nicolin Chen wrote:
> Introduce a new ioctl to set a per-viommu device virtual id that should be
> linked to the physical device id (or just a struct device pointer).
> 
> Since a viommu (user space IOMMU instance) can have multiple devices while
> it's not ideal to confine a device to one single user space IOMMU instance
> either, these two shouldn't just do a 1:1 mapping. Add two xarrays in their
> structures to bind them bidirectionally.

Since I would like to retain the dev_id, I think this is probably
better done with an allocated struct per dev-id:

struct dev_id {
    struct iommufd_device *idev;
    struct iommufd_viommu *viommu;
    u64 vdev_id;
    u64 driver_private; // Ie the driver can store the pSID here
    struct list_head idev_entry;
};

> @@ -135,7 +135,16 @@ void iommufd_device_destroy(struct iommufd_object *obj)
>  {
>  	struct iommufd_device *idev =
>  		container_of(obj, struct iommufd_device, obj);
> +	struct iommufd_viommu *viommu;
> +	unsigned long index;
>  
> +	xa_for_each(&idev->viommus, index, viommu) {
> +		if (viommu->ops->unset_dev_id)
> +			viommu->ops->unset_dev_id(viommu, idev->dev);
> +		xa_erase(&viommu->idevs, idev->obj.id);
> +		xa_erase(&idev->viommus, index);
> +	}

Then this turns into list_for_each(idev->viommu_vdevid_list)

> +int iommufd_viommu_set_device_id(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_viommu_set_dev_id *cmd = ucmd->cmd;
> +	unsigned int dev_id, viommu_id;
> +	struct iommufd_viommu *viommu;
> +	struct iommufd_device *idev;
> +	int rc;
> +
> +	idev = iommufd_get_device(ucmd, cmd->dev_id);
> +	if (IS_ERR(idev))
> +		return PTR_ERR(idev);
> +	dev_id = idev->obj.id;
> +
> +	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
> +	if (IS_ERR(viommu)) {
> +		rc = PTR_ERR(viommu);
> +		goto out_put_idev;
> +	}
> +
> +	if (viommu->iommu_dev != idev->dev->iommu->iommu_dev) {
> +		rc = -EINVAL;
> +		goto out_put_viommu;
> +	}
> +
> +	if (!viommu->ops->set_dev_id || !viommu->ops->unset_dev_id) {
> +		rc = -EOPNOTSUPP;
> +		goto out_put_viommu;
> +	}
> +
> +	rc = xa_alloc(&idev->viommus, &viommu_id, viommu,
> +		      XA_LIMIT(viommu->obj.id, viommu->obj.id),
> +		      GFP_KERNEL_ACCOUNT);
> +	if (rc)
> +		goto out_put_viommu;
> +
> +	rc = xa_alloc(&viommu->idevs, &dev_id, idev,
> +		      XA_LIMIT(dev_id, dev_id), GFP_KERNEL_ACCOUNT);
> +	if (rc)
> +		goto out_xa_erase_viommu;

Both of these are API mis-uses, you don't want an allocating xarray
you just want to use xa_cmpxchg

Put an xarray in the viommu object and fill it with pointers to the
struct dev_id thing above

The driver can piggyback on this xarray too if it wants, so we only
need one.

xa_cmpchg to install the user requests vdev_id only if the vdev_id is
already unused.

> @@ -51,6 +51,7 @@ enum {
>  	IOMMUFD_CMD_HWPT_GET_DIRTY_BITMAP,
>  	IOMMUFD_CMD_HWPT_INVALIDATE,
>  	IOMMUFD_CMD_VIOMMU_ALLOC,
> +	IOMMUFD_CMD_VIOMMU_SET_DEV_ID,
>  };

We almost certainly will need a REMOVE_DEV_ID so userspace can have
sensible error handling.

> +
> +/**
> + * struct iommu_viommu_set_dev_id - ioctl(IOMMU_VIOMMU_SET_DEV_ID)
> + * @size: sizeof(struct iommu_viommu_set_dev_id)
> + * @viommu_id: viommu ID to associate with the device to store its virtual ID
> + * @dev_id: device ID to set a device virtual ID
> + * @__reserved: Must be 0
> + * @id: Device virtual ID
> + *
> + * Set a viommu-specific virtual ID of a device
> + */
> +struct iommu_viommu_set_dev_id {
> +	__u32 size;
> +	__u32 viommu_id;
> +	__u32 dev_id;
> +	__u32 __reserved;
> +	__aligned_u64 id;

I think I'd consistently call this vdev_id throughout the API

Jason