Stefano Stabellini
2011-Jun-16 16:04 UTC
[Xen-devel] [PATCH 0/3] xen: support PV on HVM guests
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
<stefano.stabellini@eu.citrix.com>
2011-Jun-16 16:05 UTC
[Xen-devel] [PATCH RESEND 1/3] xen: Add the Xen platform pci device
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
<stefano.stabellini@eu.citrix.com>
2011-Jun-16 16:05 UTC
[Xen-devel] [PATCH 2/3] pci: export pci_unplug_device
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
<stefano.stabellini@eu.citrix.com>
2011-Jun-16 16:05 UTC
[Xen-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Anthony PERARD
2011-Jun-17 13:46 UTC
Re: [Xen-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alexander Graf
2011-Jun-20 08:26 UTC
[Xen-devel] Re: [PATCH 2/3] pci: export pci_unplug_device
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> > > pci_unplug_device is needed by the xen_platform device to perfom dynamic > nic unplug.Michael, any comments? I''d like to get an ack from you here. Alex> > 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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alexander Graf
2011-Jun-20 08:28 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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" > +#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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Alexander Graf
2011-Jun-20 08:38 UTC
[Xen-devel] Re: [Qemu-devel] [PATCH 2/3] pci: export pci_unplug_device
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> > > pci_unplug_device is needed by the xen_platform device to perfom dynamic > nic unplug.(resend because the first one got eaten by the mta) Michael, please check if this patch is ok and ack it if so :) Alex> > 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 > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Michael S. Tsirkin
2011-Jun-20 08:47 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Michael S. Tsirkin
2011-Jun-20 08:48 UTC
[Xen-devel] Re: [PATCH 2/3] pci: export pci_unplug_device
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kevin Wolf
2011-Jun-20 08:59 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-23 13:11 UTC
Re: [Xen-devel] [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 --8323329-93637647-1308829410=:12963 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --8323329-93637647-1308829410=:12963--
Stefano Stabellini
2011-Jun-23 13:12 UTC
[Xen-devel] Re: [PATCH 2/3] pci: export pci_unplug_device
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-23 13:16 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Michael S. Tsirkin
2011-Jun-26 13:14 UTC
[Xen-devel] Re: [PATCH RESEND 1/3] xen: Add the Xen platform pci device
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 0x5853Please 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kevin Wolf
2011-Jun-27 08:26 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-27 15:34 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kevin Wolf
2011-Jun-27 15:46 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2011-Jun-28 11:27 UTC
[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
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 bracesThis 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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel