diff mbox

[Xen-devel] xen-pt: Fix PCI devices re-attach failed

Message ID F2CBF3009FA73547804AE4C663CAB28E45B6A5@shsmsx102.ccr.corp.intel.com
State New
Headers show

Commit Message

Li, Liang Z Jan. 12, 2015, 1:35 p.m. UTC 
> > > Use the 'xl pci-attach $DomU $BDF' command to attach more than 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.
> > >
> > > The count of calling xen_pt_region_add and xen_pt_region_del are not
> > > the same will cause the XenPCIPassthroughState and it's related
> > > QemuOpts object not be released properly.
> >
> > Thanks for the patch!
> >
> > From this description, I don't quite understand why the
> > memory_region_ref and memory_region_unref calls are wrong.  What do
> > you mean by "The count of calling xen_pt_region_add and
> > xen_pt_region_del are not the same"?

I means for some memory regions , only the xen_pt_region_add callback function
was called while the xen_pt_region_del was not called.

> > On unplug xen_pt_region_del does not get called?
> > Or the memory region argument is not exactly the same as the one
> > initially passed to xen_pt_region_add?
> >
> 
> agree. Liang, could you elaborate how the patch is associated with above
> explanation? :-)


I have verified the following patch can work too:


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

I think use the device related address space here is more reasonable, but I am not sure.
 Could you give some suggestion?

Liang

Comments

Paolo Bonzini Jan. 12, 2015, 2:49 p.m. UTC | #1 
On 12/01/2015 14:35, Li, Liang Z wrote:
> 
> 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 further debugging, I found when using 'address_space_memory',  'xen_pt_region_del' 
> won't be called when the memory region's name is not  ' xen-pci-pt-*', when using
>  ' s->dev.bus_master_as ', there is no such issue.
> 
> I think use the device related address space here is more reasonable, but I am not sure.
>  Could you give some suggestion?

Yes, this patch makes sense.  The listener will be called every time the
command register is written.

Paolo
Stefano Stabellini Jan. 12, 2015, 3:46 p.m. UTC | #2 
On Mon, 12 Jan 2015, Paolo Bonzini wrote:
> On 12/01/2015 14:35, Li, Liang Z wrote:
> > 
> > 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 further debugging, I found when using 'address_space_memory',  'xen_pt_region_del' 
> > won't be called when the memory region's name is not  ' xen-pci-pt-*', when using
> >  ' s->dev.bus_master_as ', there is no such issue.
> > 
> > I think use the device related address space here is more reasonable, but I am not sure.
> >  Could you give some suggestion?
> 
> Yes, this patch makes sense.  The listener will be called every time the
> command register is written.

Paolo, thanks for reviewing!

Liang, please resend with an appropriate patch description and
sign-off-by line.
diff mbox

Patch

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",