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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.15 at 18:32, <george.dunlap@xxxxxxxxxx> 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@xxxxxxxx>
> 
> So first of all, having all these changes in the same patch almost
> certainly slowed down the review process a bit.

Okay, I'll try to split things up ...

>> --- 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@xxxxxxxxxx>

... and commit the EPT side.

>> --- 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?

I think you're right; using p2m_flags_to_type(~0) was not a good
idea (and I simply didn't spot that implication of changing the
initializer from 0 to ~0, as using 0 would have other issues).

But, with things getting quite complicated here, I wonder whether
we shouldn't consider an easier route as alternative: There's
currently no use for all the IOMMU side interaction in p2m-pt.c
(since on NPT iommu_hap_pt_share gets forced to false), and
hence rather than trying to get dead code right we could as well
rip it out. Which would then leave only the page table freeing fix
to be carried.

Jan

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