Re: [PATCH v5 11/19] iommufd: IOCTLs for the io_pagetable

[Eric Auger <eric.auger@redhat.com>]

On 11/28/22 19:27, Jason Gunthorpe wrote:
> On Sun, Nov 27, 2022 at 06:49:29PM +0100, Eric Auger wrote:
>
>>> +static int iommufd_ioas_load_iovas(struct rb_root_cached *itree,
>>> +				   struct iommu_iova_range __user *ranges,
>>> +				   u32 num)
>>> +{
>>> +	u32 i;
>>> +
>>> +	for (i = 0; i != num; i++) {
>> shouldn't it be < ?
> It is logically equivalent
damn. That sometimes happens to me when staring at so much code ;-)
>
>>> +int iommufd_ioas_allow_iovas(struct iommufd_ucmd *ucmd)
>>> +{
>>> +	struct iommu_ioas_allow_iovas *cmd = ucmd->cmd;
>>> +	struct rb_root_cached allowed_iova = RB_ROOT_CACHED;
>>> +	struct interval_tree_node *node;
>>> +	struct iommufd_ioas *ioas;
>>> +	struct io_pagetable *iopt;
>>> +	int rc = 0;
>>> +
>>> +	if (cmd->__reserved)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	ioas = iommufd_get_ioas(ucmd, cmd->ioas_id);
>>> +	if (IS_ERR(ioas))
>>> +		return PTR_ERR(ioas);
>>> +	iopt = &ioas->iopt;
>>> +
>>> +	rc = iommufd_ioas_load_iovas(&allowed_iova,
>>> +				     u64_to_user_ptr(cmd->allowed_iovas),
>>> +				     cmd->num_iovas);
>>> +	if (rc)
>>> +		goto out_free;
>>> +
>>> +	rc = iopt_set_allow_iova(iopt, &allowed_iova);
>> Please can you add a comment about why you need to proceed in 2 steps,
>> ie. add the ranges in a first tree and then 'swap' to the
>> iopt->allowed_tree (and eventually delete the first tree)?
> Sure
>
> 	/*
> 	 * We want the allowed tree update to be atomic, so we have to keep the
> 	 * original nodes around, and keep track of the new nodes as we allocate
> 	 * memory for them. The simplest solution is to have a new/old tree and
> 	 * then swap new for old. On success we free the old tree, on failure we
> 	 * free the new tree.
> 	 */
>
>>> +static int conv_iommu_prot(u32 map_flags)
>>> +{
>>> +	int iommu_prot;
>>> +
>>> +	/*
>>> +	 * We provide no manual cache coherency ioctls to userspace and most
>>> +	 * architectures make the CPU ops for cache flushing privileged.
>>> +	 * Therefore we require the underlying IOMMU to support CPU coherent
>>> +	 * operation. Support for IOMMU_CACHE is enforced by the
>>> +	 * IOMMU_CAP_CACHE_COHERENCY test during bind.
>>> +	 */
>>> +	iommu_prot = IOMMU_CACHE;
>> at init?
> done
>
>>> +int iommufd_ioas_map(struct iommufd_ucmd *ucmd)
>>> +{
>>> +	struct iommu_ioas_map *cmd = ucmd->cmd;
>>> +	struct iommufd_ioas *ioas;
>>> +	unsigned int flags = 0;
>>> +	unsigned long iova;
>>> +	int rc;
>>> +
>>> +	if ((cmd->flags &
>>> +	     ~(IOMMU_IOAS_MAP_FIXED_IOVA | IOMMU_IOAS_MAP_WRITEABLE |
>>> +	       IOMMU_IOAS_MAP_READABLE)) ||
>>> +	    cmd->__reserved)
>>> +		return -EOPNOTSUPP;
>>> +	if (cmd->iova >= ULONG_MAX || cmd->length >= ULONG_MAX)
>>> +		return -EOVERFLOW;
>>> +
>>> +	ioas = iommufd_get_ioas(ucmd, cmd->ioas_id);
>>> +	if (IS_ERR(ioas))
>>> +		return PTR_ERR(ioas);
>>> +
>>> +	if (!(cmd->flags & IOMMU_IOAS_MAP_FIXED_IOVA))
>>> +		flags = IOPT_ALLOC_IOVA;
>>> +	iova = cmd->iova;
>> can be done either at initialization or only if MAP_FIXED_IOVA.
> Done
>
>
>>> +int iommufd_option_rlimit_mode(struct iommu_option *cmd,
>>> +			       struct iommufd_ctx *ictx)
>>> +{
>> *object_id  and __reserved should be checked as per the uapi doc*
> Ohh, yes, thanks:
>
> @@ -317,6 +322,9 @@ int iommufd_ioas_unmap(struct iommufd_ucmd *ucmd)
>  int iommufd_option_rlimit_mode(struct iommu_option *cmd,
>                                struct iommufd_ctx *ictx)
>  {
> +       if (cmd->object_id)
> +               return -EOPNOTSUPP;
> +
>         if (cmd->op == IOMMU_OPTION_OP_GET) {
>                 cmd->val64 = ictx->account_mode == IOPT_PAGES_ACCOUNT_MM;
>                 return 0;
> diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
> index de5cc01023c0c5..bcb463e581009c 100644
> --- a/drivers/iommu/iommufd/main.c
> +++ b/drivers/iommu/iommufd/main.c
> @@ -215,6 +215,9 @@ static int iommufd_option(struct iommufd_ucmd *ucmd)
>         struct iommu_option *cmd = ucmd->cmd;
>         int rc;
>  
> +       if (cmd->__reserved)
> +               return -EOPNOTSUPP;
> +
>         switch (cmd->option_id) {
>         case IOMMU_OPTION_RLIMIT_MODE:
>                 rc = iommufd_option_rlimit_mode(cmd, ucmd->ictx);
>
>>> +/**
>>> + * struct iommu_ioas_iova_ranges - ioctl(IOMMU_IOAS_IOVA_RANGES)
>>> + * @size: sizeof(struct iommu_ioas_iova_ranges)
>>> + * @ioas_id: IOAS ID to read ranges from
>>> + * @num_iovas: Input/Output total number of ranges in the IOAS
>>> + * @__reserved: Must be 0
>>> + * @allowed_iovas: Pointer to the output array of struct iommu_iova_range
>>> + * @out_iova_alignment: Minimum alignment required for mapping IOVA
>>> + *
>>> + * Query an IOAS for ranges of allowed IOVAs. Mapping IOVA outside these ranges
>>> + * is not allowed. num_iovas will be set to the total number of iovas and
>>> + * the allowed_iovas[] will be filled in as space permits.
>>> + *
>>> + * The allowed ranges are dependent on the HW path the DMA operation takes, and
>>> + * can change during the lifetime of the IOAS. A fresh empty IOAS will have a
>>> + * full range, and each attached device will narrow the ranges based on that
>>> + * device's HW restrictions. Detatching a device can widen the ranges. Userspace
>> detaching
>>> + * should query ranges after every attach/detatch to know what IOVAs are valid
>> detach
> Done
>
>>> + * for mapping.
>>> + *
>>> + * On input num_iovas is the length of the allowed_iovas array. On output it is
>>> + * the total number of iovas filled in. The ioctl will return -EMSGSIZE and set
>>> + * num_iovas to the required value if num_iovas is too small. In this case the
>>> + * caller should allocate a larger output array and re-issue the ioctl.
>>> + */
>>> +struct iommu_ioas_iova_ranges {
>>> +	__u32 size;
>>> +	__u32 ioas_id;
>>> +	__u32 num_iovas;
>>> +	__u32 __reserved;
>>> +	__aligned_u64 allowed_iovas;
>>> +	__aligned_u64 out_iova_alignment;
>> document @out_iova_alignment?
>  * out_iova_alignment returns the minimum IOVA alignment that can be given
>  * to IOMMU_IOAS_MAP/COPY. IOVA's must satisfy:
>  *   starting_iova % out_iova_alignment == 0
>  *   (starting_iova + length) % out_iova_alignment == 0
>  * out_iova_alignment can be 1 indicating any IOVA is allowed. It cannot
>  * be higher than the system PAGE_SIZE.
>
>>> +/**
>>> + * struct iommu_ioas_map - ioctl(IOMMU_IOAS_MAP)
>>> + * @size: sizeof(struct iommu_ioas_map)
>>> + * @flags: Combination of enum iommufd_ioas_map_flags
>>> + * @ioas_id: IOAS ID to change the mapping of
>>> + * @__reserved: Must be 0
>>> + * @user_va: Userspace pointer to start mapping from
>>> + * @length: Number of bytes to map
>>> + * @iova: IOVA the mapping was placed at. If IOMMU_IOAS_MAP_FIXED_IOVA is set
>>> + *        then this must be provided as input.
>>> + *
>>> + * Set an IOVA mapping from a user pointer. If FIXED_IOVA is specified then the
>>> + * mapping will be established at iova, otherwise a suitable location based on
>>> + * the reserved and allowed lists will be automatically selected and returned in
>>> + * iova.
>> You do not mention anything about the fact the IOCTL cannot be called
>> twice for a given @user_va w/ FIXED_IOVA
>> Refering to VFIO_DMA_MAP_FLAG_VADDR.
>  * If IOMMU_IOAS_MAP_FIXED_IOVA is specified then the iova range must currently
>  * be unused, existing IOVA cannot be replaced.
>
>>> +/**
>>> + * struct iommu_ioas_copy - ioctl(IOMMU_IOAS_COPY)
>>> + * @size: sizeof(struct iommu_ioas_copy)
>>> + * @flags: Combination of enum iommufd_ioas_map_flags
>>> + * @dst_ioas_id: IOAS ID to change the mapping of
>>> + * @src_ioas_id: IOAS ID to copy from
>>> + * @length: Number of bytes to copy and map
>>> + * @dst_iova: IOVA the mapping was placed at. If IOMMU_IOAS_MAP_FIXED_IOVA is
>>> + *            set then this must be provided as input.
>>> + * @src_iova: IOVA to start the copy
>>> + *
>>> + * Copy an already existing mapping from src_ioas_id and establish it in
>>> + * dst_ioas_id. The src iova/length must exactly match a range used with
>>> + * IOMMU_IOAS_MAP.
>>> + *
>>> + * This may be used to efficiently clone a subset of an IOAS to another, or as a
>>> + * kind of 'cache' to speed up mapping. Copy has an effciency advantage over
>> efficiency
>>> + * establishing equivalent new mappings, as internal resources are shared, and
>>> + * the kernel will pin the user memory only once.
>>> + */
>>> +struct iommu_ioas_copy {
>>> +	__u32 size;
>>> +	__u32 flags;
>>> +	__u32 dst_ioas_id;
>>> +	__u32 src_ioas_id;
>> is src_ioas_id == dst_ioas_id allowed?
> Yes
>
>>> +/**
>>> + * struct iommu_option - iommu option multiplexer
>>> + * @size: sizeof(struct iommu_option)
>>> + * @option_id: One of enum iommufd_option
>>> + * @op: One of enum iommufd_option_ops
>>> + * @__reserved: Must be 0
>>> + * @object_id: ID of the object if required
>>> + * @val64: Option value to set or value returned on get
>>> + *
>>> + * Change a simple option value. This multiplexor allows controlling a options
>> s/a options/options
> Done
>
> Thanks,
> Jason
>
Eric