RE: [PATCH 3/6] iommufd: Add IOMMU_DEVICE_GET_INFO

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Sent: Thursday, February 9, 2023 12:17 PM
> 
> +int iommufd_device_get_info(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_device_info *cmd = ucmd->cmd;
> +	struct iommufd_object *dev_obj;
> +	struct device *dev;
> +	const struct iommu_ops *ops;
> +	void *data;
> +	unsigned int length, data_len;
> +	int rc;
> +
> +	if (cmd->flags || cmd->__reserved || !cmd->data_len)
> +		return -EOPNOTSUPP;

do we want !cmd->data_len being a way to check how many bytes are
required in a following call to get the vendor info?

> +
> +	/*
> +	 * Zero the trailing bytes for userspace if the buffer is bigger
> +	 * than the data size kernel actually has.
> +	 */

"Zero the trailing bytes if the user buffer ..."

> +
> +/**
> + * struct iommu_device_info - ioctl(IOMMU_DEVICE_GET_INFO)
> + * @size: sizeof(struct iommu_device_info)
> + * @flags: Must be 0
> + * @dev_id: The device being attached to the IOMMU

iommufd

> + * @data_len: Input the type specific data buffer length in bytes

also is an output field

> + *
> + * Query the hardware iommu capability for given device which has been
> + * bound to iommufd. @data_len is set to be the size of the buffer to
> + * type specific data and the data will be filled. Trailing bytes are

"@data_len is the size of the buffer which captures iommu type specific data"

> + * zeroed if the user buffer is larger than the data kernel has.
> + *
> + * The type specific data would be used to sync capability between the
> + * vIOMMU and the hardware IOMMU, also for the availabillity checking of
> + * iommu hardware features like dirty page tracking in I/O page table.

It's fine to report format information related to stage-1 page table
which userspace manages.

but IMHO this should not be an interface to report which capability is
supported by iommufd. Having hardware supporting dirty bit 
doesn't mean the underlying iommu driver provides necessary support
to iommufd dirty tracking.

We either don't state this way if following what this series does which
simply dumps all raw iommu hw info to userspace, or explicitly require
iommu driver to only report information as required when a feature
is supported by iommufd.

> + *
> + * The @out_device_type will be filled if the ioctl succeeds. It would

s/will be/is/

> + * be used to decode the data filled in the buffer pointed by @data_ptr.

s/would be/is/

> + */
> +struct iommu_device_info {
> +	__u32 size;
> +	__u32 flags;
> +	__u32 dev_id;
> +	__u32 data_len;
> +	__aligned_u64 data_ptr;

moving forward iommu hw cap is not the only information reported
via this interface, e.g. it can be also used to report pasid mode.

from this angle it's better to rename above two fields to be iommu
specific, e.g.:

	__u32 iommu_data_len;
	__aligned_u64 iommu_data_ptr;

> +	__u32 out_device_type;
> +	__u32 __reserved;
> +};
> +#define IOMMU_DEVICE_GET_INFO _IO(IOMMUFD_TYPE,
> IOMMUFD_CMD_DEVICE_GET_INFO)
>  #endif
> --
> 2.34.1