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: 12 September 2018 07:54
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> <wei.liu2@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 11.09.18 at 17:52, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 11 September 2018 15:48
> >>
> >> >>> 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?
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel