[Qemu-devel] about the re-attach more than one pci devices failed

[Li, Liang Z]

Hi Paolo,

We have found a bug in all the xen-4.4 and xen-4.5-rcx, the bug
can be reproduced by the following steps:

Use the 'xl pci-attach $DomU $BDF' command to attach more then 
one PCI devices to the guest, then detach the devices with
'xl pci-detach $DomU $BDF', after that, re-attach these PCI 
devices again, an error message will be reported like following:

libxl: error: libxl_qmp.c:287:qmp_handle_error_response: receive
an error message from QMP server: Duplicate ID 'pci-pt-03_10.1'
for device.

By debugging, I found the count of calling xen_pt_region_add and 
xen_pt_region_del are not the same, and this  may cause the 
XenPCIPassthroughState and it's related QemuOpts object not be
released properly. 

I don't know how this happened, but the following patch can fix this bug.

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index be4220b..a418c53 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -607,7 +607,6 @@ static void xen_pt_region_add(MemoryListener *l, 
MemoryRegionSection *sec)
     XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
                                              memory_listener);
 
-    memory_region_ref(sec->mr);
     xen_pt_region_update(s, sec, true);
 }
 
@@ -617,7 +616,6 @@ static void xen_pt_region_del(MemoryListener *l, 
MemoryRegionSection *sec)
                                              memory_listener);
 
     xen_pt_region_update(s, sec, false);
-    memory_region_unref(sec->mr);
 }
 
 static void xen_pt_io_region_add(MemoryListener *l, MemoryRegionSection *sec)
@@ -625,7 +623,6 @@ static void xen_pt_io_region_add(MemoryListener *l, 
MemoryRegionSection *sec)
     XenPCIPassthroughState *s = container_of(l, XenPCIPassthroughState,
                                              io_listener);
 
-    memory_region_ref(sec->mr);
     xen_pt_region_update(s, sec, true);
 }
 
@@ -635,7 +632,6 @@ static void xen_pt_io_region_del(MemoryListener *l, 
MemoryRegionSection *sec)
                                              io_listener);
 
     xen_pt_region_update(s, sec, false);
-    memory_region_unref(sec->mr);
 }
 
 static const MemoryListener xen_pt_memory_listener = {


After reading other parts of the source code, I don't think the above patch is 
a good fix.
I have verified the following patch can work too:

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index c1bf357..f2893b2 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -736,7 +736,7 @@ static int xen_pt_initfn(PCIDevice *d)
     }
 
 out:
-    memory_listener_register(&s->memory_listener, &address_space_memory);
+    memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
     memory_listener_register(&s->io_listener, &address_space_io);
     XEN_PT_LOG(d,
                "Real physical device %02x:%02x.%d registered successfully!\n",

By  debugging, I found when using 'address_space_memory', 
xen_pt_region_del won't be called when the memory region is not  ' 
xen-pci-pt-*',
when using ' s->dev.bus_master_as ', there is no such issue.

I am not sure use 's->dev.bus_master_as' instead of 'address_space_memory'
is right. Could you give some suggestion?

Liang