[PATCH] AMD IOMMU: don't free page table prematurely

[PATCH] AMD IOMMU: don't free page table prematurely

[Jan Beulich]

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

iommu_merge_pages() still wants to look at the next level page table,
the TLB flush necessary before freeing too happens in that function,
and if it fails no free should happen at all. Hence the freeing must
be done after that function returned successfully, not before it's
being called.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -691,8 +691,6 @@ int amd_iommu_map_page(struct domain *d,
         if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
                                      gfn, mfn, merge_level) )
             break;
-        /* Deallocate lower level page table */
-        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
 
         if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn, 
                                flags, merge_level) )
@@ -703,6 +701,9 @@ int amd_iommu_map_page(struct domain *d,
             domain_crash(d);
             return -EFAULT;
         }
+
+        /* Deallocate lower level page table */
+        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
     }
 
 out:




[-- Attachment #2: AMD-IOMMU-defer-merge-free.patch --]
[-- Type: text/plain, Size: 1219 bytes --]

AMD IOMMU: don't free page table prematurely

iommu_merge_pages() still wants to look at the next level page table,
the TLB flush necessary before freeing too happens in that function,
and if it fails no free should happen at all. Hence the freeing must
be done after that function returned successfully, not before it's
being called.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -691,8 +691,6 @@ int amd_iommu_map_page(struct domain *d,
         if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
                                      gfn, mfn, merge_level) )
             break;
-        /* Deallocate lower level page table */
-        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
 
         if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn, 
                                flags, merge_level) )
@@ -703,6 +701,9 @@ int amd_iommu_map_page(struct domain *d,
             domain_crash(d);
             return -EFAULT;
         }
+
+        /* Deallocate lower level page table */
+        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
     }
 
 out:

[-- 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] AMD IOMMU: don't free page table prematurely

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1440 bytes --]

On 26/05/2014 11:16, Jan Beulich wrote:
> iommu_merge_pages() still wants to look at the next level page table,
> the TLB flush necessary before freeing too happens in that function,
> and if it fails no free should happen at all. Hence the freeing must
> be done after that function returned successfully, not before it's
> being called.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -691,8 +691,6 @@ int amd_iommu_map_page(struct domain *d,
>          if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
>                                       gfn, mfn, merge_level) )
>              break;
> -        /* Deallocate lower level page table */
> -        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
>  
>          if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn, 
>                                 flags, merge_level) )
> @@ -703,6 +701,9 @@ int amd_iommu_map_page(struct domain *d,
>              domain_crash(d);
>              return -EFAULT;
>          }
> +
> +        /* Deallocate lower level page table */
> +        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
>      }
>  
>  out:
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2306 bytes --]

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

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

Re: [PATCH] AMD IOMMU: don't free page table prematurely

[Suravee Suthikulpanit]

On 05/26/2014 05:16 AM, Jan Beulich wrote:
> iommu_merge_pages() still wants to look at the next level page table,
> the TLB flush necessary before freeing too happens in that function,
> and if it fails no free should happen at all. Hence the freeing must
> be done after that function returned successfully, not before it's
> being called.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -691,8 +691,6 @@ int amd_iommu_map_page(struct domain *d,
>           if ( !iommu_update_pde_count(d, pt_mfn[merge_level],
>                                        gfn, mfn, merge_level) )
>               break;
> -        /* Deallocate lower level page table */
> -        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
>
>           if ( iommu_merge_pages(d, pt_mfn[merge_level], gfn,
>                                  flags, merge_level) )
> @@ -703,6 +701,9 @@ int amd_iommu_map_page(struct domain *d,
>               domain_crash(d);
>               return -EFAULT;
>           }
> +
> +        /* Deallocate lower level page table */
> +        free_amd_iommu_pgtable(mfn_to_page(pt_mfn[merge_level - 1]));
>       }
>
>   out:
>
>
>

Reviewed and tested. By the way, is this patch fix any existing known 
issues?

Suravee