[Qemu-devel] Cannot not unplug cold-plugged devices

[Qemu-devel] Cannot not unplug cold-plugged devices

[Cam Macdonell]

Hi,

I was trying to do a "device_del" on my ivshmem device and it won't
work unless the device is added via hotplug.  If the device is
coldplugged (added at startup) then nothing happens.  I think I
tracked this behaviour to the patch below.

Is not allowing coldplugged devices to be unplugged the desired behaviour?

Thanks,
Cam

commit 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
Author: Isaku Yamahata <yamahata@valinux.co.jp>
Date:   Mon Sep 6 16:46:18 2010 +0900

    pci: call hotplug callback even when not hotplug case for later use.

    call hotplug callback even when not hotplug case for later use.
    And move hotplug check into hotplug callback.
    PCIE slot needs this for card presence detection.

    Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index bfa1d9a..24dfcf2 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -611,6 +611,9 @@ static int piix4_device_hotplug(DeviceState *qdev,
PCIDevice *dev, int state)
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
                                 DO_UPCAST(PCIDevice, qdev, qdev));

+    if (!dev->qdev.hotplugged)
+        return 0;
+
     s->pci0_status.up = 0;
     s->pci0_status.down = 0;
     if (state) {

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Isaku Yamahata]

On Thu, Nov 11, 2010 at 11:29:39PM -0700, Cam Macdonell wrote:
> Hi,
> 
> I was trying to do a "device_del" on my ivshmem device and it won't
> work unless the device is added via hotplug.  If the device is
> coldplugged (added at startup) then nothing happens.  I think I
> tracked this behaviour to the patch below.
> 
> Is not allowing coldplugged devices to be unplugged the desired behaviour?

Oh, my bad. Does the following patch help?

>From 45914b2fc95750b65685dfb98a435f58e38b45ba Mon Sep 17 00:00:00 2001
Message-Id: <45914b2fc95750b65685dfb98a435f58e38b45ba.1289546596.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Fri, 12 Nov 2010 16:21:35 +0900
Subject: [PATCH] acpi/piix4: allow pci hotplug for cold plugged device.

This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
reported by Cam Macdonell <cam@cs.ualberta.ca>.
Before the change set, cold plugged device can be hot unplugged.
This patch unbreaks it.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/acpi_piix4.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 66c7885..bca330e 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -621,8 +621,10 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
                                 DO_UPCAST(PCIDevice, qdev, qdev));
 
-    if (!dev->qdev.hotplugged)
+    /* qemu initialization case */
+    if (state && !dev->qdev.hotplugged) {
         return 0;
+    }
 
     s->pci0_status.up = 0;
     s->pci0_status.down = 0;
-- 
1.7.1.1



-- 
yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Michael S. Tsirkin]

On Fri, Nov 12, 2010 at 04:24:11PM +0900, Isaku Yamahata wrote:
> On Thu, Nov 11, 2010 at 11:29:39PM -0700, Cam Macdonell wrote:
> > Hi,
> > 
> > I was trying to do a "device_del" on my ivshmem device and it won't
> > work unless the device is added via hotplug.  If the device is
> > coldplugged (added at startup) then nothing happens.  I think I
> > tracked this behaviour to the patch below.
> > 
> > Is not allowing coldplugged devices to be unplugged the desired behaviour?
> 
> Oh, my bad. Does the following patch help?
> 
> >From 45914b2fc95750b65685dfb98a435f58e38b45ba Mon Sep 17 00:00:00 2001
> Message-Id: <45914b2fc95750b65685dfb98a435f58e38b45ba.1289546596.git.yamahata@valinux.co.jp>
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> Date: Fri, 12 Nov 2010 16:21:35 +0900
> Subject: [PATCH] acpi/piix4: allow pci hotplug for cold plugged device.
> 
> This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> reported by Cam Macdonell <cam@cs.ualberta.ca>.
> Before the change set, cold plugged device can be hot unplugged.
> This patch unbreaks it.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>



> ---
>  hw/acpi_piix4.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 66c7885..bca330e 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -621,8 +621,10 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
>      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
>                                  DO_UPCAST(PCIDevice, qdev, qdev));
>  
> -    if (!dev->qdev.hotplugged)
> +    /* qemu initialization case */

That comment is pretty obscure :)


> +    if (state && !dev->qdev.hotplugged) {
>          return 0;
> +    }

Do we even need this check at all?

>  
>      s->pci0_status.up = 0;
>      s->pci0_status.down = 0;
> -- 
> 1.7.1.1
> 
> 
> 
> -- 
> yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Isaku Yamahata]

On Fri, Nov 12, 2010 at 11:18:57AM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 12, 2010 at 04:24:11PM +0900, Isaku Yamahata wrote:
> > On Thu, Nov 11, 2010 at 11:29:39PM -0700, Cam Macdonell wrote:
> > > Hi,
> > > 
> > > I was trying to do a "device_del" on my ivshmem device and it won't
> > > work unless the device is added via hotplug.  If the device is
> > > coldplugged (added at startup) then nothing happens.  I think I
> > > tracked this behaviour to the patch below.
> > > 
> > > Is not allowing coldplugged devices to be unplugged the desired behaviour?
> > 
> > Oh, my bad. Does the following patch help?
> > 
> > >From 45914b2fc95750b65685dfb98a435f58e38b45ba Mon Sep 17 00:00:00 2001
> > Message-Id: <45914b2fc95750b65685dfb98a435f58e38b45ba.1289546596.git.yamahata@valinux.co.jp>
> > From: Isaku Yamahata <yamahata@valinux.co.jp>
> > Date: Fri, 12 Nov 2010 16:21:35 +0900
> > Subject: [PATCH] acpi/piix4: allow pci hotplug for cold plugged device.
> > 
> > This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> > reported by Cam Macdonell <cam@cs.ualberta.ca>.
> > Before the change set, cold plugged device can be hot unplugged.
> > This patch unbreaks it.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> 
> 
> > ---
> >  hw/acpi_piix4.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > index 66c7885..bca330e 100644
> > --- a/hw/acpi_piix4.c
> > +++ b/hw/acpi_piix4.c
> > @@ -621,8 +621,10 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
> >      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
> >                                  DO_UPCAST(PCIDevice, qdev, qdev));
> >  
> > -    if (!dev->qdev.hotplugged)
> > +    /* qemu initialization case */
> 
> That comment is pretty obscure :)

How about the following?

Reached here during qemu creating a machine before
VM runs. hot plug event shouldn't be triggered
because it's dangerous.

> 
> 
> > +    if (state && !dev->qdev.hotplugged) {
> >          return 0;
> > +    }
> 
> Do we even need this check at all?

Hmm, I suppose you don't like the condition more complex.
How about "if (qdev_hotplug)".
An access function to qdev_hotplug in qdev.c would be desirable.


> 
> >  
> >      s->pci0_status.up = 0;
> >      s->pci0_status.down = 0;
> > -- 
> > 1.7.1.1
> > 
> > 
> > 
> > -- 
> > yamahata
> 

-- 
yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Michael S. Tsirkin]

On Fri, Nov 12, 2010 at 06:59:52PM +0900, Isaku Yamahata wrote:
> On Fri, Nov 12, 2010 at 11:18:57AM +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 12, 2010 at 04:24:11PM +0900, Isaku Yamahata wrote:
> > > On Thu, Nov 11, 2010 at 11:29:39PM -0700, Cam Macdonell wrote:
> > > > Hi,
> > > > 
> > > > I was trying to do a "device_del" on my ivshmem device and it won't
> > > > work unless the device is added via hotplug.  If the device is
> > > > coldplugged (added at startup) then nothing happens.  I think I
> > > > tracked this behaviour to the patch below.
> > > > 
> > > > Is not allowing coldplugged devices to be unplugged the desired behaviour?
> > > 
> > > Oh, my bad. Does the following patch help?
> > > 
> > > >From 45914b2fc95750b65685dfb98a435f58e38b45ba Mon Sep 17 00:00:00 2001
> > > Message-Id: <45914b2fc95750b65685dfb98a435f58e38b45ba.1289546596.git.yamahata@valinux.co.jp>
> > > From: Isaku Yamahata <yamahata@valinux.co.jp>
> > > Date: Fri, 12 Nov 2010 16:21:35 +0900
> > > Subject: [PATCH] acpi/piix4: allow pci hotplug for cold plugged device.
> > > 
> > > This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> > > reported by Cam Macdonell <cam@cs.ualberta.ca>.
> > > Before the change set, cold plugged device can be hot unplugged.
> > > This patch unbreaks it.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > 
> > 
> > > ---
> > >  hw/acpi_piix4.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> > > index 66c7885..bca330e 100644
> > > --- a/hw/acpi_piix4.c
> > > +++ b/hw/acpi_piix4.c
> > > @@ -621,8 +621,10 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
> > >      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
> > >                                  DO_UPCAST(PCIDevice, qdev, qdev));
> > >  
> > > -    if (!dev->qdev.hotplugged)
> > > +    /* qemu initialization case */
> > 
> > That comment is pretty obscure :)
> 
> How about the following?
> 
> Reached here during qemu creating a machine before
> VM runs. hot plug event shouldn't be triggered
> because it's dangerous.
> 
> > 
> > 
> > > +    if (state && !dev->qdev.hotplugged) {
> > >          return 0;
> > > +    }
> > 
> > Do we even need this check at all?
> 
> Hmm, I suppose you don't like the condition more complex.
> How about "if (qdev_hotplug)".
> An access function to qdev_hotplug in qdev.c would be desirable.


No, I am just trying to understand why is hotplug event dangerous.
We still get it if we do device add before starting the VM, right?

> 
> > 
> > >  
> > >      s->pci0_status.up = 0;
> > >      s->pci0_status.down = 0;
> > > -- 
> > > 1.7.1.1
> > > 
> > > 
> > > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Isaku Yamahata]

On Fri, Nov 12, 2010 at 01:26:30PM +0200, Michael S. Tsirkin wrote:
> No, I am just trying to understand why is hotplug event dangerous.
> We still get it if we do device add before starting the VM, right?

I'm not sure if it's safe to call enable/disable_device() and
pm_update_sci() before starting VM.
So I'd like to avoid to call them instead of making sure it.
If someone else ensures its safety, I'm willing to eliminate the if clause.
-- 
yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Michael S. Tsirkin]

On Fri, Nov 12, 2010 at 09:50:30PM +0900, Isaku Yamahata wrote:
> On Fri, Nov 12, 2010 at 01:26:30PM +0200, Michael S. Tsirkin wrote:
> > No, I am just trying to understand why is hotplug event dangerous.
> > We still get it if we do device add before starting the VM, right?
> 
> I'm not sure if it's safe to call enable/disable_device() and
> pm_update_sci() before starting VM.
> So I'd like to avoid to call them instead of making sure it.
> If someone else ensures its safety, I'm willing to eliminate the if clause.

Well I don't think it's dangerous.
I guess it makes sense for whatever is supplied on qemu command line
to not cause a hotplug event.

> -- 
> yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Isaku Yamahata]

On Sat, Nov 13, 2010 at 10:59:15PM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 12, 2010 at 09:50:30PM +0900, Isaku Yamahata wrote:
> > On Fri, Nov 12, 2010 at 01:26:30PM +0200, Michael S. Tsirkin wrote:
> > > No, I am just trying to understand why is hotplug event dangerous.
> > > We still get it if we do device add before starting the VM, right?
> > 
> > I'm not sure if it's safe to call enable/disable_device() and
> > pm_update_sci() before starting VM.
> > So I'd like to avoid to call them instead of making sure it.
> > If someone else ensures its safety, I'm willing to eliminate the if clause.
> 
> Well I don't think it's dangerous.

Why?

> I guess it makes sense for whatever is supplied on qemu command line
> to not cause a hotplug event.

But, those functions inject a hot plug event into a guest.
-- 
yamahata

[Qemu-devel] Re: Cannot not unplug cold-plugged devices

[Michael S. Tsirkin]

On Sun, Nov 14, 2010 at 11:57:52AM +0900, Isaku Yamahata wrote:
> On Sat, Nov 13, 2010 at 10:59:15PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 12, 2010 at 09:50:30PM +0900, Isaku Yamahata wrote:
> > > On Fri, Nov 12, 2010 at 01:26:30PM +0200, Michael S. Tsirkin wrote:
> > > > No, I am just trying to understand why is hotplug event dangerous.
> > > > We still get it if we do device add before starting the VM, right?
> > > 
> > > I'm not sure if it's safe to call enable/disable_device() and
> > > pm_update_sci() before starting VM.
> > > So I'd like to avoid to call them instead of making sure it.
> > > If someone else ensures its safety, I'm willing to eliminate the if clause.
> > 
> > Well I don't think it's dangerous.
> 
> Why?

Because the same effect can be achieved by starting qemu with vm
stopped, adding device, then starting qemu.

> > I guess it makes sense for whatever is supplied on qemu command line
> > to not cause a hotplug event.
> 
> But, those functions inject a hot plug event into a guest.

That's why I think your patch is correct. Just the comment is
a bit misleading in saying it's dangerous.
It's just more correct without the event.

> -- 
> yamahata