diff mbox

[PULL,00/20] acpi,pc,pci fixes and enhancements

Message ID alpine.DEB.2.02.1402181421310.27926@kaball.uk.xensource.com
State Superseded
Headers show

Commit Message

Stefano Stabellini Feb. 18, 2014, 2:25 p.m. UTC 
On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > of the email :-P).
> > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > response to the guest writing to a magic ioport specifically to unplug
> > the emulated disk.
> > With this patch after the guest boots I can still access both xvda and
> > sda for the same disk, leading to fs corruptions.
> 
> Ok, the last paragraph is what I was missing.
> 
> So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> hotplug handler, dc->unplug is not called anymore.
> 
> But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't free
> the device, it just drops the disks underneath.  I think the simplest solution
> is to _not_ make it a dc->unplug callback at all, and call
> pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> qdev_unplug means "ask guest to start unplug", which is not what Xen wants to
> do here.

Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
Calling it directly from unplug_disks fixes the issue:


---

Call pci_piix3_xen_ide_unplug from unplug_disks

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Comments

Stefano Stabellini Feb. 18, 2014, 5:10 p.m. UTC | #1 
On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > of the email :-P).
> > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > response to the guest writing to a magic ioport specifically to unplug
> > > > the emulated disk.
> > > > With this patch after the guest boots I can still access both xvda and
> > > > sda for the same disk, leading to fs corruptions.
> > > 
> > > Ok, the last paragraph is what I was missing.
> > > 
> > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > hotplug handler, dc->unplug is not called anymore.
> > > 
> > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > free
> > > the device, it just drops the disks underneath.  I think the simplest
> > > solution
> > > is to _not_ make it a dc->unplug callback at all, and call
> > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > to
> > > do here.
> > 
> > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > Calling it directly from unplug_disks fixes the issue:
> > 
> > 
> > ---
> > 
> > Call pci_piix3_xen_ide_unplug from unplug_disks
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index 0eda301..40757eb 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> >      return 0;
> >  }
> > 
> > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> >  {
> >      PCIIDEState *pci_ide;
> >      DriveInfo *di;
> > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > void *data)
> >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> >      k->class_id = PCI_CLASS_STORAGE_IDE;
> >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > -    dc->unplug = pci_piix3_xen_ide_unplug;
> >  }
> > 
> >  static const TypeInfo piix3_ide_xen_info = {
> > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > index 70875e4..1d9d0e9 100644
> > --- a/hw/xen/xen_platform.c
> > +++ b/hw/xen/xen_platform.c
> > @@ -27,6 +27,7 @@
> > 
> >  #include "hw/hw.h"
> >  #include "hw/i386/pc.h"
> > +#include "hw/ide.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/irq.h"
> >  #include "hw/xen/xen_common.h"
> > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > *o)
> >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> >              PCI_CLASS_STORAGE_IDE
> >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > -        qdev_unplug(DEVICE(d), NULL);
> > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> >      }
> >  }
> > 
> > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > index 507e6d3..bc8bd32 100644
> > --- a/include/hw/ide.h
> > +++ b/include/hw/ide.h
> > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > **hd_table,
> >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > devfn);
> > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > 
> >  /* ide-mmio.c */
> > 
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks. Should I send it to Peter via the xen tree or anybody else wants
to pick this up?
Stefano Stabellini Feb. 19, 2014, 11:53 a.m. UTC | #2 
On Wed, 19 Feb 2014, Michael S. Tsirkin wrote:
> On Wed, Feb 19, 2014 at 11:08:25AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 18, 2014 at 05:10:00PM +0000, Stefano Stabellini wrote:
> > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > Il 18/02/2014 15:25, Stefano Stabellini ha scritto:
> > > > > On Tue, 18 Feb 2014, Paolo Bonzini wrote:
> > > > > > Il 18/02/2014 13:45, Stefano Stabellini ha scritto:
> > > > > > > Disk unplug: hw/ide/piix.c:pci_piix3_xen_ide_unplug (see the beginning
> > > > > > > of the email :-P).
> > > > > > > It is called by hw/xen/xen_platform.c:platform_fixed_ioport_writew, in
> > > > > > > response to the guest writing to a magic ioport specifically to unplug
> > > > > > > the emulated disk.
> > > > > > > With this patch after the guest boots I can still access both xvda and
> > > > > > > sda for the same disk, leading to fs corruptions.
> > > > > > 
> > > > > > Ok, the last paragraph is what I was missing.
> > > > > > 
> > > > > > So this is dc->unplug for the PIIX3 IDE device.  Because PCI declares a
> > > > > > hotplug handler, dc->unplug is not called anymore.
> > > > > > 
> > > > > > But unlike other dc->unplug callbacks, pci_piix3_xen_ide_unplug doesn't
> > > > > > free
> > > > > > the device, it just drops the disks underneath.  I think the simplest
> > > > > > solution
> > > > > > is to _not_ make it a dc->unplug callback at all, and call
> > > > > > pci_piix3_xen_ide_unplug from unplug_disks instead of qdev_unplug.
> > > > > > qdev_unplug means "ask guest to start unplug", which is not what Xen wants
> > > > > > to
> > > > > > do here.
> > > > > 
> > > > > Yes, you are right, pci_piix3_xen_ide_unplug is not called anymore.
> > > > > Calling it directly from unplug_disks fixes the issue:
> > > > > 
> > > > > 
> > > > > ---
> > > > > 
> > > > > Call pci_piix3_xen_ide_unplug from unplug_disks
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > > 
> > > > > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > > > > index 0eda301..40757eb 100644
> > > > > --- a/hw/ide/piix.c
> > > > > +++ b/hw/ide/piix.c
> > > > > @@ -167,7 +167,7 @@ static int pci_piix_ide_initfn(PCIDevice *dev)
> > > > >      return 0;
> > > > >  }
> > > > > 
> > > > > -static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > > > >  {
> > > > >      PCIIDEState *pci_ide;
> > > > >      DriveInfo *di;
> > > > > @@ -266,7 +266,6 @@ static void piix3_ide_xen_class_init(ObjectClass *klass,
> > > > > void *data)
> > > > >      k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
> > > > >      k->class_id = PCI_CLASS_STORAGE_IDE;
> > > > >      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> > > > > -    dc->unplug = pci_piix3_xen_ide_unplug;
> > > > >  }
> > > > > 
> > > > >  static const TypeInfo piix3_ide_xen_info = {
> > > > > diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
> > > > > index 70875e4..1d9d0e9 100644
> > > > > --- a/hw/xen/xen_platform.c
> > > > > +++ b/hw/xen/xen_platform.c
> > > > > @@ -27,6 +27,7 @@
> > > > > 
> > > > >  #include "hw/hw.h"
> > > > >  #include "hw/i386/pc.h"
> > > > > +#include "hw/ide.h"
> > > > >  #include "hw/pci/pci.h"
> > > > >  #include "hw/irq.h"
> > > > >  #include "hw/xen/xen_common.h"
> > > > > @@ -110,7 +111,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
> > > > > *o)
> > > > >      if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
> > > > >              PCI_CLASS_STORAGE_IDE
> > > > >              && strcmp(d->name, "xen-pci-passthrough") != 0) {
> > > > > -        qdev_unplug(DEVICE(d), NULL);
> > > > > +        pci_piix3_xen_ide_unplug(DEVICE(d));
> > > > >      }
> > > > >  }
> > > > > 
> > > > > diff --git a/include/hw/ide.h b/include/hw/ide.h
> > > > > index 507e6d3..bc8bd32 100644
> > > > > --- a/include/hw/ide.h
> > > > > +++ b/include/hw/ide.h
> > > > > @@ -17,6 +17,7 @@ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo
> > > > > **hd_table,
> > > > >  PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int
> > > > > devfn);
> > > > > +int pci_piix3_xen_ide_unplug(DeviceState *dev);
> > > > >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > > > > 
> > > > >  /* ide-mmio.c */
> > > > > 
> > > > 
> > > > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > > 
> > > Thanks. Should I send it to Peter via the xen tree or anybody else wants
> > > to pick this up?
> > 
> > I'll rebase my tree on top of this, to avoid bisect failures.
> 
> Oh sorry, I noticed what broke this is - is merged already.
> Pls merge fix through xen tree then, makes more sense.

All right, thanks.
diff mbox

Patch

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 0eda301..40757eb 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,7 +167,7 @@  static int pci_piix_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static int pci_piix3_xen_ide_unplug(DeviceState *dev)
+int pci_piix3_xen_ide_unplug(DeviceState *dev)
 {
     PCIIDEState *pci_ide;
     DriveInfo *di;
@@ -266,7 +266,6 @@  static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
-    dc->unplug = pci_piix3_xen_ide_unplug;
 }
 
 static const TypeInfo piix3_ide_xen_info = {
diff --git a/hw/xen/xen_platform.c b/hw/xen/xen_platform.c
index 70875e4..1d9d0e9 100644
--- a/hw/xen/xen_platform.c
+++ b/hw/xen/xen_platform.c
@@ -27,6 +27,7 @@ 
 
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
+#include "hw/ide.h"
 #include "hw/pci/pci.h"
 #include "hw/irq.h"
 #include "hw/xen/xen_common.h"
@@ -110,7 +111,7 @@  static void unplug_disks(PCIBus *b, PCIDevice *d, void *o)
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_STORAGE_IDE
             && strcmp(d->name, "xen-pci-passthrough") != 0) {
-        qdev_unplug(DEVICE(d), NULL);
+        pci_piix3_xen_ide_unplug(DEVICE(d));
     }
 }
 
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 507e6d3..bc8bd32 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -17,6 +17,7 @@  void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
 PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+int pci_piix3_xen_ide_unplug(DeviceState *dev);
 void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 
 /* ide-mmio.c */