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

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

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?

 -George