[Qemu-devel] [PATCH 0/3] xen: support PV on HVM guests

[Qemu-devel] [PATCH 0/3] xen: support PV on HVM guests

[Stefano Stabellini]

Hi all,
this small patch series implements the components needed by PV on HVM
guests to boot; in particular the "unplug protocol" to dynamically unplug
emulated disks and nics when the guest starts.

The patch to implement the Xen platform device has been sent to the list
several times before by Anthony, I believe no issues were found in the
last few iterations.

The other two patches are new and implement the disks and nics unplug.
The old code we have in qemu-xen to do this is not very "elegant", to
use an euphemism; I tried to prettify the implementation as much as I
could, please let me know if you have any suggestions to improve it.


Stefano Stabellini (2):
      pci: export pci_unplug_device
      xen: implement unplug protocol in xen_platform

Steven Smith (1):
      xen: Add the Xen platform pci device

 Makefile.target   |    2 +
 hw/hw.h           |    3 +
 hw/pc_piix.c      |    4 +
 hw/pci.c          |    2 +-
 hw/pci.h          |    1 +
 hw/pci_ids.h      |    2 +
 hw/xen_platform.c |  401 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 trace-events      |    3 +
 8 files changed, 417 insertions(+), 1 deletions(-)


A git branch based on xen-next is available here:

git://xenbits.xen.org/people/sstabellini/qemu-dm.git pv_on_hvm

Cheers,

Stefano

[Qemu-devel] [PATCH RESEND 1/3] xen: Add the Xen platform pci device

[stefano.stabellini]

From: Steven Smith <ssmith@xensource.com>

Introduce a new emulated PCI device, specific to fully virtualized Xen
guests.  The device is necessary for PV on HVM drivers to work.

Signed-off-by: Steven Smith <ssmith@xensource.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 Makefile.target   |    2 +
 hw/hw.h           |    3 +
 hw/pc_piix.c      |    4 +
 hw/pci_ids.h      |    2 +
 hw/xen_platform.c |  340 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 trace-events      |    3 +
 6 files changed, 354 insertions(+), 0 deletions(-)
 create mode 100644 hw/xen_platform.c

diff --git a/Makefile.target b/Makefile.target
index b1a0f6d..760aa02 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -218,6 +218,8 @@ obj-$(CONFIG_NO_XEN) += xen-stub.o
 obj-i386-$(CONFIG_XEN_MAPCACHE) += xen-mapcache.o
 obj-$(CONFIG_NO_XEN_MAPCACHE) += xen-mapcache-stub.o
 
+obj-i386-$(CONFIG_XEN) += xen_platform.o
+
 # Inter-VM PCI shared memory
 CONFIG_IVSHMEM =
 ifeq ($(CONFIG_KVM), y)
diff --git a/hw/hw.h b/hw/hw.h
index 56447a7..9dd7096 100644
--- a/hw/hw.h
+++ b/hw/hw.h
@@ -780,6 +780,9 @@ extern const VMStateDescription vmstate_ptimer;
 #define VMSTATE_INT32_LE(_f, _s)                                   \
     VMSTATE_SINGLE(_f, _s, 0, vmstate_info_int32_le, int32_t)
 
+#define VMSTATE_UINT8_TEST(_f, _s, _t)                               \
+    VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint8, uint8_t)
+
 #define VMSTATE_UINT16_TEST(_f, _s, _t)                               \
     VMSTATE_SINGLE_TEST(_f, _s, _t, 0, vmstate_info_uint16, uint16_t)
 
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 9a22a8a..76b01cc 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -140,6 +140,10 @@ static void pc_init1(ram_addr_t ram_size,
 
     pc_vga_init(pci_enabled? pci_bus: NULL);
 
+    if (xen_enabled()) {
+        pci_create_simple(pci_bus, -1, "xen-platform");
+    }
+
     /* init basic PC hardware */
     pc_basic_device_init(isa_irq, &rtc_state, xen_enabled());
 
diff --git a/hw/pci_ids.h b/hw/pci_ids.h
index d9457ed..d94578c 100644
--- a/hw/pci_ids.h
+++ b/hw/pci_ids.h
@@ -109,3 +109,5 @@
 #define PCI_DEVICE_ID_INTEL_82371AB      0x7111
 #define PCI_DEVICE_ID_INTEL_82371AB_2    0x7112
 #define PCI_DEVICE_ID_INTEL_82371AB_3    0x7113
+
+#define PCI_VENDOR_ID_XENSOURCE          0x5853
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
new file mode 100644
index 0000000..b167eee
--- /dev/null
+++ b/hw/xen_platform.c
@@ -0,0 +1,340 @@
+/*
+ * XEN platform pci device, formerly known as the event channel device
+ *
+ * Copyright (c) 2003-2004 Intel Corp.
+ * Copyright (c) 2006 XenSource
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <assert.h>
+
+#include "hw.h"
+#include "pc.h"
+#include "pci.h"
+#include "irq.h"
+#include "xen_common.h"
+#include "net.h"
+#include "xen_backend.h"
+#include "rwhandler.h"
+#include "trace.h"
+
+#include <xenguest.h>
+
+//#define DEBUG_PLATFORM
+
+#ifdef DEBUG_PLATFORM
+#define DPRINTF(fmt, ...) do { \
+    fprintf(stderr, "xen_platform: " fmt, ## __VA_ARGS__); \
+} while (0)
+#else
+#define DPRINTF(fmt, ...) do { } while (0)
+#endif
+
+#define PFFLAG_ROM_LOCK 1 /* Sets whether ROM memory area is RW or RO */
+
+typedef struct PCIXenPlatformState {
+    PCIDevice  pci_dev;
+    uint8_t flags; /* used only for version_id == 2 */
+    int drivers_blacklisted;
+    uint16_t driver_product_version;
+
+    /* Log from guest drivers */
+    char log_buffer[4096];
+    int log_buffer_off;
+} PCIXenPlatformState;
+
+#define XEN_PLATFORM_IOPORT 0x10
+
+/* Send bytes to syslog */
+static void log_writeb(PCIXenPlatformState *s, char val)
+{
+    if (val == '\n' || s->log_buffer_off == sizeof(s->log_buffer) - 1) {
+        /* Flush buffer */
+        s->log_buffer[s->log_buffer_off] = 0;
+        trace_xen_platform_log(s->log_buffer);
+        s->log_buffer_off = 0;
+    } else {
+        s->log_buffer[s->log_buffer_off++] = val;
+    }
+}
+
+/* Xen Platform, Fixed IOPort */
+
+static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIXenPlatformState *s = opaque;
+
+    switch (addr - XEN_PLATFORM_IOPORT) {
+    case 0:
+        /* TODO: */
+        /* Unplug devices.  Value is a bitmask of which devices to
+           unplug, with bit 0 the IDE devices, bit 1 the network
+           devices, and bit 2 the non-primary-master IDE devices. */
+        break;
+    case 2:
+        switch (val) {
+        case 1:
+            DPRINTF("Citrix Windows PV drivers loaded in guest\n");
+            break;
+        case 0:
+            DPRINTF("Guest claimed to be running PV product 0?\n");
+            break;
+        default:
+            DPRINTF("Unknown PV product %d loaded in guest\n", val);
+            break;
+        }
+        s->driver_product_version = val;
+        break;
+    }
+}
+
+static void platform_fixed_ioport_writel(void *opaque, uint32_t addr,
+                                         uint32_t val)
+{
+    switch (addr - XEN_PLATFORM_IOPORT) {
+    case 0:
+        /* PV driver version */
+        break;
+    }
+}
+
+static void platform_fixed_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIXenPlatformState *s = opaque;
+
+    switch (addr - XEN_PLATFORM_IOPORT) {
+    case 0: /* Platform flags */ {
+        hvmmem_type_t mem_type = (val & PFFLAG_ROM_LOCK) ?
+            HVMMEM_ram_ro : HVMMEM_ram_rw;
+        if (xc_hvm_set_mem_type(xen_xc, xen_domid, mem_type, 0xc0, 0x40)) {
+            DPRINTF("unable to change ro/rw state of ROM memory area!\n");
+        } else {
+            s->flags = val & PFFLAG_ROM_LOCK;
+            DPRINTF("changed ro/rw state of ROM memory area. now is %s state.\n",
+                    (mem_type == HVMMEM_ram_ro ? "ro":"rw"));
+        }
+        break;
+    }
+    case 2:
+        log_writeb(s, val);
+        break;
+    }
+}
+
+static uint32_t platform_fixed_ioport_readw(void *opaque, uint32_t addr)
+{
+    PCIXenPlatformState *s = opaque;
+
+    switch (addr - XEN_PLATFORM_IOPORT) {
+    case 0:
+        if (s->drivers_blacklisted) {
+            /* The drivers will recognise this magic number and refuse
+             * to do anything. */
+            return 0xd249;
+        } else {
+            /* Magic value so that you can identify the interface. */
+            return 0x49d2;
+        }
+    default:
+        return 0xffff;
+    }
+}
+
+static uint32_t platform_fixed_ioport_readb(void *opaque, uint32_t addr)
+{
+    PCIXenPlatformState *s = opaque;
+
+    switch (addr - XEN_PLATFORM_IOPORT) {
+    case 0:
+        /* Platform flags */
+        return s->flags;
+    case 2:
+        /* Version number */
+        return 1;
+    default:
+        return 0xff;
+    }
+}
+
+static void platform_fixed_ioport_reset(void *opaque)
+{
+    PCIXenPlatformState *s = opaque;
+
+    platform_fixed_ioport_writeb(s, XEN_PLATFORM_IOPORT, 0);
+}
+
+static void platform_fixed_ioport_init(PCIXenPlatformState* s)
+{
+    register_ioport_write(XEN_PLATFORM_IOPORT, 16, 4, platform_fixed_ioport_writel, s);
+    register_ioport_write(XEN_PLATFORM_IOPORT, 16, 2, platform_fixed_ioport_writew, s);
+    register_ioport_write(XEN_PLATFORM_IOPORT, 16, 1, platform_fixed_ioport_writeb, s);
+    register_ioport_read(XEN_PLATFORM_IOPORT, 16, 2, platform_fixed_ioport_readw, s);
+    register_ioport_read(XEN_PLATFORM_IOPORT, 16, 1, platform_fixed_ioport_readb, s);
+}
+
+/* Xen Platform PCI Device */
+
+static uint32_t xen_platform_ioport_readb(void *opaque, uint32_t addr)
+{
+    addr &= 0xff;
+
+    if (addr == 0) {
+        return platform_fixed_ioport_readb(opaque, XEN_PLATFORM_IOPORT);
+    } else {
+        return ~0u;
+    }
+}
+
+static void xen_platform_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIXenPlatformState *s = opaque;
+
+    addr &= 0xff;
+    val  &= 0xff;
+
+    switch (addr) {
+    case 0: /* Platform flags */
+        platform_fixed_ioport_writeb(opaque, XEN_PLATFORM_IOPORT, val);
+        break;
+    case 8:
+        log_writeb(s, val);
+        break;
+    default:
+        break;
+    }
+}
+
+static void platform_ioport_map(PCIDevice *pci_dev, int region_num, pcibus_t addr, pcibus_t size, int type)
+{
+    PCIXenPlatformState *d = DO_UPCAST(PCIXenPlatformState, pci_dev, pci_dev);
+
+    register_ioport_write(addr, size, 1, xen_platform_ioport_writeb, d);
+    register_ioport_read(addr, size, 1, xen_platform_ioport_readb, d);
+}
+
+static uint32_t platform_mmio_read(ReadWriteHandler *handler, pcibus_t addr, int len)
+{
+    DPRINTF("Warning: attempted read from physical address "
+            "0x" TARGET_FMT_plx " in xen platform mmio space\n", addr);
+
+    return 0;
+}
+
+static void platform_mmio_write(ReadWriteHandler *handler, pcibus_t addr,
+                                uint32_t val, int len)
+{
+    DPRINTF("Warning: attempted write of 0x%x to physical "
+            "address 0x" TARGET_FMT_plx " in xen platform mmio space\n",
+            val, addr);
+}
+
+static ReadWriteHandler platform_mmio_handler = {
+    .read = &platform_mmio_read,
+    .write = &platform_mmio_write,
+};
+
+static void platform_mmio_map(PCIDevice *d, int region_num,
+                              pcibus_t addr, pcibus_t size, int type)
+{
+    int mmio_io_addr;
+
+    mmio_io_addr = cpu_register_io_memory_simple(&platform_mmio_handler,
+                                                 DEVICE_NATIVE_ENDIAN);
+
+    cpu_register_physical_memory(addr, size, mmio_io_addr);
+}
+
+static int xen_platform_post_load(void *opaque, int version_id)
+{
+    PCIXenPlatformState *s = opaque;
+
+    platform_fixed_ioport_writeb(s, XEN_PLATFORM_IOPORT, s->flags);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_xen_platform = {
+    .name = "platform",
+    .version_id = 4,
+    .minimum_version_id = 4,
+    .minimum_version_id_old = 4,
+    .post_load = xen_platform_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_PCI_DEVICE(pci_dev, PCIXenPlatformState),
+        VMSTATE_UINT8(flags, PCIXenPlatformState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int xen_platform_initfn(PCIDevice *dev)
+{
+    PCIXenPlatformState *d = DO_UPCAST(PCIXenPlatformState, pci_dev, dev);
+    uint8_t *pci_conf;
+
+    pci_conf = d->pci_dev.config;
+
+    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_XENSOURCE);
+    pci_config_set_device_id(pci_conf, 0x0001);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, PCI_VENDOR_ID_XENSOURCE);
+    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0001);
+
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
+
+    pci_config_set_revision(pci_conf, 1);
+    pci_config_set_prog_interface(pci_conf, 0);
+
+    pci_config_set_class(pci_conf, PCI_CLASS_OTHERS << 8 | 0x80);
+
+    pci_conf[PCI_INTERRUPT_PIN] = 1;
+
+    pci_register_bar(&d->pci_dev, 0, 0x100,
+            PCI_BASE_ADDRESS_SPACE_IO, platform_ioport_map);
+
+    /* reserve 16MB mmio address for share memory*/
+    pci_register_bar(&d->pci_dev, 1, 0x1000000,
+            PCI_BASE_ADDRESS_MEM_PREFETCH, platform_mmio_map);
+
+    platform_fixed_ioport_init(d);
+
+    return 0;
+}
+
+static void platform_reset(DeviceState *dev)
+{
+    PCIXenPlatformState *s = DO_UPCAST(PCIXenPlatformState, pci_dev.qdev, dev);
+
+    platform_fixed_ioport_reset(s);
+}
+
+static PCIDeviceInfo xen_platform_info = {
+    .init = xen_platform_initfn,
+    .qdev.name = "xen-platform",
+    .qdev.desc = "XEN platform pci device",
+    .qdev.size = sizeof(PCIXenPlatformState),
+    .qdev.vmsd = &vmstate_xen_platform,
+    .qdev.reset = platform_reset,
+};
+
+static void xen_platform_register(void)
+{
+    pci_qdev_register(&xen_platform_info);
+}
+
+device_init(xen_platform_register);
diff --git a/trace-events b/trace-events
index 975baa3..9c51898 100644
--- a/trace-events
+++ b/trace-events
@@ -387,3 +387,6 @@ disable xen_unmap_block(void* addr, unsigned long size) "%p, size %#lx"
 
 # exec.c
 disable qemu_put_ram_ptr(void* addr) "%p"
+
+# hw/xen_platform.c
+disable xen_platform_log(char *s) "xen platform: %s"
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH RESEND 1/3] xen: Add the Xen platform pci device

[Michael S. Tsirkin]

On Thu, Jun 16, 2011 at 05:05:17PM +0100, stefano.stabellini@eu.citrix.com wrote:
> diff --git a/hw/pci_ids.h b/hw/pci_ids.h
> index d9457ed..d94578c 100644
> --- a/hw/pci_ids.h
> +++ b/hw/pci_ids.h
> @@ -109,3 +109,5 @@
>  #define PCI_DEVICE_ID_INTEL_82371AB      0x7111
>  #define PCI_DEVICE_ID_INTEL_82371AB_2    0x7112
>  #define PCI_DEVICE_ID_INTEL_82371AB_3    0x7113
> +
> +#define PCI_VENDOR_ID_XENSOURCE          0x5853

Please stick to names in  include/linux/pci_ids.h
in the linux tree.

$ grep XEN /scm/linux-2.6/include/linux/pci_ids.h 
#define PCI_VENDOR_ID_NETXEN            0x4040
#define PCI_VENDOR_ID_XEN               0x5853
#define PCI_DEVICE_ID_XEN_PLATFORM      0x0001



> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> new file mode 100644
> index 0000000..b167eee

...

> +
> +    pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_XENSOURCE);
> +    pci_config_set_device_id(pci_conf, 0x0001);

PCI_DEVICE_ID_XEN_PLATFORM

> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, PCI_VENDOR_ID_XENSOURCE);
> +    pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0001);

Same.

[Qemu-devel] [PATCH 2/3] pci: export pci_unplug_device

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

pci_unplug_device is needed by the xen_platform device to perfom dynamic
nic unplug.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/pci.c |    2 +-
 hw/pci.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 1d297d6..679e976 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1692,7 +1692,7 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
     return 0;
 }
 
-static int pci_unplug_device(DeviceState *qdev)
+int pci_unplug_device(DeviceState *qdev)
 {
     PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
     PCIDeviceInfo *info = container_of(qdev->info, PCIDeviceInfo, qdev);
diff --git a/hw/pci.h b/hw/pci.h
index 0d288ce..868f793 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -452,6 +452,7 @@ typedef struct {
 
 void pci_qdev_register(PCIDeviceInfo *info);
 void pci_qdev_register_many(PCIDeviceInfo *info);
+int pci_unplug_device(DeviceState *qdev);
 
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
                                     const char *name);
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH 2/3] pci: export pci_unplug_device

[Michael S. Tsirkin]

On Thu, Jun 16, 2011 at 05:05:18PM +0100, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> pci_unplug_device is needed by the xen_platform device to perfom dynamic
> nic unplug.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

I think it's better to go through qdev, pci_unplug_device
was intended as an internal API.

> ---
>  hw/pci.c |    2 +-
>  hw/pci.h |    1 +
>  2 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 1d297d6..679e976 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1692,7 +1692,7 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      return 0;
>  }
>  
> -static int pci_unplug_device(DeviceState *qdev)
> +int pci_unplug_device(DeviceState *qdev)
>  {
>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>      PCIDeviceInfo *info = container_of(qdev->info, PCIDeviceInfo, qdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 0d288ce..868f793 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -452,6 +452,7 @@ typedef struct {
>  
>  void pci_qdev_register(PCIDeviceInfo *info);
>  void pci_qdev_register_many(PCIDeviceInfo *info);
> +int pci_unplug_device(DeviceState *qdev);
>  
>  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
>                                      const char *name);
> -- 
> 1.7.2.3
> 

Re: [Qemu-devel] [PATCH 2/3] pci: export pci_unplug_device

[Stefano Stabellini]

On Mon, 20 Jun 2011, Michael S. Tsirkin wrote:
> On Thu, Jun 16, 2011 at 05:05:18PM +0100, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > pci_unplug_device is needed by the xen_platform device to perfom dynamic
> > nic unplug.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> I think it's better to go through qdev, pci_unplug_device
> was intended as an internal API.

You mean qdev_unplug? Sure, I'll use that instead and drop this patch.

[Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

The unplug protocol is necessary to support PV drivers in the guest: the
drivers expect to be able to "unplug" emulated disks and nics before
initializing the Xen PV interfaces.
It is responsibility of the guest to make sure that the unplug is done
before the emulated devices or the PV interface start to be used.

We use pci_for_each_device to walk the PCI bus, identify the devices and
disks that we want to disable and dynamically unplug them.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_platform.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 62 insertions(+), 1 deletions(-)

diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index b167eee..9f8c843 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -34,6 +34,9 @@
 #include "xen_backend.h"
 #include "rwhandler.h"
 #include "trace.h"
+#include "hw/ide/internal.h"
+#include "hw/ide/pci.h"
+#include "hw/pci_ids.h"
 
 #include <xenguest.h>
 
@@ -76,6 +79,54 @@ static void log_writeb(PCIXenPlatformState *s, char val)
 }
 
 /* Xen Platform, Fixed IOPort */
+#define UNPLUG_ALL_IDE_DISKS 1
+#define UNPLUG_ALL_NICS 2
+#define UNPLUG_AUX_IDE_DISKS 4
+
+static int unplug_param;
+
+static void unplug_nic(PCIBus *b, PCIDevice *d)
+{
+    if (d->config[0xa] == 0 && d->config[0xb] == 2) {
+        pci_unplug_device(&(d->qdev));
+    }
+}
+
+static void pci_unplug_nics(PCIBus *bus)
+{
+    pci_for_each_device(bus, 0, unplug_nic);
+}
+
+static void unplug_disks(PCIBus *b, PCIDevice *d)
+{
+    if (d->config[0xa] == 1 && d->config[0xb] == 1) {
+        PCIIDEState *pci_ide = DO_UPCAST(PCIIDEState, dev, d);
+        DriveInfo *di;
+        int i = 0;
+
+        if (unplug_param & UNPLUG_AUX_IDE_DISKS)
+            i++;
+
+        for (; i < 3; i++) {
+            di = drive_get_by_index(IF_IDE, i); 
+            if (di != NULL && di->bdrv != NULL && di->bdrv->type != BDRV_TYPE_CDROM) {
+                DeviceState *ds = bdrv_get_attached(di->bdrv);
+                if (ds)
+                    bdrv_detach(di->bdrv, ds);
+                bdrv_close(di->bdrv);
+                pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
+                drive_put_ref(di);
+            }
+        }
+        ide_bus_reset(&pci_ide->bus[0]);
+        ide_bus_reset(&pci_ide->bus[1]);
+    }
+}
+
+static void pci_unplug_disks(PCIBus *bus)
+{
+    pci_for_each_device(bus, 0, unplug_disks);
+}
 
 static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
 {
@@ -83,10 +134,20 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
 
     switch (addr - XEN_PLATFORM_IOPORT) {
     case 0:
-        /* TODO: */
+        unplug_param = val;
         /* Unplug devices.  Value is a bitmask of which devices to
            unplug, with bit 0 the IDE devices, bit 1 the network
            devices, and bit 2 the non-primary-master IDE devices. */
+        if (val & UNPLUG_ALL_IDE_DISKS || val & UNPLUG_AUX_IDE_DISKS) {
+            DPRINTF("unplug disks\n");
+            qemu_aio_flush();
+            bdrv_flush_all();
+            pci_unplug_disks(s->pci_dev.bus);
+        }
+        if (val & UNPLUG_ALL_NICS) {
+            DPRINTF("unplug nics\n");
+            pci_unplug_nics(s->pci_dev.bus);
+        }
         break;
     case 2:
         switch (val) {
-- 
1.7.2.3

Re: [Qemu-devel] [Xen-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Anthony PERARD]

On Thu, Jun 16, 2011 at 17:05,  <stefano.stabellini@eu.citrix.com> wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
>
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen_platform.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
>
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index b167eee..9f8c843 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -34,6 +34,9 @@
>  #include "xen_backend.h"
>  #include "rwhandler.h"
>  #include "trace.h"
> +#include "hw/ide/internal.h"
> +#include "hw/ide/pci.h"
> +#include "hw/pci_ids.h"
>
>  #include <xenguest.h>
>
> @@ -76,6 +79,54 @@ static void log_writeb(PCIXenPlatformState *s, char val)
>  }
>
>  /* Xen Platform, Fixed IOPort */
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4
> +
> +static int unplug_param;
> +
> +static void unplug_nic(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 0 && d->config[0xb] == 2) {

You should use:
pci_get_word(d->config+PCI_CLASS_DEVICE) == PCI_CLASS_NETWORK_ETHERNET

It'll be clearer.

> +        pci_unplug_device(&(d->qdev));
> +    }
> +}
> +
> +static void pci_unplug_nics(PCIBus *bus)
> +{
> +    pci_for_each_device(bus, 0, unplug_nic);
> +}
> +
> +static void unplug_disks(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 1 && d->config[0xb] == 1) {

Same here with PCI_CLASS_STORAGE_IDE.

Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] [Xen-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Fri, 17 Jun 2011, Anthony PERARD wrote:
> > +static void unplug_nic(PCIBus *b, PCIDevice *d)
> > +{
> > +    if (d->config[0xa] == 0 && d->config[0xb] == 2) {
> 
> You should use:
> pci_get_word(d->config+PCI_CLASS_DEVICE) == PCI_CLASS_NETWORK_ETHERNET
> 
> It'll be clearer.
> 

thanks, I'll do that

> > +static void unplug_disks(PCIBus *b, PCIDevice *d)
> > +{
> > +    if (d->config[0xa] == 1 && d->config[0xb] == 1) {
> 
> Same here with PCI_CLASS_STORAGE_IDE.
> 

OK

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Michael S. Tsirkin]

On Thu, Jun 16, 2011 at 05:05:19PM +0100, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> The unplug protocol is necessary to support PV drivers in the guest: the
> drivers expect to be able to "unplug" emulated disks and nics before
> initializing the Xen PV interfaces.
> It is responsibility of the guest to make sure that the unplug is done
> before the emulated devices or the PV interface start to be used.
> 
> We use pci_for_each_device to walk the PCI bus, identify the devices and
> disks that we want to disable and dynamically unplug them.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  hw/xen_platform.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 62 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> index b167eee..9f8c843 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -34,6 +34,9 @@
>  #include "xen_backend.h"
>  #include "rwhandler.h"
>  #include "trace.h"
> +#include "hw/ide/internal.h"

I'm not an expert here but it looks like
you should put some code in hw/ide/xen.c
and export an API from there rather
than calling ide_bus_reset and tweaking
PCIIDEState directly.

> +#include "hw/ide/pci.h"
> +#include "hw/pci_ids.h"
>  
>  #include <xenguest.h>
>  
> @@ -76,6 +79,54 @@ static void log_writeb(PCIXenPlatformState *s, char val)
>  }
>  
>  /* Xen Platform, Fixed IOPort */
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4
> +
> +static int unplug_param;
> +
> +static void unplug_nic(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 0 && d->config[0xb] == 2) {

Please use registers from pci_regs.h and pci_ids.h

> +        pci_unplug_device(&(d->qdev));

Can't you use qdev_unplug?
That does other useful checks and updates system state.
Also, are there non hotpluggable devices?
If not you can assert on qdev_unplug failure.

> +    }
> +}
> +
> +static void pci_unplug_nics(PCIBus *bus)
> +{
> +    pci_for_each_device(bus, 0, unplug_nic);
> +}
> +
> +static void unplug_disks(PCIBus *b, PCIDevice *d)
> +{
> +    if (d->config[0xa] == 1 && d->config[0xb] == 1) {

Same comment about hardcoded constants.

> +        PCIIDEState *pci_ide = DO_UPCAST(PCIIDEState, dev, d);
> +        DriveInfo *di;
> +        int i = 0;
> +
> +        if (unplug_param & UNPLUG_AUX_IDE_DISKS)
> +            i++;
> +
> +        for (; i < 3; i++) {
> +            di = drive_get_by_index(IF_IDE, i); 
> +            if (di != NULL && di->bdrv != NULL && di->bdrv->type != BDRV_TYPE_CDROM) {

line too long

> +                DeviceState *ds = bdrv_get_attached(di->bdrv);
> +                if (ds)
> +                    bdrv_detach(di->bdrv, ds);
> +                bdrv_close(di->bdrv);
> +                pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> +                drive_put_ref(di);
> +            }
> +        }
> +        ide_bus_reset(&pci_ide->bus[0]);
> +        ide_bus_reset(&pci_ide->bus[1]);
> +    }
> +}
> +
> +static void pci_unplug_disks(PCIBus *bus)
> +{
> +    pci_for_each_device(bus, 0, unplug_disks);
> +}
>  
>  static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>  {
> @@ -83,10 +134,20 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
>  
>      switch (addr - XEN_PLATFORM_IOPORT) {
>      case 0:
> -        /* TODO: */
> +        unplug_param = val;
>          /* Unplug devices.  Value is a bitmask of which devices to
>             unplug, with bit 0 the IDE devices, bit 1 the network
>             devices, and bit 2 the non-primary-master IDE devices. */
> +        if (val & UNPLUG_ALL_IDE_DISKS || val & UNPLUG_AUX_IDE_DISKS) {
> +            DPRINTF("unplug disks\n");
> +            qemu_aio_flush();
> +            bdrv_flush_all();
> +            pci_unplug_disks(s->pci_dev.bus);
> +        }
> +        if (val & UNPLUG_ALL_NICS) {
> +            DPRINTF("unplug nics\n");
> +            pci_unplug_nics(s->pci_dev.bus);
> +        }
>          break;
>      case 2:
>          switch (val) {
> -- 
> 1.7.2.3
> 

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 20.06.2011 10:28, schrieb Alexander Graf:
> 
> On 16.06.2011, at 18:05, <stefano.stabellini@eu.citrix.com> <stefano.stabellini@eu.citrix.com> wrote:
> 
>> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>> The unplug protocol is necessary to support PV drivers in the guest: the
>> drivers expect to be able to "unplug" emulated disks and nics before
>> initializing the Xen PV interfaces.
>> It is responsibility of the guest to make sure that the unplug is done
>> before the emulated devices or the PV interface start to be used.
>>
>> We use pci_for_each_device to walk the PCI bus, identify the devices and
>> disks that we want to disable and dynamically unplug them.
> 
> Kevin, please check the block parts of this code.
> Michael, please check the PCI parts of this code.
> 
> Thanks :)
> 
> Alex
> 
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> ---
>> hw/xen_platform.c |   63 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 62 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
>> index b167eee..9f8c843 100644
>> --- a/hw/xen_platform.c
>> +++ b/hw/xen_platform.c
>> @@ -34,6 +34,9 @@
>> #include "xen_backend.h"
>> #include "rwhandler.h"
>> #include "trace.h"
>> +#include "hw/ide/internal.h"

Sorry, no. :-)

This is not using a proper interface, but just a hack that depends on
the internal structure of the IDE emulation. It's going to break sooner
or later.

It seems your problem is that IDE isn't unpluggable. I'm not entirely
sure what the right solution is, maybe just adding a new xen-ide device
that is used for the Xen machine and closely resembles piix4-ide, but
can be hot-unplugged.

Kevin

>> +#include "hw/ide/pci.h"
>> +#include "hw/pci_ids.h"
>>
>> #include <xenguest.h>
>>
>> @@ -76,6 +79,54 @@ static void log_writeb(PCIXenPlatformState *s, char val)
>> }
>>
>> /* Xen Platform, Fixed IOPort */
>> +#define UNPLUG_ALL_IDE_DISKS 1
>> +#define UNPLUG_ALL_NICS 2
>> +#define UNPLUG_AUX_IDE_DISKS 4
>> +
>> +static int unplug_param;
>> +
>> +static void unplug_nic(PCIBus *b, PCIDevice *d)
>> +{
>> +    if (d->config[0xa] == 0 && d->config[0xb] == 2) {
>> +        pci_unplug_device(&(d->qdev));
>> +    }
>> +}
>> +
>> +static void pci_unplug_nics(PCIBus *bus)
>> +{
>> +    pci_for_each_device(bus, 0, unplug_nic);
>> +}
>> +
>> +static void unplug_disks(PCIBus *b, PCIDevice *d)
>> +{
>> +    if (d->config[0xa] == 1 && d->config[0xb] == 1) {
>> +        PCIIDEState *pci_ide = DO_UPCAST(PCIIDEState, dev, d);
>> +        DriveInfo *di;
>> +        int i = 0;
>> +
>> +        if (unplug_param & UNPLUG_AUX_IDE_DISKS)
>> +            i++;
>> +
>> +        for (; i < 3; i++) {
>> +            di = drive_get_by_index(IF_IDE, i); 
>> +            if (di != NULL && di->bdrv != NULL && di->bdrv->type != BDRV_TYPE_CDROM) {
>> +                DeviceState *ds = bdrv_get_attached(di->bdrv);
>> +                if (ds)
>> +                    bdrv_detach(di->bdrv, ds);
>> +                bdrv_close(di->bdrv);
>> +                pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
>> +                drive_put_ref(di);
>> +            }
>> +        }
>> +        ide_bus_reset(&pci_ide->bus[0]);
>> +        ide_bus_reset(&pci_ide->bus[1]);
>> +    }
>> +}
>> +
>> +static void pci_unplug_disks(PCIBus *bus)
>> +{
>> +    pci_for_each_device(bus, 0, unplug_disks);
>> +}
>>
>> static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>> {
>> @@ -83,10 +134,20 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
>>
>>     switch (addr - XEN_PLATFORM_IOPORT) {
>>     case 0:
>> -        /* TODO: */
>> +        unplug_param = val;
>>         /* Unplug devices.  Value is a bitmask of which devices to
>>            unplug, with bit 0 the IDE devices, bit 1 the network
>>            devices, and bit 2 the non-primary-master IDE devices. */
>> +        if (val & UNPLUG_ALL_IDE_DISKS || val & UNPLUG_AUX_IDE_DISKS) {
>> +            DPRINTF("unplug disks\n");
>> +            qemu_aio_flush();
>> +            bdrv_flush_all();
>> +            pci_unplug_disks(s->pci_dev.bus);
>> +        }
>> +        if (val & UNPLUG_ALL_NICS) {
>> +            DPRINTF("unplug nics\n");
>> +            pci_unplug_nics(s->pci_dev.bus);
>> +        }
>>         break;
>>     case 2:
>>         switch (val) {
>> -- 
>> 1.7.2.3
>>
> 

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Mon, 20 Jun 2011, Kevin Wolf wrote:
> >> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
> >> index b167eee..9f8c843 100644
> >> --- a/hw/xen_platform.c
> >> +++ b/hw/xen_platform.c
> >> @@ -34,6 +34,9 @@
> >> #include "xen_backend.h"
> >> #include "rwhandler.h"
> >> #include "trace.h"
> >> +#include "hw/ide/internal.h"
> 
> Sorry, no. :-)
> 
> This is not using a proper interface, but just a hack that depends on
> the internal structure of the IDE emulation. It's going to break sooner
> or later.
> 
> It seems your problem is that IDE isn't unpluggable. I'm not entirely
> sure what the right solution is, maybe just adding a new xen-ide device
> that is used for the Xen machine and closely resembles piix4-ide, but
> can be hot-unplugged.
> 

Actually the only thing I was using from hw/ide/internal.h is
ide_bus_reset, but I can replace it with a qdev_reset_all call.
Also it seems that at least Linux PV on HVM doesn't have any problems
even without an ide bus reset when the disk is unplugged.

So I am going to resend this patch removing this import and replacing
ide_bus_reset with qdev_reset_all. All the other block related functions
I am using seem to be public.

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 23.06.2011 15:16, schrieb Stefano Stabellini:
> On Mon, 20 Jun 2011, Kevin Wolf wrote:
>>>> diff --git a/hw/xen_platform.c b/hw/xen_platform.c
>>>> index b167eee..9f8c843 100644
>>>> --- a/hw/xen_platform.c
>>>> +++ b/hw/xen_platform.c
>>>> @@ -34,6 +34,9 @@
>>>> #include "xen_backend.h"
>>>> #include "rwhandler.h"
>>>> #include "trace.h"
>>>> +#include "hw/ide/internal.h"
>>
>> Sorry, no. :-)
>>
>> This is not using a proper interface, but just a hack that depends on
>> the internal structure of the IDE emulation. It's going to break sooner
>> or later.
>>
>> It seems your problem is that IDE isn't unpluggable. I'm not entirely
>> sure what the right solution is, maybe just adding a new xen-ide device
>> that is used for the Xen machine and closely resembles piix4-ide, but
>> can be hot-unplugged.
>>
> 
> Actually the only thing I was using from hw/ide/internal.h is
> ide_bus_reset, but I can replace it with a qdev_reset_all call.
> Also it seems that at least Linux PV on HVM doesn't have any problems
> even without an ide bus reset when the disk is unplugged.
> 
> So I am going to resend this patch removing this import and replacing
> ide_bus_reset with qdev_reset_all. All the other block related functions
> I am using seem to be public.

hw/ide/pci.h is just as internal as internal.h is. And even if you
managed to access the same things without any IDE header file, I still
think it's not the right level of abstraction because it relies on the
implementation details of IDE.

Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
really look right to you to do anywhere outside IDE?

I'm basically looking for the same as Michael who wanted to have network
unplug handled through qdev, just that the IDE code doesn't support
unplug yet.

Kevin

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Mon, 27 Jun 2011, Kevin Wolf wrote:
> hw/ide/pci.h is just as internal as internal.h is. And even if you
> managed to access the same things without any IDE header file, I still
> think it's not the right level of abstraction because it relies on the
> implementation details of IDE.
> 
> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
> really look right to you to do anywhere outside IDE?
> 
> I'm basically looking for the same as Michael who wanted to have network
> unplug handled through qdev, just that the IDE code doesn't support
> unplug yet.

I understand.

I created pci_piix3_xen_ide_init and moved the unplug code to
hw/ide/piix.c so that we don't have any internal knowledge of IDE code
in xen_platform.c any more, see below.

One remaining problem is that the generic pci unplug function is not
what I need in this case so I have to setup my own; but I hope that in
general the changes are going in the right direction.

---

diff --git a/hw/ide.h b/hw/ide.h
index 34d9394..a490cbb 100644
--- a/hw/ide.h
+++ b/hw/ide.h
@@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
 /* ide-pci.c */
 void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
                          int secondary_ide_enabled);
+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);
 void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index c349644..8ae9ff0 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev)
     return pci_piix_ide_initfn(d);
 }
 
+static int pci_piix3_xen_ide_unplug(DeviceState *dev)
+{
+    PCIDevice *pci_dev;
+    PCIIDEState *pci_ide;
+    DriveInfo *di;
+    int i = 0;
+
+    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
+    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
+
+    for (; i < 3; i++) {
+        di = drive_get_by_index(IF_IDE, i); 
+        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
+            DeviceState *ds = bdrv_get_attached(di->bdrv);
+            if (ds)
+                bdrv_detach(di->bdrv, ds);
+            bdrv_close(di->bdrv);
+            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
+            drive_put_ref(di);
+        }
+    }
+    qdev_reset_all(&(pci_ide->dev.qdev));
+    return 0;
+}
+
+PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+{
+    PCIDevice *dev;
+
+    dev = pci_create_simple(bus, devfn, "piix3-ide");
+    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
+    pci_ide_create_devs(dev, hd_table);
+    return dev;
+}
+
 /* hd_table must contain 4 block drivers */
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 8dbeb0c..b59adcc 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -155,7 +155,11 @@ static void pc_init1(ram_addr_t ram_size,
     ide_drive_get(hd, MAX_IDE_BUS);
     if (pci_enabled) {
         PCIDevice *dev;
-        dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
+        if (xen_enabled()) {
+            dev = pci_piix3_xen_ide_init(pci_bus, hd, piix3_devfn + 1);
+        } else {
+            dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
+        }
         idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0");
         idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1");
     } else {
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index b167eee..a271369 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -76,6 +76,35 @@ static void log_writeb(PCIXenPlatformState *s, char val)
 }
 
 /* Xen Platform, Fixed IOPort */
+#define UNPLUG_ALL_IDE_DISKS 1
+#define UNPLUG_ALL_NICS 2
+#define UNPLUG_AUX_IDE_DISKS 4
+
+static void unplug_nic(PCIBus *b, PCIDevice *d)
+{
+    if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
+            PCI_CLASS_NETWORK_ETHERNET) {
+        qdev_unplug(&(d->qdev));
+    }
+}
+
+static void pci_unplug_nics(PCIBus *bus)
+{
+    pci_for_each_device(bus, 0, unplug_nic);
+}
+
+static void unplug_disks(PCIBus *b, PCIDevice *d)
+{
+    if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
+            PCI_CLASS_STORAGE_IDE) {
+        qdev_unplug(&(d->qdev));
+    }
+}
+
+static void pci_unplug_disks(PCIBus *bus)
+{
+    pci_for_each_device(bus, 0, unplug_disks);
+}
 
 static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
 {
@@ -83,10 +112,22 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
 
     switch (addr - XEN_PLATFORM_IOPORT) {
     case 0:
-        /* TODO: */
         /* Unplug devices.  Value is a bitmask of which devices to
            unplug, with bit 0 the IDE devices, bit 1 the network
            devices, and bit 2 the non-primary-master IDE devices. */
+        if (val & UNPLUG_ALL_IDE_DISKS) {
+            DPRINTF("unplug disks\n");
+            qemu_aio_flush();
+            bdrv_flush_all();
+            pci_unplug_disks(s->pci_dev.bus);
+        }
+        if (val & UNPLUG_ALL_NICS) {
+            DPRINTF("unplug nics\n");
+            pci_unplug_nics(s->pci_dev.bus);
+        }
+        if (val & UNPLUG_AUX_IDE_DISKS) {
+            DPRINTF("unplug auxiliary disks not supported\n");
+        }
         break;
     case 2:
         switch (val) {

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Kevin Wolf]

Am 27.06.2011 17:34, schrieb Stefano Stabellini:
> On Mon, 27 Jun 2011, Kevin Wolf wrote:
>> hw/ide/pci.h is just as internal as internal.h is. And even if you
>> managed to access the same things without any IDE header file, I still
>> think it's not the right level of abstraction because it relies on the
>> implementation details of IDE.
>>
>> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
>> really look right to you to do anywhere outside IDE?
>>
>> I'm basically looking for the same as Michael who wanted to have network
>> unplug handled through qdev, just that the IDE code doesn't support
>> unplug yet.
> 
> I understand.
> 
> I created pci_piix3_xen_ide_init and moved the unplug code to
> hw/ide/piix.c so that we don't have any internal knowledge of IDE code
> in xen_platform.c any more, see below.
> 
> One remaining problem is that the generic pci unplug function is not
> what I need in this case so I have to setup my own; but I hope that in
> general the changes are going in the right direction.

Looks much better to me now. :-)

Just a few comments inline:

> diff --git a/hw/ide.h b/hw/ide.h
> index 34d9394..a490cbb 100644
> --- a/hw/ide.h
> +++ b/hw/ide.h
> @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
>  /* ide-pci.c */
>  void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
>                           int secondary_ide_enabled);
> +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);
>  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index c349644..8ae9ff0 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev)
>      return pci_piix_ide_initfn(d);
>  }
>  
> +static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> +{
> +    PCIDevice *pci_dev;
> +    PCIIDEState *pci_ide;
> +    DriveInfo *di;
> +    int i = 0;
> +
> +    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> +    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
> +
> +    for (; i < 3; i++) {
> +        di = drive_get_by_index(IF_IDE, i); 
> +        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
> +            DeviceState *ds = bdrv_get_attached(di->bdrv);
> +            if (ds)
> +                bdrv_detach(di->bdrv, ds);

Missing braces

> +            bdrv_close(di->bdrv);
> +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> +            drive_put_ref(di);
> +        }
> +    }
> +    qdev_reset_all(&(pci_ide->dev.qdev));
> +    return 0;
> +}
> +
> +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> +{
> +    PCIDevice *dev;
> +
> +    dev = pci_create_simple(bus, devfn, "piix3-ide");
> +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;

Doesn't this modify the piix3-ide definition? Shouldn't matter in
practice because you can't have additional IDE controllers anyway, but
maybe worth a comment stating this.

The other, probably cleaner way of doing it would be adding another
PCIDeviceInfo for xen-ide.

Kevin

Re: [Qemu-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform

[Stefano Stabellini]

On Mon, 27 Jun 2011, Kevin Wolf wrote:
> Am 27.06.2011 17:34, schrieb Stefano Stabellini:
> > On Mon, 27 Jun 2011, Kevin Wolf wrote:
> >> hw/ide/pci.h is just as internal as internal.h is. And even if you
> >> managed to access the same things without any IDE header file, I still
> >> think it's not the right level of abstraction because it relies on the
> >> implementation details of IDE.
> >>
> >> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
> >> really look right to you to do anywhere outside IDE?
> >>
> >> I'm basically looking for the same as Michael who wanted to have network
> >> unplug handled through qdev, just that the IDE code doesn't support
> >> unplug yet.
> > 
> > I understand.
> > 
> > I created pci_piix3_xen_ide_init and moved the unplug code to
> > hw/ide/piix.c so that we don't have any internal knowledge of IDE code
> > in xen_platform.c any more, see below.
> > 
> > One remaining problem is that the generic pci unplug function is not
> > what I need in this case so I have to setup my own; but I hope that in
> > general the changes are going in the right direction.
> 
> Looks much better to me now. :-)

great :)


> Just a few comments inline:
> 
> > diff --git a/hw/ide.h b/hw/ide.h
> > index 34d9394..a490cbb 100644
> > --- a/hw/ide.h
> > +++ b/hw/ide.h
> > @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
> >  /* ide-pci.c */
> >  void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> >                           int secondary_ide_enabled);
> > +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);
> >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index c349644..8ae9ff0 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev)
> >      return pci_piix_ide_initfn(d);
> >  }
> >  
> > +static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > +{
> > +    PCIDevice *pci_dev;
> > +    PCIIDEState *pci_ide;
> > +    DriveInfo *di;
> > +    int i = 0;
> > +
> > +    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> > +    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
> > +
> > +    for (; i < 3; i++) {
> > +        di = drive_get_by_index(IF_IDE, i); 
> > +        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
> > +            DeviceState *ds = bdrv_get_attached(di->bdrv);
> > +            if (ds)
> > +                bdrv_detach(di->bdrv, ds);
> 
> Missing braces

This time I run checkpatch.pl on it.


> 
> > +            bdrv_close(di->bdrv);
> > +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> > +            drive_put_ref(di);
> > +        }
> > +    }
> > +    qdev_reset_all(&(pci_ide->dev.qdev));
> > +    return 0;
> > +}
> > +
> > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
> > +{
> > +    PCIDevice *dev;
> > +
> > +    dev = pci_create_simple(bus, devfn, "piix3-ide");
> > +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
> 
> Doesn't this modify the piix3-ide definition? Shouldn't matter in
> practice because you can't have additional IDE controllers anyway, but
> maybe worth a comment stating this.
> 
> The other, probably cleaner way of doing it would be adding another
> PCIDeviceInfo for xen-ide.
 
I have added piix3-ide-xen, that doesn't set no_hotplug = 1, so now the
code should be more coherent.

I'll send the new version of the patch separately.