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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/10/15 15:36, Jan Beulich wrote:
>>>> On 01.10.15 at 15:34, <andrew.cooper3@citrix.com> 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.

Ah - I appear to have missed that in my review queue - apologies. 
Consider it Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

As for the logic presented in this patch, I believe it is correct, so
also Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>