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

From: Paul Durrant <Paul.Durrant@citrix.com>
To: 'Jan Beulich' <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Andrew Cooper <Andrew.Cooper3@citrix.com>,
	"Tim (Xen.org)" <tim@xen.org>,
	George Dunlap <George.Dunlap@citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@citrix.com>
Subject: Re: [PATCH v6 11/14] x86: add iommu_op to enable modification of IOMMU mappings
Date: Wed, 12 Sep 2018 08:04:28 +0000	[thread overview]
Message-ID: <0e0d58463e254532a1661937165cf26b@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <5B98B80402000078001E7907@prv1-mh.provo.novell.com>

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 12 September 2018 07:54
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH v6 11/14] x86: add iommu_op to enable modification of
> IOMMU mappings
> 
> >>> On 11.09.18 at 17:52, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 11 September 2018 15:48
> >>
> >> >>> On 23.08.18 at 11:47, <paul.durrant@citrix.com> 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?
> 
> I'm not sure - it very much depends on the downsides. An obvious
> one is that dropping all RAM mappings is going to take quite some
> time for a big domain.
> 
> >> > --- 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.
> 
> Oh, I've misread hd->iommu_op_ranges (taking it for a check for the
> required op that you check for in iommu_op_enable_modification()).
> 
> >> > --- 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.
> 
> I'd be surprised, as I think we use such mix of ! and ? elsewhere
> too. I can't exclude that's in no-copy-back scenarios only, though.
> 
> > Since you wanted by to adjust the compat code
> > to avoid unnecessary copying (in an earlier patch) I can probably change
> > these to ?.
> 
> Perhaps I'm missing the connection, so I don't know for sure what
> to reply here - most likely the answer is "yes".
> 

It wasn't a question... perhaps I should have quoted that '?' :-) My point was that I'm going to have to re-investigate the copy-back anyway so I'll try to go for check rather than xlat where I can.

  Paul

> Jan
> 

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