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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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