Re: [Xen-devel] [PATCH v6 10/14] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.18 at 17:40, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
>> Of Jan Beulich
>> Sent: 11 September 2018 15:31
>> 
>> >>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/x86_64/mm.c
>> > +++ b/xen/arch/x86/x86_64/mm.c
>> > @@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned
>> long epfn, unsigned int pxm)
>> >      if ( ret )
>> >          goto destroy_m2p;
>> >
>> > -    if ( iommu_enabled && !iommu_passthrough &&
>> !need_iommu(hardware_domain) )
>> > +    if ( iommu_enabled && !iommu_passthrough &&
>> > +         !need_iommu_pt_sync(hardware_domain) )
>> >      {
>> >          for ( i = spfn; i < epfn; i++ )
>> >              if ( iommu_map_page(hardware_domain, _bfn(i), _mfn(i),
>> 
>> I'm confused - the condition you change looks to be inverted. Wouldn't
>> we better fix this?
> 
> I don't think it is inverted. I think this is to add new hotplugged memory 
> to the 1:1 map in the case that dom0 is not in strict mode. I could be wrong. 

Oh, I think you're right. It is just rather confusing to see an
iommu_map_page() call qualified by !need_iommu(). But that's
as confusing (to me) as the setup logic for Dom0's page tables.

>> And then I again wonder whether you've chosen the right predicate:
>> Where would the equivalent mappings come from in the opposite case?
> 
> If dom0 is in strict mode then I assume that the synchronization is handled 
> when the calls are made to add memory into the p2m (which IIRC happens even 
> for PV guests).

Right you are.

> My aim for this patch is to avoid any visible functional  change.

Sure - I didn't mean anything here (if at all) to be done in this patch
(or perhaps even series), I've merely noticed this as an apparent
oddity (which if I were right would perhaps better have been fixed
before your transformations).

>> > --- a/xen/common/memory.c
>> > +++ b/xen/common/memory.c
>> > @@ -805,8 +805,8 @@ int xenmem_add_to_physmap(struct domain *d,
>> struct xen_add_to_physmap *xatp,
>> >      xatp->size -= start;
>> >
>> >  #ifdef CONFIG_HAS_PASSTHROUGH
>> > -    if ( need_iommu(d) )
>> > -        this_cpu(iommu_dont_flush_iotlb) = 1;
>> > +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
>> > +       this_cpu(iommu_dont_flush_iotlb) = 1;
>> >  #endif
>> 
>> Rather than making the conditional more complicated, perhaps
>> simply drop it (and move the reset-to-false code out of ...
>> 
>> > @@ -828,7 +828,7 @@ int xenmem_add_to_physmap(struct domain *d,
>> struct xen_add_to_physmap *xatp,
>> >      }
>> >
>> >  #ifdef CONFIG_HAS_PASSTHROUGH
>> > -    if ( need_iommu(d) )
>> > +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
>> >      {
>> >          int ret;
>> 
>> ... this if())?
>> 
>> Also it looks to me as if here you've got confused by the meaning
>> you've assigned to need_iommu_pt_sync(): According to the
>> description, it is about sync-ing of page tables. Here, however,
>> we're checking whether to flush TLBs.
> 
> Yes, I may be confused here but it would seem to me that flushing the IOTLB 
> would be necessary even in the case where the page tables are shared. I'll 
> check the logic again.

Flushing is necessary always, and my comment didn't go in that
direction. What I was trying to point out is that the value of
iommu_dont_flush_iotlb doesn't matter when no flushing
happens anyway. I.e. setting it to true unconditionally should
not have any bad effect (but the non-strict-mode-Dom0 case
may need double checking, albeit even in that case suppressing
individual page flushing would be desirable, in which case - if
needed - the second if() might need adjustment, independent
of the change you're doing here).

Jan



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