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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@xxxxxxxxx]
> Sent: 07 August 2018 05:08
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: RE: [Xen-devel] [PATCH v5 12/15] x86: add iommu_op to enable
> modification of IOMMU mappings
> 
> > 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.
> 

Indeed.

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

No that's not the point here. I'm not trying to re-architect Xen's IOMMU 
handling. All the IOMMU code in Xen AFAICT is built around the assumption there 
is one set of page tables per-VM and all devices assigned to the VM get the 
same page tables. I suspect trying to change that will be a huge can of worms 
and I have no need to go there for my purposes.

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

It does make that assumption because that assumption is baked into Xen's IOMMU 
support.

> Did I misunderstand anything?

Only perhaps that moving away from per-VM IOMMU pagetables will be something 
that is something I could do without making very invasive and lengthy changes 
to Xen's IOMMU code.

  Paul

> 
> >
> > NOTE: The actual map and unmap operations are introduced by
> > subsequent
> >       patches.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > Cc: Julien Grall <julien.grall@xxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel