Patchwork [1/4] Introduce a new hotplug state: Force eject.

login
register
mail settings
Submitter Stefano Stabellini
Date May 16, 2012, 10:32 a.m.
Message ID <alpine.DEB.2.00.1205161124090.26786@kaball-desktop>
Download mbox | patch
Permalink /patch/159575/
State New
Headers show

Comments

Stefano Stabellini - May 16, 2012, 10:32 a.m.
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:



Anthony, can you confirm that this solves the problem for you?
Anthony PERARD - May 16, 2012, 11:15 a.m.
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.
Paolo Bonzini - May 16, 2012, 11:23 a.m.
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
Anthony PERARD - May 16, 2012, 11:37 a.m.
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.
Michael S. Tsirkin - May 16, 2012, 1:20 p.m.
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
Paolo Bonzini - May 16, 2012, 3:52 p.m.
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
Anthony Liguori - May 16, 2012, 4:02 p.m.
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
Michael S. Tsirkin - May 16, 2012, 4:24 p.m.
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?
Michael S. Tsirkin - May 16, 2012, 4:25 p.m.
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.

Patch

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));
     }
 }