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