| Message ID | 20101112072411.GG29605@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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
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.
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
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;