Re: [PATCH v5 12/15] x86: add iommu_op to enable modification of IOMMU mappings

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Paul Durrant
> Sent: Saturday, August 4, 2018 1:22 AM
> 
> 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.

Have to say that there might be a concept mismatch between us,
so I will stop review here until we get aligned on the basic 
understanding. 

What an IOMMU does is to provide DMA isolation between devices. 
Each device can be hooked with a different translation structure 
(representing a different bfn address space). Linux kernel uses this
mechanism to harden kernel drivers (through dma APIs). Multiple 
devices can be also attached to the same address space, used by
hypervisor when devices are assigned to the same VM.

Now with pvIOMMU exposed to dom0, , dom0 could use it to harden 
kernel drivers too. Then there will be multiple bfn address spaces:

- A default bfn address space created by Xen, where bfn = pfn
- multiple per-bdf bfn address spaces created by Dom0, where
bfn is completely irrelevant to pfn.

the default space should not be changed by Dom0. It is attached
to devices which dom0 doesn't enable pviommu mapping.

per-bdf address spaces can be changed by Dom0, attached to
devices which dom0 enables pviommu mapping. then pviommu ops
should accept a bdf parameter. and internally Xen needs to maintain
multiple page tables under dom0, and find a right page table according
to specified bdf to complete the operation.

Now your series look assuming always just one bfn address space
cross all assigned devices per domain... I'm not sure how it works.

Did I misunderstand anything?

> 
> NOTE: The actual map and unmap operations are introduced by
> subsequent
>       patches.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
> v4:
>  - Set sync_iommu_pt to false instead of need_iommu.
> 
> v2:
>  - New in v2.
> ---
>  xen/arch/x86/iommu_op.c         | 42
> +++++++++++++++++++++++++++++++++++++++++
>  xen/drivers/passthrough/iommu.c |  2 +-
>  xen/drivers/passthrough/pci.c   |  4 ++--
>  xen/include/public/iommu_op.h   |  6 ++++++
>  xen/include/xen/iommu.h         |  3 +++
>  5 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/iommu_op.c b/xen/arch/x86/iommu_op.c
> index bcfcd49102..b29547bffd 100644
> --- a/xen/arch/x86/iommu_op.c
> +++ b/xen/arch/x86/iommu_op.c
> @@ -78,6 +78,42 @@ static int iommu_op_query_reserved(struct
> xen_iommu_op_query_reserved *op)
>      return 0;
>  }
> 
> +static int iommu_op_enable_modification(void)
> +{
> +    struct domain *currd = current->domain;
> +    struct domain_iommu *iommu = dom_iommu(currd);
> +    const struct iommu_ops *ops = iommu->platform_ops;
> +
> +    /* Has modification already been enabled? */
> +    if ( iommu->iommu_op_ranges )
> +        return 0;
> +
> +    /*
> +     * 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);
> +    if ( !iommu->iommu_op_ranges )
> +        return -ENOMEM;
> +
> +    currd->sync_iommu_pt = 0; /* Disable synchronization */
> +    return 0;
> +}
> +
>  static void iommu_op(xen_iommu_op_t *op)
>  {
>      switch ( op->op )
> @@ -86,6 +122,10 @@ static void iommu_op(xen_iommu_op_t *op)
>          op->status = iommu_op_query_reserved(&op->u.query_reserved);
>          break;
> 
> +    case XEN_IOMMUOP_enable_modification:
> +        op->status = iommu_op_enable_modification();
> +        break;
> +
>      default:
>          op->status = -EOPNOTSUPP;
>          break;
> @@ -98,6 +138,7 @@ int do_one_iommu_op(xen_iommu_op_buf_t *buf)
>      size_t offset;
>      static const size_t op_size[] = {
>          [XEN_IOMMUOP_query_reserved] = sizeof(struct
> xen_iommu_op_query_reserved),
> +        [XEN_IOMMUOP_enable_modification] = 0,
>      };
>      size_t size;
>      int rc;
> @@ -184,6 +225,7 @@ int
> compat_one_iommu_op(compat_iommu_op_buf_t *buf)
>      size_t offset;
>      static const size_t op_size[] = {
>          [XEN_IOMMUOP_query_reserved] = sizeof(struct
> compat_iommu_op_query_reserved),
> +        [XEN_IOMMUOP_enable_modification] = 0,
>      };
>      size_t size;
>      xen_iommu_op_t nat;
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index caf3d125ae..8f635a5cdb 100644
> --- 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:
> @@ -265,6 +264,7 @@ void iommu_domain_destroy(struct domain *d)
>      arch_iommu_domain_destroy(d);
> 
>      rangeset_destroy(hd->reserved_ranges);
> +    rangeset_destroy(hd->iommu_op_ranges);
>  }
> 
>  int iommu_map_page(struct domain *d, bfn_t bfn, mfn_t mfn,
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3d3ad484e7..d4033af41a 100644
> --- 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);
> 
>      return ret;
> diff --git a/xen/include/public/iommu_op.h
> b/xen/include/public/iommu_op.h
> index ade404a877..9bf74bd007 100644
> --- a/xen/include/public/iommu_op.h
> +++ b/xen/include/public/iommu_op.h
> @@ -61,6 +61,12 @@ 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 {
>      uint16_t op;    /* op type */
>      uint16_t pad;
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 7c5d46df81..08b163cbcb 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -130,6 +130,9 @@ struct domain_iommu {
>       * must not be modified after initialization.
>       */
>      struct rangeset *reserved_ranges;
> +
> +    /* Ranges under the control of iommu_op */
> +    struct rangeset *iommu_op_ranges;
>  };
> 
>  #define dom_iommu(d)              (&(d)->iommu)
> --
> 2.11.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel