| Message ID | 1308240319-13949-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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,
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 >
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 >> >
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
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.
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
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) {