Re: [Xen-devel] [PATCH v6 11/14] x86: add iommu_op to enable modification of IOMMU mappings

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> This patch adds an iommu_op which checks whether it is possible or
> safe for a domain to modify its own IOMMU mappings and, if so, creates
> a rangeset to track modifications.

Now this can surely grow pretty big?

> --- a/xen/common/iommu_op.c
> +++ b/xen/common/iommu_op.c
> @@ -78,6 +78,51 @@ static int iommu_op_query_reserved(struct 
> xen_iommu_op_query_reserved *op)
>      return 0;
>  }
>  
> +static int iommu_op_enable_modification(
> +    struct xen_iommu_op_enable_modification *op)
> +{
> +    struct domain *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    const struct iommu_ops *ops = iommu->platform_ops;
> +
> +    if ( op->cap || op->pad )
> +        return -EINVAL;
> +
> +    /*
> +     * XEN_IOMMU_CAP_per_device_mappings is not supported yet so we can
> +     * leave op->cap alone.
> +     */
> +
> +    /* Has modification already been enabled? */
> +    if ( iommu->iommu_op_ranges )
> +        return 0;

I don't recall there being any synchronization around the check
here until ...

> +    /*
> +     * The IOMMU mappings cannot be modified if:
> +     * - the IOMMU is not enabled or,
> +     * - the current domain is dom0 and tranlsation is disabled or,
> +     * - HAP is enabled and the IOMMU shares the mappings.
> +     */
> +    if ( !iommu_enabled ||
> +         (is_hardware_domain(currd) && iommu_passthrough) ||
> +         iommu_use_hap_pt(currd) )
> +        return -EACCES;
> +
> +    /*
> +     * The IOMMU implementation must provide the lookup method if
> +     * modification of the mappings is to be supported.
> +     */
> +    if ( !ops->lookup_page )
> +        return -EOPNOTSUPP;
> +
> +    iommu->iommu_op_ranges = rangeset_new(currd, NULL, 0);

... the assignment here. In which case this is a (multi-vCPU) guest
controllable leak.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -26,7 +26,6 @@ static void iommu_dump_p2m_table(unsigned char key);
>  
>  unsigned int __read_mostly iommu_dev_iotlb_timeout = 1000;
>  integer_param("iommu_dev_iotlb_timeout", iommu_dev_iotlb_timeout);
> -
>  /*
>   * The 'iommu' parameter enables the IOMMU.  Optional comma separated
>   * value may contain:

Stray change?

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1460,7 +1460,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      }
>  
>   done:
> -    if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
> +    if ( !has_arch_pdevs(d) && has_iommu_pt(d) && !hd->iommu_op_ranges )
>          iommu_teardown(d);
>      pcidevs_unlock();
>  
> @@ -1510,7 +1510,7 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, 
> u8 devfn)
>  
>      pdev->fault.count = 0;
>  
> -    if ( !has_arch_pdevs(d) && has_iommu_pt(d) )
> +    if ( !has_arch_pdevs(d) && has_iommu_pt(d) && !hd->iommu_op_ranges )
>          iommu_teardown(d);

These additions are pretty un-obvious, and hence at least need
comments. But I'm also unclear about the intended behavior: For
a guest not meaning to play with its mappings, why would you
keep the tables around (and the memory uselessly allocated)?

> --- a/xen/include/public/iommu_op.h
> +++ b/xen/include/public/iommu_op.h
> @@ -61,6 +61,25 @@ struct xen_iommu_op_query_reserved {
>      XEN_GUEST_HANDLE(xen_iommu_reserved_range_t) ranges;
>  };
>  
> +/*
> + * XEN_IOMMUOP_enable_modification: Enable operations that modify IOMMU
> + *                                  mappings.
> + */
> +#define XEN_IOMMUOP_enable_modification 2
> +
> +struct xen_iommu_op_enable_modification {
> +    /*
> +     * OUT - On successful return this is set to the bitwise OR of 
> capabilities
> +     *       defined below. On entry this must be set to zero.
> +     */
> +    unsigned int cap;
> +    unsigned int pad;

uint32_t

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -79,6 +79,7 @@
>  ?    vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
>  !    iommu_op                        iommu_op.h
>  !    iommu_op_buf                    iommu_op.h
> +!    iommu_op_enable_modification    iommu_op.h

The structure above looks to be 32-/64-bit agnostic. Why is this !
instead of ? ?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel