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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 6624 bytes --]

Whether the MFN changes does not depend on the new entry being valid
(but solely on the old one), and the need to update or TLB-flush also
depends on permission changes.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split from larger patch. Fix logic determining whether to update/
    flush IOMMU mappings.
---
TBD: As already mentioned on the large-page-MMIO-mapping patch, there
     is an apparent inconsistency with PoD handling: 2M mappings get
     valid entries created, while 4k mappings don't. It would seem to
     me that the 4k case needs changing, even if today this may only
     be a latent bug. Question of course is why we don't rely on
     p2m_type_to_flags() doing its job properly and instead special
     case certain P2M types.
TBD: Rip out hap-pt-share code from the file altogether?

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -494,7 +494,18 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     l3_pgentry_t l3e_content;
     int rc;
     unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
-    unsigned long old_mfn = 0;
+    /*
+     * old_mfn and iommu_old_flags control possible flush/update needs on the
+     * IOMMU: We need to flush when MFN or flags (i.e. permissions) change.
+     * iommu_old_flags being initialized to zero covers the case of the entry
+     * getting replaced being a non-present (leaf or intermediate) one. For
+     * present leaf entries the real value will get calculated below, while
+     * for present intermediate entries ~0 (guaranteed != iommu_pte_flags)
+     * will be used (to cover all cases of what the leaf entries underneath
+     * the intermediate one might be).
+     */
+    unsigned int flags, iommu_old_flags = 0;
+    unsigned long old_mfn = INVALID_MFN;
 
     ASSERT(sve != 0);
 
@@ -543,12 +554,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
                                    L3_PAGETABLE_SHIFT - PAGE_SHIFT,
                                    L3_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
-             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
+        flags = l1e_get_flags(*p2m_entry);
+        if ( flags & _PAGE_PRESENT )
         {
-            /* We're replacing a non-SP page with a superpage.  Make sure to
-             * handle freeing the table properly. */
-            intermediate_entry = *p2m_entry;
+            if ( flags & _PAGE_PSE )
+            {
+                iommu_old_flags =
+                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
+                old_mfn = l1e_get_pfn(*p2m_entry);
+            }
+            else
+            {
+                iommu_old_flags = ~0;
+                intermediate_entry = *p2m_entry;
+            }
         }
 
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
@@ -559,10 +578,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         entry_content.l1 = l3e_content.l3;
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
 
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -587,7 +603,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
                                    0, L1_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        
+        iommu_old_flags =
+            p2m_get_iommu_flags(p2m_flags_to_type(l1e_get_flags(*p2m_entry)));
+        old_mfn = l1e_get_pfn(*p2m_entry);
+
         if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct)
                             || p2m_is_paging(p2mt) )
             entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
@@ -596,10 +615,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
             entry_content = l1e_empty();
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
+
         /* level 1 entry */
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -610,14 +627,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
                                    L2_PAGETABLE_SHIFT - PAGE_SHIFT,
                                    L2_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        
-        /* FIXME: Deal with 4k replaced by 2meg pages */
-        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
-             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
-        {
-            /* We're replacing a non-SP page with a superpage.  Make sure to
-             * handle freeing the table properly. */
-            intermediate_entry = *p2m_entry;
+        flags = l1e_get_flags(*p2m_entry);
+        if ( flags & _PAGE_PRESENT )
+        {
+            if ( flags & _PAGE_PSE )
+            {
+                iommu_old_flags =
+                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
+                old_mfn = l1e_get_pfn(*p2m_entry);
+            }
+            else
+            {
+                iommu_old_flags = ~0;
+                intermediate_entry = *p2m_entry;
+            }
         }
         
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
@@ -631,10 +654,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         entry_content.l1 = l2e_content.l2;
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
 
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -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);
         }
         else



[-- Attachment #2: x86-p2m-pt-share.patch --]
[-- Type: text/plain, Size: 6679 bytes --]

x86/p2m-pt: tighten conditions of IOMMU mapping updates

Whether the MFN changes does not depend on the new entry being valid
(but solely on the old one), and the need to update or TLB-flush also
depends on permission changes.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Split from larger patch. Fix logic determining whether to update/
    flush IOMMU mappings.
---
TBD: As already mentioned on the large-page-MMIO-mapping patch, there
     is an apparent inconsistency with PoD handling: 2M mappings get
     valid entries created, while 4k mappings don't. It would seem to
     me that the 4k case needs changing, even if today this may only
     be a latent bug. Question of course is why we don't rely on
     p2m_type_to_flags() doing its job properly and instead special
     case certain P2M types.
TBD: Rip out hap-pt-share code from the file altogether?

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -494,7 +494,18 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
     l3_pgentry_t l3e_content;
     int rc;
     unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
-    unsigned long old_mfn = 0;
+    /*
+     * old_mfn and iommu_old_flags control possible flush/update needs on the
+     * IOMMU: We need to flush when MFN or flags (i.e. permissions) change.
+     * iommu_old_flags being initialized to zero covers the case of the entry
+     * getting replaced being a non-present (leaf or intermediate) one. For
+     * present leaf entries the real value will get calculated below, while
+     * for present intermediate entries ~0 (guaranteed != iommu_pte_flags)
+     * will be used (to cover all cases of what the leaf entries underneath
+     * the intermediate one might be).
+     */
+    unsigned int flags, iommu_old_flags = 0;
+    unsigned long old_mfn = INVALID_MFN;
 
     ASSERT(sve != 0);
 
@@ -543,12 +554,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
                                    L3_PAGETABLE_SHIFT - PAGE_SHIFT,
                                    L3_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
-             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
+        flags = l1e_get_flags(*p2m_entry);
+        if ( flags & _PAGE_PRESENT )
         {
-            /* We're replacing a non-SP page with a superpage.  Make sure to
-             * handle freeing the table properly. */
-            intermediate_entry = *p2m_entry;
+            if ( flags & _PAGE_PSE )
+            {
+                iommu_old_flags =
+                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
+                old_mfn = l1e_get_pfn(*p2m_entry);
+            }
+            else
+            {
+                iommu_old_flags = ~0;
+                intermediate_entry = *p2m_entry;
+            }
         }
 
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
@@ -559,10 +578,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         entry_content.l1 = l3e_content.l3;
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
 
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -587,7 +603,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
                                    0, L1_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        
+        iommu_old_flags =
+            p2m_get_iommu_flags(p2m_flags_to_type(l1e_get_flags(*p2m_entry)));
+        old_mfn = l1e_get_pfn(*p2m_entry);
+
         if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct)
                             || p2m_is_paging(p2mt) )
             entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
@@ -596,10 +615,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
             entry_content = l1e_empty();
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
+
         /* level 1 entry */
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -610,14 +627,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
                                    L2_PAGETABLE_SHIFT - PAGE_SHIFT,
                                    L2_PAGETABLE_ENTRIES);
         ASSERT(p2m_entry);
-        
-        /* FIXME: Deal with 4k replaced by 2meg pages */
-        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
-             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
-        {
-            /* We're replacing a non-SP page with a superpage.  Make sure to
-             * handle freeing the table properly. */
-            intermediate_entry = *p2m_entry;
+        flags = l1e_get_flags(*p2m_entry);
+        if ( flags & _PAGE_PRESENT )
+        {
+            if ( flags & _PAGE_PSE )
+            {
+                iommu_old_flags =
+                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
+                old_mfn = l1e_get_pfn(*p2m_entry);
+            }
+            else
+            {
+                iommu_old_flags = ~0;
+                intermediate_entry = *p2m_entry;
+            }
         }
         
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
@@ -631,10 +654,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
         entry_content.l1 = l2e_content.l2;
 
         if ( entry_content.l1 != 0 )
-        {
             p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
-            old_mfn = l1e_get_pfn(*p2m_entry);
-        }
 
         p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
@@ -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);
         }
         else

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Andrew Cooper]

On 01/10/15 11:25, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Split from larger patch. Fix logic determining whether to update/
>     flush IOMMU mappings.
> ---
> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>      is an apparent inconsistency with PoD handling: 2M mappings get
>      valid entries created, while 4k mappings don't. It would seem to
>      me that the 4k case needs changing, even if today this may only
>      be a latent bug. Question of course is why we don't rely on
>      p2m_type_to_flags() doing its job properly and instead special
>      case certain P2M types.
> TBD: Rip out hap-pt-share code from the file altogether?
>
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -494,7 +494,18 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>      l3_pgentry_t l3e_content;
>      int rc;
>      unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> -    unsigned long old_mfn = 0;
> +    /*
> +     * old_mfn and iommu_old_flags control possible flush/update needs on the
> +     * IOMMU: We need to flush when MFN or flags (i.e. permissions) change.
> +     * iommu_old_flags being initialized to zero covers the case of the entry
> +     * getting replaced being a non-present (leaf or intermediate) one. For
> +     * present leaf entries the real value will get calculated below, while
> +     * for present intermediate entries ~0 (guaranteed != iommu_pte_flags)
> +     * will be used (to cover all cases of what the leaf entries underneath
> +     * the intermediate one might be).
> +     */
> +    unsigned int flags, iommu_old_flags = 0;
> +    unsigned long old_mfn = INVALID_MFN;
>  
>      ASSERT(sve != 0);
>  
> @@ -543,12 +554,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>                                     L3_PAGETABLE_SHIFT - PAGE_SHIFT,
>                                     L3_PAGETABLE_ENTRIES);
>          ASSERT(p2m_entry);
> -        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> -             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> +        flags = l1e_get_flags(*p2m_entry);
> +        if ( flags & _PAGE_PRESENT )
>          {
> -            /* We're replacing a non-SP page with a superpage.  Make sure to
> -             * handle freeing the table properly. */
> -            intermediate_entry = *p2m_entry;
> +            if ( flags & _PAGE_PSE )
> +            {
> +                iommu_old_flags =
> +                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
> +                old_mfn = l1e_get_pfn(*p2m_entry);
> +            }
> +            else
> +            {
> +                iommu_old_flags = ~0;
> +                intermediate_entry = *p2m_entry;
> +            }
>          }
>  
>          ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -559,10 +578,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          entry_content.l1 = l3e_content.l3;
>  
>          if ( entry_content.l1 != 0 )
> -        {
>              p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> -            old_mfn = l1e_get_pfn(*p2m_entry);
> -        }
>  
>          p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
>          /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -587,7 +603,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
>                                     0, L1_PAGETABLE_ENTRIES);
>          ASSERT(p2m_entry);
> -        
> +        iommu_old_flags =
> +            p2m_get_iommu_flags(p2m_flags_to_type(l1e_get_flags(*p2m_entry)));
> +        old_mfn = l1e_get_pfn(*p2m_entry);
> +
>          if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct)
>                              || p2m_is_paging(p2mt) )
>              entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
> @@ -596,10 +615,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>              entry_content = l1e_empty();
>  
>          if ( entry_content.l1 != 0 )
> -        {
>              p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> -            old_mfn = l1e_get_pfn(*p2m_entry);
> -        }
> +
>          /* level 1 entry */
>          p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
>          /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -610,14 +627,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>                                     L2_PAGETABLE_SHIFT - PAGE_SHIFT,
>                                     L2_PAGETABLE_ENTRIES);
>          ASSERT(p2m_entry);
> -        
> -        /* FIXME: Deal with 4k replaced by 2meg pages */
> -        if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> -             !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> -        {
> -            /* We're replacing a non-SP page with a superpage.  Make sure to
> -             * handle freeing the table properly. */
> -            intermediate_entry = *p2m_entry;
> +        flags = l1e_get_flags(*p2m_entry);
> +        if ( flags & _PAGE_PRESENT )
> +        {
> +            if ( flags & _PAGE_PSE )
> +            {
> +                iommu_old_flags =
> +                    p2m_get_iommu_flags(p2m_flags_to_type(flags));
> +                old_mfn = l1e_get_pfn(*p2m_entry);
> +            }
> +            else
> +            {
> +                iommu_old_flags = ~0;
> +                intermediate_entry = *p2m_entry;
> +            }
>          }
>          
>          ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -631,10 +654,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>          entry_content.l1 = l2e_content.l2;
>  
>          if ( entry_content.l1 != 0 )
> -        {
>              p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> -            old_mfn = l1e_get_pfn(*p2m_entry);
> -        }
>  
>          p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
>          /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -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.

>          }
>          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).

~Andrew

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

[Jan Beulich]

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

Jan

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

[Andrew Cooper]

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>

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

[Wei Liu]

On Thu, Oct 01, 2015 at 04:25:01AM -0600, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

[George Dunlap]

On 01/10/15 11:25, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

> ---
> v2: Split from larger patch. Fix logic determining whether to update/
>     flush IOMMU mappings.
> ---
> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>      is an apparent inconsistency with PoD handling: 2M mappings get
>      valid entries created, while 4k mappings don't. It would seem to
>      me that the 4k case needs changing, even if today this may only
>      be a latent bug. Question of course is why we don't rely on
>      p2m_type_to_flags() doing its job properly and instead special
>      case certain P2M types.

The inconsistency in the conditionals there is a bit strange; but I'm
pretty sure that in the 2MB case it is (at the moment) superfluous,
because at the moment it seems that when setting a page with type
p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.

(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
some point without comment.)

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

[Jan Beulich]

>>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
>On 01/10/15 11:25, Jan Beulich wrote:
>> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>>      is an apparent inconsistency with PoD handling: 2M mappings get
>>      valid entries created, while 4k mappings don't. It would seem to
>>      me that the 4k case needs changing, even if today this may only
>>      be a latent bug. Question of course is why we don't rely on
>>      p2m_type_to_flags() doing its job properly and instead special
>>      case certain P2M types.
>
>The inconsistency in the conditionals there is a bit strange; but I'm
>pretty sure that in the 2MB case it is (at the moment) superfluous,
>because at the moment it seems that when setting a page with type
>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>
>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>some point without comment.)

Perhaps just because the magic MFN didn't always work? Tim?
To me it looks wrong to pass anything other than INVALID_MFN
there.

Jan

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

[Wei Liu]

On Fri, Oct 02, 2015 at 01:31:37AM -0600, Jan Beulich wrote:
> >>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
> >On 01/10/15 11:25, Jan Beulich wrote:
> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
> >>      is an apparent inconsistency with PoD handling: 2M mappings get
> >>      valid entries created, while 4k mappings don't. It would seem to
> >>      me that the 4k case needs changing, even if today this may only
> >>      be a latent bug. Question of course is why we don't rely on
> >>      p2m_type_to_flags() doing its job properly and instead special
> >>      case certain P2M types.
> >
> >The inconsistency in the conditionals there is a bit strange; but I'm
> >pretty sure that in the 2MB case it is (at the moment) superfluous,
> >because at the moment it seems that when setting a page with type
> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
> >
> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
> >some point without comment.)
> 
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.
> 

I think George and you are talking about another function?  Is there
anything that prevents this patch from being committed as-is?

Wei.

> Jan
> 
> 

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

[Jan Beulich]

>>> Wei Liu <wei.liu2@citrix.com> 10/02/15 11:16 AM >>>
>I think George and you are talking about another function?  Is there
>anything that prevents this patch from being committed as-is?

Nothing technical. Just me getting into the office (early afternoon I guess).

Jan

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

[George Dunlap]

On Fri, Oct 2, 2015 at 10:16 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Fri, Oct 02, 2015 at 01:31:37AM -0600, Jan Beulich wrote:
>> >>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
>> >On 01/10/15 11:25, Jan Beulich wrote:
>> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>> >>      is an apparent inconsistency with PoD handling: 2M mappings get
>> >>      valid entries created, while 4k mappings don't. It would seem to
>> >>      me that the 4k case needs changing, even if today this may only
>> >>      be a latent bug. Question of course is why we don't rely on
>> >>      p2m_type_to_flags() doing its job properly and instead special
>> >>      case certain P2M types.
>> >
>> >The inconsistency in the conditionals there is a bit strange; but I'm
>> >pretty sure that in the 2MB case it is (at the moment) superfluous,
>> >because at the moment it seems that when setting a page with type
>> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>> >
>> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>> >some point without comment.)
>>
>> Perhaps just because the magic MFN didn't always work? Tim?
>> To me it looks wrong to pass anything other than INVALID_MFN
>> there.
>>
>
> I think George and you are talking about another function?  Is there
> anything that prevents this patch from being committed as-is?

No -- I'm just answering Jan's "To Be Done" comment (I assume that's
what TBD means).  He's noticed something strange; but it's been there
for quite a while, and so both by inspection and long testing
(probably a few releases now) works.  No point fiddling with it until
after the release.

 -George

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

[George Dunlap]

On Fri, Oct 2, 2015 at 8:31 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
>>On 01/10/15 11:25, Jan Beulich wrote:
>>> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>>>      is an apparent inconsistency with PoD handling: 2M mappings get
>>>      valid entries created, while 4k mappings don't. It would seem to
>>>      me that the 4k case needs changing, even if today this may only
>>>      be a latent bug. Question of course is why we don't rely on
>>>      p2m_type_to_flags() doing its job properly and instead special
>>>      case certain P2M types.
>>
>>The inconsistency in the conditionals there is a bit strange; but I'm
>>pretty sure that in the 2MB case it is (at the moment) superfluous,
>>because at the moment it seems that when setting a page with type
>>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>>
>>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>>some point without comment.)
>
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.

Well the magic MFN was also a valid MFN (it was like 1<<9 or
something), which is probably worse than _mfn(0). :-)

If we change it to INVALID_MFN, we'd have to either add exceptions in
all of those conditionals (so it doesn't get l1e_empty()), or perhaps
(as you say) trust p2m_type_to_flags() to DTRT.  The second is
probably the best option, but obviously not at this stage in the
release.

 -George

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

[Jan Beulich]

>>> On 02.10.15 at 11:47, <George.Dunlap@eu.citrix.com> wrote:
> On Fri, Oct 2, 2015 at 8:31 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
>>>On 01/10/15 11:25, Jan Beulich wrote:
>>>> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>>>>      is an apparent inconsistency with PoD handling: 2M mappings get
>>>>      valid entries created, while 4k mappings don't. It would seem to
>>>>      me that the 4k case needs changing, even if today this may only
>>>>      be a latent bug. Question of course is why we don't rely on
>>>>      p2m_type_to_flags() doing its job properly and instead special
>>>>      case certain P2M types.
>>>
>>>The inconsistency in the conditionals there is a bit strange; but I'm
>>>pretty sure that in the 2MB case it is (at the moment) superfluous,
>>>because at the moment it seems that when setting a page with type
>>>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>>>
>>>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>>>some point without comment.)
>>
>> Perhaps just because the magic MFN didn't always work? Tim?
>> To me it looks wrong to pass anything other than INVALID_MFN
>> there.
> 
> Well the magic MFN was also a valid MFN (it was like 1<<9 or
> something), which is probably worse than _mfn(0). :-)
> 
> If we change it to INVALID_MFN, we'd have to either add exceptions in
> all of those conditionals (so it doesn't get l1e_empty()), or perhaps
> (as you say) trust p2m_type_to_flags() to DTRT.  The second is
> probably the best option, but obviously not at this stage in the
> release.

Oh, of course I meant such a change to go into unstable only. Not
the least because surely there is a risk for regressions there.

Jan

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

[Tim Deegan]

At 01:31 -0600 on 02 Oct (1443749497), Jan Beulich wrote:
> >>> George Dunlap <george.dunlap@citrix.com> 10/01/15 6:16 PM >>>
> >On 01/10/15 11:25, Jan Beulich wrote:
> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
> >>      is an apparent inconsistency with PoD handling: 2M mappings get
> >>      valid entries created, while 4k mappings don't. It would seem to
> >>      me that the 4k case needs changing, even if today this may only
> >>      be a latent bug. Question of course is why we don't rely on
> >>      p2m_type_to_flags() doing its job properly and instead special
> >>      case certain P2M types.
> >
> >The inconsistency in the conditionals there is a bit strange; but I'm
> >pretty sure that in the 2MB case it is (at the moment) superfluous,
> >because at the moment it seems that when setting a page with type
> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
> >
> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
> >some point without comment.)
> 
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.

Hmm, I really didn't explain that at all, did I? :(  ISTR some more
discussion about this, but it doesn't seem to have been on the
list.

I suspect I wanted to change it to INVALID_MFN but got tangled up in
code that treated that as an error without checking the p2m type --
the p2m type stuff was not in use everywhere at that point.

I'd agree that using P2M_INVALID and fixing any confused callers is
the right thing to do.

Cheers,

Tim.