Xen devel - Mar 2012 - [PATCH] qemu-traditional/passthrough: adjust MSI-X device cleanup (bug 1809)

To address http://bugzilla.xen.org/bugzilla/show_bug.cgi?id=1809,
pt_unregister_regions() also needs to use the newly introduced
_pt_iomem_helper() instead of calling xc_domain_memory_mapping()
directly, to take into consideration the hole created for the MSI-X
table.

For this to work, two calls in unregister_real_device() need to be
swapped, since otherwise we''d have

unregister_real_device()
  -> pt_config_delete()
    -> pt_msix_delete() (frees [and fails to clear] ->msix)
  -> pt_unregister_regions()
    -> _pt_iomem_helper() (with the patch below)
      -> has_msix_mapping() (uses ->msix)

And to be certain to prevent (catch) further/future use-after-free
instances, let''s also clear dev->msix in pt_msix_delete().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Yongjie Ren <yongjie.ren@intel.com>

--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -1969,11 +1969,9 @@ static void pt_unregister_regions(struct
         if ( type == PCI_ADDRESS_SPACE_MEM ||
              type == PCI_ADDRESS_SPACE_MEM_PREFETCH )
         {
-            ret = xc_domain_memory_mapping(xc_handle, domid,
-                    assigned_device->bases[i].e_physbase >>
XC_PAGE_SHIFT,
-                    assigned_device->bases[i].access.maddr >>
XC_PAGE_SHIFT,
-                    (e_size+XC_PAGE_SIZE-1) >> XC_PAGE_SHIFT,
-                    DPCI_REMOVE_MAPPING);
+            ret = _pt_iomem_helper(assigned_device, i,
+                                   assigned_device->bases[i].e_physbase,
+                                   e_size, DPCI_REMOVE_MAPPING);
             if ( ret != 0 )
             {
                 PT_LOG("Error: remove old mem mapping failed!\n");
@@ -4393,12 +4391,12 @@ static int unregister_real_device(int de
         }
     }
 
-    /* delete all emulated config registers */
-    pt_config_delete(assigned_device);
-
     /* unregister real device''s MMIO/PIO BARs */
     pt_unregister_regions(assigned_device);
 
+    /* delete all emulated config registers */
+    pt_config_delete(assigned_device);
+
     pt_iomul_free(assigned_device);
 
     /* mark this devfn as free */
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -627,4 +627,5 @@ void pt_msix_delete(struct pt_dev *dev)
 
 
     free(dev->msix);
+    dev->msix = NULL;
 }





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

On Mon, 26 Mar 2012, Jan Beulich wrote:
> To address http://bugzilla.xen.org/bugzilla/show_bug.cgi?id=1809,
> pt_unregister_regions() also needs to use the newly introduced
> _pt_iomem_helper() instead of calling xc_domain_memory_mapping()
> directly, to take into consideration the hole created for the MSI-X
> table.
> 
> For this to work, two calls in unregister_real_device() need to be
> swapped, since otherwise we''d have
> 
> unregister_real_device()
>   -> pt_config_delete()
>     -> pt_msix_delete() (frees [and fails to clear] ->msix)
>   -> pt_unregister_regions()
>     -> _pt_iomem_helper() (with the patch below)
>       -> has_msix_mapping() (uses ->msix)
> 
> And to be certain to prevent (catch) further/future use-after-free
> instances, let''s also clear dev->msix in pt_msix_delete().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Yongjie Ren <yongjie.ren@intel.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>>> On 26.03.12 at 12:24, Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
wrote:
> On Mon, 26 Mar 2012, Jan Beulich wrote:
>> To address http://bugzilla.xen.org/bugzilla/show_bug.cgi?id=1809,
>> pt_unregister_regions() also needs to use the newly introduced
>> _pt_iomem_helper() instead of calling xc_domain_memory_mapping()
>> directly, to take into consideration the hole created for the MSI-X
>> table.
>> 
>> For this to work, two calls in unregister_real_device() need to be
>> swapped, since otherwise we''d have
>> 
>> unregister_real_device()
>>   -> pt_config_delete()
>>     -> pt_msix_delete() (frees [and fails to clear] ->msix)
>>   -> pt_unregister_regions()
>>     -> _pt_iomem_helper() (with the patch below)
>>       -> has_msix_mapping() (uses ->msix)
>> 
>> And to be certain to prevent (catch) further/future use-after-free
>> instances, let''s also clear dev->msix in pt_msix_delete().
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Tested-by: Yongjie Ren <yongjie.ren@intel.com>
> 
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Ping (http://lists.xen.org/archives/html/xen-devel/2012-03/msg02163.html)?

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH]
qemu-traditional/passthrough: adjust MSI-X device cleanup (bug
1809)"):
> On Mon, 26 Mar 2012, Jan Beulich wrote:
> > To address http://bugzilla.xen.org/bugzilla/show_bug.cgi?id=1809,
> > pt_unregister_regions() also needs to use the newly introduced
> > _pt_iomem_helper() instead of calling xc_domain_memory_mapping()
> > directly, to take into consideration the hole created for the MSI-X
> > table.
...
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > Tested-by: Yongjie Ren <yongjie.ren@intel.com>
> 
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

I will update the QEMU_TAG in xen-unstable soon.

Ian.