Xen devel - May 2012 - [PATCH 1/4] Introduce a new hotplug state: Force eject.

This hotplug state will be used to remove a device without the guest
cooperation.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 hw/acpi_piix4.c |    5 +++++
 hw/pci.h        |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 585da4e..dfd5a9d 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev,
PCIDevice *dev,
         return 0;
     }
 
+    if (state == PCI_FORCE_EJECT) {
+        acpi_piix_eject_slot(s, 1 << slot);
+        return 0;
+    }
+
     if (state == PCI_HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
diff --git a/hw/pci.h b/hw/pci.h
index 8d0aa49..3b61e43 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -273,6 +273,7 @@ typedef enum {
     PCI_HOTPLUG_DISABLED,
     PCI_HOTPLUG_ENABLED,
     PCI_COLDPLUG_ENABLED,
+    PCI_FORCE_EJECT,
 } PCIHotplugState;
 
 typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
-- 
Anthony PERARD

On Tue, 15 May 2012, 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>
> ---
>  hw/acpi_piix4.c |    5 +++++
>  hw/pci.h        |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..dfd5a9d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev,
PCIDevice *dev,
>          return 0;
>      }
>  
> +    if (state == PCI_FORCE_EJECT) {
> +        acpi_piix_eject_slot(s, 1 << slot);
> +        return 0;
> +    }
> +
>      if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3b61e43 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -273,6 +273,7 @@ typedef enum {
>      PCI_HOTPLUG_DISABLED,
>      PCI_HOTPLUG_ENABLED,
>      PCI_COLDPLUG_ENABLED,
> +    PCI_FORCE_EJECT,
>  } PCIHotplugState;
Given that it is supposed to be a state rather than an action, it should
be called PCI_FORCE_DISABLED or something like that.

On Tue, May 15, 2012 at 7:19 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
>
> Given that it is supposed to be a state rather than an action, it should
> be called PCI_FORCE_DISABLED or something like that.
Ok, I will change to that.

Thanks,

-- 
Anthony PERARD

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.
> ---
>  hw/acpi_piix4.c |    5 +++++
>  hw/pci.h        |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 585da4e..dfd5a9d 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -587,6 +587,11 @@ static int piix4_device_hotplug(DeviceState *qdev,
PCIDevice *dev,
>          return 0;
>      }
>  
> +    if (state == PCI_FORCE_EJECT) {
> +        acpi_piix_eject_slot(s, 1 << slot);
> +        return 0;
> +    }
> +
>      if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3b61e43 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -273,6 +273,7 @@ typedef enum {
>      PCI_HOTPLUG_DISABLED,
>      PCI_HOTPLUG_ENABLED,
>      PCI_COLDPLUG_ENABLED,
> +    PCI_FORCE_EJECT,
>  } PCIHotplugState;
>  
>  typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> -- 
> Anthony PERARD

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?

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.

-- 
Anthony PERARD

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

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.

-- 
Anthony PERARD

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

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?

-- 
MST

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.

-- 
MST