From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFlde-0006x7-5n for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:27:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFldZ-0004uL-8U for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:27:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30756) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFldZ-0004u1-0N for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:27:05 -0500 Message-ID: <53036DA8.3080804@redhat.com> Date: Tue, 18 Feb 2014 15:26:48 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1392050814-31814-1-git-send-email-mst@redhat.com> <530351B4.1010402@redhat.com> <53035BD8.6050805@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 00/20] acpi,pc,pci fixes and enhancements List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Peter Maydell , xen-devel@lists.xensource.com, "Michael S. Tsirkin" , QEMU Developers , armbru@redhat.com, Anthony Liguori , Anthony.Perard@citrix.com 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 > > 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 Paolo From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PULL 00/20] acpi,pc,pci fixes and enhancements Date: Tue, 18 Feb 2014 15:26:48 +0100 Message-ID: <53036DA8.3080804@redhat.com> References: <1392050814-31814-1-git-send-email-mst@redhat.com> <530351B4.1010402@redhat.com> <53035BD8.6050805@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Stefano Stabellini Cc: Peter Maydell , xen-devel@lists.xensource.com, "Michael S. Tsirkin" , QEMU Developers , armbru@redhat.com, Anthony Liguori , Anthony.Perard@citrix.com List-Id: xen-devel@lists.xenproject.org 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 > > 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 Paolo