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
> > +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
