>>> On 11.09.18 at 17:40, <[email protected]> wrote:
>> From: Xen-devel [mailto:[email protected]] On Behalf
>> Of Jan Beulich
>> Sent: 11 September 2018 15:31
>>
>> >>> On 23.08.18 at 11:47, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
