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
