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

["Michael S. Tsirkin" <mst@redhat.com>]

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