| Message ID | alpine.DEB.2.00.1205161124090.26786@kaball-desktop |
|---|---|
| State | New |
| Headers | show |
On Wed, May 16, 2012 at 11:32 AM, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > On Tue, 15 May 2012, Michael S. Tsirkin wrote: >> On Tue, May 15, 2012 at 04:26:36PM +0100, Anthony PERARD wrote: >> > This hotplug state will be used to remove a device without the guest >> > cooperation. >> > >> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >> >> This can crash guest, can't it? If you are fine with crashing guest, >> we already let you do this: >> - delete device >> - reset guest >> no need for new flags. > > Given that the guest is not going to crash (if it knows what it is > doing), we could just: > > > diff --git a/hw/xen_platform.c b/hw/xen_platform.c > index a9c52a6..a1e1a33 100644 > --- a/hw/xen_platform.c > +++ b/hw/xen_platform.c > @@ -88,6 +88,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) > if (pci_get_word(d->config + PCI_CLASS_DEVICE) == > PCI_CLASS_NETWORK_ETHERNET) { > qdev_unplug(&(d->qdev), NULL); > + qdev_free(&(d->qdev)); > } > } > > > Anthony, can you confirm that this solves the problem for you? This work until I try to hotplug a new device to the guest at wish point I have this: ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: assertion failed: (obj->ref == 0) This is because there is still a pending request of the hotunplug in the acpi piix4. If I call qdev_free without qdev_unplug, I hit the same assert, but rigth away. This is way something new.
Il 16/05/2012 13:15, Anthony PERARD ha scritto: >> > qdev_unplug(&(d->qdev), NULL); >> > + qdev_free(&(d->qdev)); >> > } >> > } >> > >> > >> > Anthony, can you confirm that this solves the problem for you? > This work until I try to hotplug a new device to the guest at wish > point I have this: > ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: > assertion failed: (obj->ref == 0) > > This is because there is still a pending request of the hotunplug in > the acpi piix4. > If I call qdev_free without qdev_unplug, I hit the same assert, but > rigth away. This is way something new. Because it's missing the object_unparent done by qdev_unplug. Does object_unparent+qdev_free work? (I believe object_unparent should be done by qdev_free rather than qdev_unplug, but that's something for 1.2). Paolo
On Wed, May 16, 2012 at 12:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 16/05/2012 13:15, Anthony PERARD ha scritto: >>> > qdev_unplug(&(d->qdev), NULL); >>> > + qdev_free(&(d->qdev)); >>> > } >>> > } >>> > >>> > >>> > Anthony, can you confirm that this solves the problem for you? >> This work until I try to hotplug a new device to the guest at wish >> point I have this: >> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: >> assertion failed: (obj->ref == 0) >> >> This is because there is still a pending request of the hotunplug in >> the acpi piix4. >> If I call qdev_free without qdev_unplug, I hit the same assert, but >> rigth away. This is way something new. > > Because it's missing the object_unparent done by qdev_unplug. Does > object_unparent+qdev_free work? (I believe object_unparent should be > done by qdev_free rather than qdev_unplug, but that's something for 1.2). Cool, this seems to work fine. Thanks. I'll test a bit more and resend a patch with only object_unparent+qdev_free.
On Wed, May 16, 2012 at 12:37:54PM +0100, Anthony PERARD wrote: > On Wed, May 16, 2012 at 12:23 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 16/05/2012 13:15, Anthony PERARD ha scritto: > >>> > qdev_unplug(&(d->qdev), NULL); > >>> > + qdev_free(&(d->qdev)); > >>> > } > >>> > } > >>> > > >>> > > >>> > Anthony, can you confirm that this solves the problem for you? > >> This work until I try to hotplug a new device to the guest at wish > >> point I have this: > >> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: > >> assertion failed: (obj->ref == 0) > >> > >> This is because there is still a pending request of the hotunplug in > >> the acpi piix4. > >> If I call qdev_free without qdev_unplug, I hit the same assert, but > >> rigth away. This is way something new. > > > > Because it's missing the object_unparent done by qdev_unplug. Does > > object_unparent+qdev_free work? (I believe object_unparent should be > > done by qdev_free rather than qdev_unplug, but that's something for 1.2). > > Cool, this seems to work fine. Thanks. > > I'll test a bit more and resend a patch with only object_unparent+qdev_free. Separately it was suggested to make qdev_free do object_unparent automatically. Anthony is yet to respond. > -- > Anthony PERARD
Il 16/05/2012 15:20, Michael S. Tsirkin ha scritto: >>> Because it's missing the object_unparent done by qdev_unplug. Does >>> object_unparent+qdev_free work? (I believe object_unparent should be >>> done by qdev_free rather than qdev_unplug, but that's something for 1.2). >> >> Cool, this seems to work fine. Thanks. >> >> I'll test a bit more and resend a patch with only object_unparent+qdev_free. > > Separately it was suggested to make qdev_free do object_unparent > automatically. Anthony is yet to respond. Yes, but for 1.1 this Xen-only patch would be nice. Paolo
On 05/16/2012 06:23 AM, Paolo Bonzini wrote: > Il 16/05/2012 13:15, Anthony PERARD ha scritto: >>>> qdev_unplug(&(d->qdev), NULL); >>>> + qdev_free(&(d->qdev)); >>>> } >>>> } >>>> >>>> >>>> Anthony, can you confirm that this solves the problem for you? >> This work until I try to hotplug a new device to the guest at wish >> point I have this: >> ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: >> assertion failed: (obj->ref == 0) >> >> This is because there is still a pending request of the hotunplug in >> the acpi piix4. >> If I call qdev_free without qdev_unplug, I hit the same assert, but >> rigth away. This is way something new. > > Because it's missing the object_unparent done by qdev_unplug. Does > object_unparent+qdev_free work? (I believe object_unparent should be > done by qdev_free rather than qdev_unplug, but that's something for 1.2). qdev_free() is trivially object_delete today. What we should do is make an object_destroy() which emits a destroy event and then decrements the reference count. When ref == 0, we should emit a delete event. We could then register a slot to object_unparent in the destroy event handler, and then object_new() could register a free handler in the delete event. Then object_delete()/qdev_free() just become trivial invocations of object_unref(). But for 1.1, we definitely should just do an explicit object_unparent(). Regards, Anthony Liguori > > Paolo
On Wed, May 16, 2012 at 11:02:30AM -0500, Anthony Liguori wrote: > On 05/16/2012 06:23 AM, Paolo Bonzini wrote: > >Il 16/05/2012 13:15, Anthony PERARD ha scritto: > >>>> qdev_unplug(&(d->qdev), NULL); > >>>>+ qdev_free(&(d->qdev)); > >>>> } > >>>> } > >>>> > >>>> > >>>>Anthony, can you confirm that this solves the problem for you? > >>This work until I try to hotplug a new device to the guest at wish > >>point I have this: > >>ERROR:/local/home/anthony/work/qemu/qom/object.c:389:object_delete: > >>assertion failed: (obj->ref == 0) > >> > >>This is because there is still a pending request of the hotunplug in > >>the acpi piix4. > >>If I call qdev_free without qdev_unplug, I hit the same assert, but > >>rigth away. This is way something new. > > > >Because it's missing the object_unparent done by qdev_unplug. Does > >object_unparent+qdev_free work? (I believe object_unparent should be > >done by qdev_free rather than qdev_unplug, but that's something for 1.2). > > qdev_free() is trivially object_delete today. > > What we should do is make an object_destroy() which emits a destroy > event and then decrements the reference count. When ref == 0, we > should emit a delete event. > > We could then register a slot to object_unparent in the destroy > event handler, and then object_new() could register a free handler > in the delete event. > > Then object_delete()/qdev_free() just become trivial invocations of object_unref(). > > But for 1.1, we definitely should just do an explicit object_unparent(). > > Regards, > > Anthony Liguori > > > > >Paolo Okay. Can you fix the bug Amos reported this way too to avoid confusion? Or prefer Amos to do it?
On Wed, May 16, 2012 at 05:52:17PM +0200, Paolo Bonzini wrote: > Il 16/05/2012 15:20, Michael S. Tsirkin ha scritto: > >>> Because it's missing the object_unparent done by qdev_unplug. Does > >>> object_unparent+qdev_free work? (I believe object_unparent should be > >>> done by qdev_free rather than qdev_unplug, but that's something for 1.2). > >> > >> Cool, this seems to work fine. Thanks. > >> > >> I'll test a bit more and resend a patch with only object_unparent+qdev_free. > > > > Separately it was suggested to make qdev_free do object_unparent > > automatically. Anthony is yet to respond. > > Yes, but for 1.1 this Xen-only patch would be nice. > > Paolo No sure which this patch is meant, the force eject is not a good idea. Freeing directly from Xen is fine.
diff --git a/hw/xen_platform.c b/hw/xen_platform.c index a9c52a6..a1e1a33 100644 --- a/hw/xen_platform.c +++ b/hw/xen_platform.c @@ -88,6 +88,7 @@ static void unplug_nic(PCIBus *b, PCIDevice *d) if (pci_get_word(d->config + PCI_CLASS_DEVICE) == PCI_CLASS_NETWORK_ETHERNET) { qdev_unplug(&(d->qdev), NULL); + qdev_free(&(d->qdev)); } }