Re: [PATCH 1/3] x86/p2m: tighten conditions of IOMMU mapping updates

[George Dunlap <george.dunlap@citrix.com>]

On 28/09/15 17:32, George Dunlap wrote:
> On 21/09/15 15:02, Jan Beulich wrote:
>> In the EPT case permission changes should also result in updates or
>> TLB flushes.
>>
>> In the NPT case the old MFN does not depend on the new entry being
>> valid (but solely on the old one), and the need to update or TLB-flush
>> again also depends on permission changes.
>>
>> In the shadow mode case, iommu_hap_pt_share should be ignored.
>>
>> Furthermore in the NPT/shadow case old intermediate page tables must
>> be freed only after IOMMU side updates/flushes have got carried out.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> So first of all, having all these changes in the same patch almost
> certainly slowed down the review process a bit.
> 
>> ---
>> In addition to the fixes here it looks to me as if both EPT and
>> NPT/shadow code lack invalidation of IOMMU side paging structure
>> caches, i.e. further changes may be needed. Am I overlooking something?
>>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -668,6 +668,7 @@ ept_set_entry(struct p2m_domain *p2m, un
>>      uint8_t ipat = 0;
>>      int need_modify_vtd_table = 1;
>>      int vtd_pte_present = 0;
>> +    unsigned int iommu_flags = p2m_get_iommu_flags(p2mt);
>>      enum { sync_off, sync_on, sync_check } needs_sync = sync_check;
>>      ept_entry_t old_entry = { .epte = 0 };
>>      ept_entry_t new_entry = { .epte = 0 };
>> @@ -798,8 +799,9 @@ ept_set_entry(struct p2m_domain *p2m, un
>>          new_entry.mfn = mfn_x(mfn);
>>  
>>          /* Safe to read-then-write because we hold the p2m lock */
>> -        if ( ept_entry->mfn == new_entry.mfn )
>> -             need_modify_vtd_table = 0;
>> +        if ( ept_entry->mfn == new_entry.mfn &&
>> +             p2m_get_iommu_flags(ept_entry->sa_p2mt) == iommu_flags )
>> +            need_modify_vtd_table = 0;
>>  
>>          ept_p2m_type_to_flags(p2m, &new_entry, p2mt, p2ma);
>>      }
>> @@ -830,11 +832,9 @@ out:
>>              iommu_pte_flush(d, gfn, &ept_entry->epte, order, vtd_pte_present);
>>          else
>>          {
>> -            unsigned int flags = p2m_get_iommu_flags(p2mt);
>> -
>> -            if ( flags != 0 )
>> +            if ( iommu_flags )
>>                  for ( i = 0; i < (1 << order); i++ )
>> -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i, flags);
>> +                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
>>              else
>>                  for ( i = 0; i < (1 << order); i++ )
>>                      iommu_unmap_page(d, gfn + i);
> 
> EPT changes:
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> 
> And now...
> 
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -488,12 +488,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>>      void *table;
>>      unsigned long i, gfn_remainder = gfn;
>>      l1_pgentry_t *p2m_entry;
>> -    l1_pgentry_t entry_content;
>> +    l1_pgentry_t entry_content, old_entry = l1e_empty();
>>      l2_pgentry_t l2e_content;
>>      l3_pgentry_t l3e_content;
>>      int rc;
>>      unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
>> -    unsigned long old_mfn = 0;
>> +    unsigned int old_flags = 0;
>> +    unsigned long old_mfn = INVALID_MFN;
> 
> I think this could use at least a comment, and perhaps some better
> variable naming here explaining what setting or not setting each of
> these different variables means.
> 
> Trying to sort out what the effect of setting old_flags to ~0 is
> particularly convoluted.
> 
> Inferring from the code:
> 
> 1) Setting old_entry() to some value will cause those values to be
> unmapped.  This will only be set for 1G and 2M entries, if the existing
> entry is both present and not set as a superpage.  This is pretty
> straightforward and looks correct.
> 
> 2) old_mfn and old_flags are primarily used to control whether an IOMMU
> flush happens.
> 
> 3) old_mfn
>  - old_mfn begins at INVALID_MFN, and is set only if the entry itself is
> leaf entry.
>  - a flush will happen if old_mfn != mfn
>  - The effect of this will be to force a flush if replacing a leaf with
> a leaf but a different mfn, or if replacing an intermediate table with a
> leaf *if* the leaf is not INVALID_MFN
> 
> 4) old_flags
>  - old_flags will be the actual flags if replacing a leaf with a leaf,
> or ~0 if replacing an intermediate table with a leaf.
>  - a flush will happen if
> p2m_iommu_get_flags(p2m_flags_to_type(old_flags)) != iommu_pte_flags.
>  - p2m_flags_to_type
>    - returns p2m_invalid if flags == 0.  This should never happen here
>    - returns the type bits from the flags otherwires.
>    - So in the case of old_flags being the actual flags, you get the
> actual type of the old entry
>    - in the case of old_flags being ~0, you get 0x7f, which is undefined
>  - p2m_iommu_get_flags() will return 0 for unknown types.
>  - so the effect of the above will be to force a flush if replacing a
> leaf with a leaf of different iommu permisions; or, to force a flush if
> replacing an intermediate table with any permissions with a leaf that
> has at least some iommu permissions.
> 
> Did I get the logic there right?
> 
> It *looks* to me, therefore, like this won't handle flushes properly in
> the case of replacing an intermediate table with an empty leaf entry:
> - old_mfn will be INVALID_MFN, matching mfn
> - the result of the function call around old_flags will be 0, matching
> iommu_pte_flags for the new entry (0)
> - so no flush will happen
> - but because you're removing permissions, a flush actually should happen.
> 
> Did I miss something?

Regardless whether I did or not, this code is very difficult to reason
about, even after you've figured out what old_mfn and old_flags are
meant to be used for.  It seems like maintaining a boolean like
"needs_flush" would be clearer and easier to reason about.

I might rename "old_entry" to "intermediate_table" as well, with a
comment saying "/* Intermediate table to free if we're replacing it with
a superpage */" or something like that.

 -George