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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 11 September 2018 15:48
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH v6 11/14] x86: add iommu_op to enable modification of
> IOMMU mappings
> 
> >>> 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?
> 

It could but I don't see any good way round it. The alternative is to shoot all 
RAM mappings out of the IOMMU when enabling PV IOMMU and determine the validity 
of an unmap op only on the basis of whether the mapping exists in the IOMMU. Is 
that option preferable?

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

True, yes. This would need locking.

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

Yes.

> 
> > --- 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)?
> 

No. If the guest has not enabled PV-IOMMu then hd->iommu_op_ranges would be 
NULL and so the tables will get torn down, otherwise the guest is in control 
and so Xen should leave the tables alone.

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

Ok.

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

IIRC I investigated this. I think the reason is that the XLAT macro won't do 
the necessary copy-back of the struct in the compat path and so it messed up 
getting the status field out. Since you wanted by to adjust the compat code to 
avoid unnecessary copying (in an earlier patch) I can probably change these to 
?.

  Paul

> 



> Jan


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