Patchwork Re: Cannot not unplug cold-plugged devices

login
register
mail settings
Submitter Isaku Yamahata
Date Nov. 12, 2010, 7:24 a.m.
Message ID <20101112072411.GG29605@valinux.co.jp>
Download mbox | patch
Permalink /patch/70934/
State New
Headers show

Comments

Isaku Yamahata - Nov. 12, 2010, 7:24 a.m.
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(-)
Michael S. Tsirkin - Nov. 12, 2010, 9:18 a.m.
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
Isaku Yamahata - Nov. 12, 2010, 9:59 a.m.
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
>
Michael S. Tsirkin - Nov. 12, 2010, 11:26 a.m.
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
Isaku Yamahata - Nov. 12, 2010, 12:50 p.m.
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.
Michael S. Tsirkin - Nov. 13, 2010, 8:59 p.m.
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
Isaku Yamahata - Nov. 14, 2010, 2:57 a.m.
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.
Michael S. Tsirkin - Nov. 14, 2010, 10:11 a.m.
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

Patch

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;