Patchwork [3/3] xen: implement unplug protocol in xen_platform

login
register
mail settings
Submitter Stefano Stabellini
Date June 16, 2011, 4:05 p.m.
Message ID <1308240319-13949-3-git-send-email-stefano.stabellini@eu.citrix.com>
Download mbox | patch
Permalink /patch/100671/
State New
Headers show

Comments

Stefano Stabellini - June 16, 2011, 4:05 p.m.
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(-)
Anthony PERARD - June 17, 2011, 1:46 p.m.
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,
Michael S. Tsirkin - June 20, 2011, 8:47 a.m.
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
>
Kevin Wolf - June 20, 2011, 8:59 a.m.
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
>>
>
Stefano Stabellini - June 23, 2011, 1:11 p.m.
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
Stefano Stabellini - June 23, 2011, 1:16 p.m.
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.
Kevin Wolf - June 27, 2011, 8:26 a.m.
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

Patch

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) {