Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.10.15 at 15:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/10/15 11:25, Jan Beulich wrote:
>> @@ -645,11 +665,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>>           && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
>>          p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>>  
>> -    if ( iommu_enabled && need_iommu(p2m->domain) )
>> +    if ( iommu_enabled && need_iommu(p2m->domain) &&
>> +         (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
>>      {
>>          if ( iommu_use_hap_pt(p2m->domain) )
>>          {
>> -            if ( old_mfn && (old_mfn != mfn_x(mfn)) )
>> +            if ( iommu_old_flags )
>>                  amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> 
> iommu_hap_pt_share is hardwired to 0 on AMD, making this if() clause
> effectively dead.  Is this what you mean by your second TBD?  I would
> suggest dropping it.

Yes, that's what I mean.

>>          }
>>          else
>>
>>
> 
> In this else clause there is a now-shadowed "flags" which might better
> be renamed to iommu_flags to avoid confusion.
> 
> There is also an extra shadowed 'i' which could do with removing, as it
> introduces a 64bit->32bit truncation (which is not currently a problem).

See the (not re-submitted because it didn't really change) follow-up
patch
(http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg02585.html)
which deletes that pointless variable altogether.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel