[Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Marcos Oviedo]

This adds a way to force the removal/unplug of previously added pci
devices when ACPI-based hotplug mechanism is not present.

Signed-off-by: Marcos Oviedo <moviedo@gmail.com>
---
 hw/pci-hotplug.c |   16 +++++++++++++---
 qemu-monitor.hx  |    4 ++--
 sysemu.h         |    2 +-
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index a8f3df1..1007e5b 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -260,11 +260,12 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict)
 }
 #endif

-int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
+int pci_device_hot_remove(Monitor *mon, const char *pci_addr, const int forced)
 {
     PCIDevice *d;
     int dom, bus;
     unsigned slot;
+    int ret = -1;

     if (pci_read_devaddr(mon, pci_addr, &dom, &bus, &slot)) {
         return -1;
@@ -275,10 +276,19 @@ int pci_device_hot_remove(Monitor *mon, const
char *pci_addr)
         monitor_printf(mon, "slot %d empty\n", slot);
         return -1;
     }
-    return qdev_unplug(&d->qdev);
+
+    if (forced) {
+        qdev_free(&d->qdev);
+        ret = 0;
+    }
+    else {
+        ret = qdev_unplug(&d->qdev);
+    }
+
+    return ret;
 }

 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict)
 {
-    pci_device_hot_remove(mon, qdict_get_str(qdict, "pci_addr"));
+    pci_device_hot_remove(mon, qdict_get_str(qdict, "pci_addr"),
qdict_get_int(qdict, "force"));
 }
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index f6a94f2..ce8cddd 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1140,8 +1140,8 @@ ETEXI
 #if defined(TARGET_I386)
     {
         .name       = "pci_del",
-        .args_type  = "pci_addr:s",
-        .params     = "[[<domain>:]<bus>:]<slot>",
+        .args_type  = "pci_addr:s,force:-f",
+        .params     = "[[<domain>:]<bus>:]<slot> [-f]",
         .help       = "hot remove PCI device",
         .mhandler.cmd = do_pci_device_hot_remove,
     },
diff --git a/sysemu.h b/sysemu.h
index 879446a..22303b3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -202,7 +202,7 @@ DriveInfo *add_init_drive(const char *opts);
 /* pci-hotplug */
 void pci_device_hot_add(Monitor *mon, const QDict *qdict);
 void drive_hot_add(Monitor *mon, const QDict *qdict);
-int pci_device_hot_remove(Monitor *mon, const char *pci_addr);
+int pci_device_hot_remove(Monitor *mon, const char *pci_addr, const
int forced);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict);

 /* serial ports */
-- 
1.5.6.5

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Markus Armbruster]

Marcos Oviedo <moviedo.maillist@gmail.com> writes:

> This adds a way to force the removal/unplug of previously added pci
> devices when ACPI-based hotplug mechanism is not present.
>
> Signed-off-by: Marcos Oviedo <moviedo@gmail.com>

If this makes sense for pci_del (I'm not passing judgement), then we
need it for device_del as well.

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Gerd Hoffmann]

On 06/09/10 07:37, Marcos Oviedo wrote:
> This adds a way to force the removal/unplug of previously added pci
> devices when ACPI-based hotplug mechanism is not present.

Point being?

If your guest can't handle pci hotplug it is pretty useless to plug in 
hardware in the first place.

If your guest supports pci hotplug it will be quite upset if you zap the 
hardware without asking via ACPI.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Marcos Oviedo]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Wed, Jun 9, 2010 at 4:38 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 06/09/10 07:37, Marcos Oviedo wrote:
>
>> This adds a way to force the removal/unplug of previously added pci
>> devices when ACPI-based hotplug mechanism is not present.
>>
>
> Point being?
>
> If your guest can't handle pci hotplug it is pretty useless to plug in
> hardware in the first place.
>
> If your guest supports pci hotplug it will be quite upset if you zap the
> hardware without asking via ACPI.
>

This make sense when you mistakenly add a pci device on a -s -S scenario,
like the scenario described on the following bug:
https://bugs.launchpad.net/qemu/+bug/544367.

When ACPI-based hotplug support is present on the guest and we run pci_del
with the force option, the hotplug events will still be generated to the
guest and the guest still will trigger the EJx event, which will end by
calling pciej_write() on qemu side. This function will do nothing on a -f
and pci hotplug support scenario, as the pci device was previously removed
by pci_del.

Thanks!

Marcos



>
> cheers,
>  Gerd
>
>

[-- Attachment #2: Type: text/html, Size: 1819 bytes --]

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Gerd Hoffmann]

   Hi,

> This make sense when you mistakenly add a pci device on a -s -S
> scenario, like the scenario described on the following bug:
> https://bugs.launchpad.net/qemu/+bug/544367.

It doesn't IMHO.

> When ACPI-based hotplug support is present on the guest and we run
> pci_del with the force option, the hotplug events will still be
> generated to the guest and the guest still will trigger the EJx event,
> which will end by calling pciej_write() on qemu side. This function will
> do nothing on a -f and pci hotplug support scenario, as the pci device
> was previously removed by pci_del.

And in case the guest wants to do anything (like flushing dirty buffers) 
before triggering the EJx event it will horribly fail.

If the guest is stopped while unplugging the device the unplug should 
happen as soon as the guest is unpaused.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Anthony Liguori]

On 06/09/2010 09:27 AM, Gerd Hoffmann wrote:
>   Hi,
>
>> This make sense when you mistakenly add a pci device on a -s -S
>> scenario, like the scenario described on the following bug:
>> https://bugs.launchpad.net/qemu/+bug/544367.
>
> It doesn't IMHO.
>
>> When ACPI-based hotplug support is present on the guest and we run
>> pci_del with the force option, the hotplug events will still be
>> generated to the guest and the guest still will trigger the EJx event,
>> which will end by calling pciej_write() on qemu side. This function will
>> do nothing on a -f and pci hotplug support scenario, as the pci device
>> was previously removed by pci_del.
>
> And in case the guest wants to do anything (like flushing dirty 
> buffers) before triggering the EJx event it will horribly fail.
>
> If the guest is stopped while unplugging the device the unplug should 
> happen as soon as the guest is unpaused.

This is a case where the fundamental problem is that the pci_del command 
should block until the guest has actually responded to the request.

pci_del returning with no error and yet not having the operation 
complete is certainly a usability issue.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Gerd Hoffmann]

   Hi,

>> If the guest is stopped while unplugging the device the unplug should
>> happen as soon as the guest is unpaused.
>
> This is a case where the fundamental problem is that the pci_del command
> should block until the guest has actually responded to the request.

You can't block.  Unplug might never ever happen for various reasons. 
IMHO the only sane way to handle it is sending a event when the unplug 
is done.

> pci_del returning with no error and yet not having the operation
> complete is certainly a usability issue.

There simply is no clear error condition.  If the guest didn't respond 
(yet) you don't know whenever it just needs a bit more time to shutdown 
the device or if unplugging the device failed.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 06/09/2010 09:27 AM, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> This make sense when you mistakenly add a pci device on a -s -S
>>> scenario, like the scenario described on the following bug:
>>> https://bugs.launchpad.net/qemu/+bug/544367.
>>
>> It doesn't IMHO.
>>
>>> When ACPI-based hotplug support is present on the guest and we run
>>> pci_del with the force option, the hotplug events will still be
>>> generated to the guest and the guest still will trigger the EJx event,
>>> which will end by calling pciej_write() on qemu side. This function will
>>> do nothing on a -f and pci hotplug support scenario, as the pci device
>>> was previously removed by pci_del.
>>
>> And in case the guest wants to do anything (like flushing dirty
>> buffers) before triggering the EJx event it will horribly fail.
>>
>> If the guest is stopped while unplugging the device the unplug
>> should happen as soon as the guest is unpaused.
>
> This is a case where the fundamental problem is that the pci_del
> command should block until the guest has actually responded to the
> request.
>
> pci_del returning with no error and yet not having the operation
> complete is certainly a usability issue.

s/pci_del/device_del/g  :)

What should device_del do?  Wait until ACPI reports that the guest has
processed the unplug event?  What if the guest doesn't?  Hanging
indefinitely is not an option.  Can we reliably detect this case?

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Luiz Capitulino]

On Tue, 15 Jun 2010 11:03:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 06/09/2010 09:27 AM, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>> This make sense when you mistakenly add a pci device on a -s -S
> >>> scenario, like the scenario described on the following bug:
> >>> https://bugs.launchpad.net/qemu/+bug/544367.
> >>
> >> It doesn't IMHO.
> >>
> >>> When ACPI-based hotplug support is present on the guest and we run
> >>> pci_del with the force option, the hotplug events will still be
> >>> generated to the guest and the guest still will trigger the EJx event,
> >>> which will end by calling pciej_write() on qemu side. This function will
> >>> do nothing on a -f and pci hotplug support scenario, as the pci device
> >>> was previously removed by pci_del.
> >>
> >> And in case the guest wants to do anything (like flushing dirty
> >> buffers) before triggering the EJx event it will horribly fail.
> >>
> >> If the guest is stopped while unplugging the device the unplug
> >> should happen as soon as the guest is unpaused.
> >
> > This is a case where the fundamental problem is that the pci_del
> > command should block until the guest has actually responded to the
> > request.
> >
> > pci_del returning with no error and yet not having the operation
> > complete is certainly a usability issue.
> 
> s/pci_del/device_del/g  :)
> 
> What should device_del do?  Wait until ACPI reports that the guest has
> processed the unplug event?  What if the guest doesn't?  Hanging
> indefinitely is not an option.  Can we reliably detect this case?

 This is a general question for all commands that can take way too long
or never return.

 For QMP the question is whether we should handle this in QEMU or in the
client. Ie, if the guest doesn't respond the client could detect that
and cancel the async command.

 For HMP we could just live with that or suspend the shell and allow the
user to cancel the operation (eg. ctrl+c) and the obvious alternative is to
have timeouts, allowing the user to set them.

Re: [Qemu-devel] [PATCH] monitor: Add force option support to pci_del command

[Anthony Liguori]

On 06/17/2010 01:15 PM, Luiz Capitulino wrote:
>   This is a general question for all commands that can take way too long
> or never return.
>
>   For QMP the question is whether we should handle this in QEMU or in the
> client. Ie, if the guest doesn't respond the client could detect that
> and cancel the async command.
>    

Exactly.  It's no different than a migration that takes too long.

>   For HMP we could just live with that or suspend the shell and allow the
> user to cancel the operation (eg. ctrl+c) and the obvious alternative is to
> have timeouts, allowing the user to set them.
>    

Yeah, ctrl+c to cancel would be a very nice feature.

Regards,

Anthony Liguori