* [Qemu-devel] [PATCH v3] xen-platform: separate unplugging of NVMe disks
@ 2017-03-24 13:40 ` Paul Durrant
0 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2017-03-24 13:40 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Paul Durrant, Anthony Perard
Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
The recent thread on the xen-devel mailing list [1] has highlighted that
this is not desirable behaviour: PV frontends should be able to distinguish
NVMe disks from other types of disk and should have separate control over
whether they are unplugged.
This patch defines a new bit in the unplug mask for this purpose (see Xen
commit [2]) and also tidies up the definitions of, and improves the
comments regarding, the previously exiting bits in the protocol.
[1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
[2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
--
Cc: Anthony Perard <anthony.perard@citrix.com>
v3:
- Updated to reference Xen documentation patch
v2:
- Fix the commit comment
---
hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 6010f35..983d532 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
}
}
-/* Xen Platform, Fixed IOPort */
-#define UNPLUG_ALL_DISKS 1
-#define UNPLUG_ALL_NICS 2
-#define UNPLUG_AUX_IDE_DISKS 4
+/*
+ * Unplug device flags.
+ *
+ * The logic got a little confused at some point in the past but this is
+ * what they do now.
+ *
+ * bit 0: Unplug all IDE and SCSI disks.
+ * bit 1: Unplug all NICs.
+ * bit 2: Unplug IDE disks except primary master. This is overridden if
+ * bit 0 is also present in the mask.
+ * bit 3: Unplug all NVMe disks.
+ *
+ */
+#define _UNPLUG_IDE_SCSI_DISKS 0
+#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
+
+#define _UNPLUG_ALL_NICS 1
+#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
+
+#define _UNPLUG_AUX_IDE_DISKS 2
+#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
+
+#define _UNPLUG_NVME_DISKS 3
+#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
{
@@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
{
uint32_t flags = *(uint32_t *)opaque;
bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
- !(flags & UNPLUG_ALL_DISKS);
+ !(flags & UNPLUG_IDE_SCSI_DISKS);
/* We have to ignore passthrough devices */
if (!strcmp(d->name, "xen-pci-passthrough")) {
@@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
break;
case PCI_CLASS_STORAGE_SCSI:
- case PCI_CLASS_STORAGE_EXPRESS:
if (!aux) {
object_unparent(OBJECT(d));
}
break;
+ case PCI_CLASS_STORAGE_EXPRESS:
+ if (flags & UNPLUG_NVME_DISKS) {
+ object_unparent(OBJECT(d));
+ }
+
default:
break;
}
@@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
switch (addr) {
case 0: {
PCIDevice *pci_dev = PCI_DEVICE(s);
- /* Unplug devices. Value is a bitmask of which devices to
- unplug, with bit 0 the disk devices, bit 1 the network
- devices, and bit 2 the non-primary-master IDE devices. */
- if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
+ /* Unplug devices. See comment above flag definitions */
+ if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
+ UNPLUG_NVME_DISKS)) {
DPRINTF("unplug disks\n");
pci_unplug_disks(pci_dev->bus, val);
}
@@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
* If VMDP was to control both disk and LAN it would use 4.
* If it controlled just disk or just LAN, it would use 8 below.
*/
- pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
+ pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
pci_unplug_nics(pci_dev->bus);
}
break;
case 8:
switch (val) {
case 1:
- pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
+ pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
break;
case 2:
pci_unplug_nics(pci_dev->bus);
--
2.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3] xen-platform: separate unplugging of NVMe disks
@ 2017-03-24 13:40 ` Paul Durrant
0 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2017-03-24 13:40 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Anthony Perard, Paul Durrant
Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
The recent thread on the xen-devel mailing list [1] has highlighted that
this is not desirable behaviour: PV frontends should be able to distinguish
NVMe disks from other types of disk and should have separate control over
whether they are unplugged.
This patch defines a new bit in the unplug mask for this purpose (see Xen
commit [2]) and also tidies up the definitions of, and improves the
comments regarding, the previously exiting bits in the protocol.
[1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
[2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
--
Cc: Anthony Perard <anthony.perard@citrix.com>
v3:
- Updated to reference Xen documentation patch
v2:
- Fix the commit comment
---
hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
1 file changed, 35 insertions(+), 12 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 6010f35..983d532 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
}
}
-/* Xen Platform, Fixed IOPort */
-#define UNPLUG_ALL_DISKS 1
-#define UNPLUG_ALL_NICS 2
-#define UNPLUG_AUX_IDE_DISKS 4
+/*
+ * Unplug device flags.
+ *
+ * The logic got a little confused at some point in the past but this is
+ * what they do now.
+ *
+ * bit 0: Unplug all IDE and SCSI disks.
+ * bit 1: Unplug all NICs.
+ * bit 2: Unplug IDE disks except primary master. This is overridden if
+ * bit 0 is also present in the mask.
+ * bit 3: Unplug all NVMe disks.
+ *
+ */
+#define _UNPLUG_IDE_SCSI_DISKS 0
+#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
+
+#define _UNPLUG_ALL_NICS 1
+#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
+
+#define _UNPLUG_AUX_IDE_DISKS 2
+#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
+
+#define _UNPLUG_NVME_DISKS 3
+#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
{
@@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
{
uint32_t flags = *(uint32_t *)opaque;
bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
- !(flags & UNPLUG_ALL_DISKS);
+ !(flags & UNPLUG_IDE_SCSI_DISKS);
/* We have to ignore passthrough devices */
if (!strcmp(d->name, "xen-pci-passthrough")) {
@@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
break;
case PCI_CLASS_STORAGE_SCSI:
- case PCI_CLASS_STORAGE_EXPRESS:
if (!aux) {
object_unparent(OBJECT(d));
}
break;
+ case PCI_CLASS_STORAGE_EXPRESS:
+ if (flags & UNPLUG_NVME_DISKS) {
+ object_unparent(OBJECT(d));
+ }
+
default:
break;
}
@@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
switch (addr) {
case 0: {
PCIDevice *pci_dev = PCI_DEVICE(s);
- /* Unplug devices. Value is a bitmask of which devices to
- unplug, with bit 0 the disk devices, bit 1 the network
- devices, and bit 2 the non-primary-master IDE devices. */
- if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
+ /* Unplug devices. See comment above flag definitions */
+ if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
+ UNPLUG_NVME_DISKS)) {
DPRINTF("unplug disks\n");
pci_unplug_disks(pci_dev->bus, val);
}
@@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
* If VMDP was to control both disk and LAN it would use 4.
* If it controlled just disk or just LAN, it would use 8 below.
*/
- pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
+ pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
pci_unplug_nics(pci_dev->bus);
}
break;
case 8:
switch (val) {
case 1:
- pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
+ pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
break;
case 2:
pci_unplug_nics(pci_dev->bus);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] xen-platform: separate unplugging of NVMe disks
2017-03-24 13:40 ` Paul Durrant
@ 2017-07-10 14:03 ` Anthony PERARD
-1 siblings, 0 replies; 6+ messages in thread
From: Anthony PERARD @ 2017-07-10 14:03 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel, xen-devel, Paul Durrant
Hi Stefano,
Looks like this patch can be applied.
On Fri, Mar 24, 2017 at 01:40:25PM +0000, Paul Durrant wrote:
> Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
> existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
>
> The recent thread on the xen-devel mailing list [1] has highlighted that
> this is not desirable behaviour: PV frontends should be able to distinguish
> NVMe disks from other types of disk and should have separate control over
> whether they are unplugged.
>
> This patch defines a new bit in the unplug mask for this purpose (see Xen
> commit [2]) and also tidies up the definitions of, and improves the
> comments regarding, the previously exiting bits in the protocol.
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
> [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> --
> Cc: Anthony Perard <anthony.perard@citrix.com>
>
> v3:
> - Updated to reference Xen documentation patch
>
> v2:
> - Fix the commit comment
> ---
> hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> index 6010f35..983d532 100644
> --- a/hw/i386/xen/xen_platform.c
> +++ b/hw/i386/xen/xen_platform.c
> @@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
> }
> }
>
> -/* Xen Platform, Fixed IOPort */
> -#define UNPLUG_ALL_DISKS 1
> -#define UNPLUG_ALL_NICS 2
> -#define UNPLUG_AUX_IDE_DISKS 4
> +/*
> + * Unplug device flags.
> + *
> + * The logic got a little confused at some point in the past but this is
> + * what they do now.
> + *
> + * bit 0: Unplug all IDE and SCSI disks.
> + * bit 1: Unplug all NICs.
> + * bit 2: Unplug IDE disks except primary master. This is overridden if
> + * bit 0 is also present in the mask.
> + * bit 3: Unplug all NVMe disks.
> + *
> + */
> +#define _UNPLUG_IDE_SCSI_DISKS 0
> +#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
> +
> +#define _UNPLUG_ALL_NICS 1
> +#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
> +
> +#define _UNPLUG_AUX_IDE_DISKS 2
> +#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
> +
> +#define _UNPLUG_NVME_DISKS 3
> +#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
>
> static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
> {
> @@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> {
> uint32_t flags = *(uint32_t *)opaque;
> bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
> - !(flags & UNPLUG_ALL_DISKS);
> + !(flags & UNPLUG_IDE_SCSI_DISKS);
>
> /* We have to ignore passthrough devices */
> if (!strcmp(d->name, "xen-pci-passthrough")) {
> @@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> break;
>
> case PCI_CLASS_STORAGE_SCSI:
> - case PCI_CLASS_STORAGE_EXPRESS:
> if (!aux) {
> object_unparent(OBJECT(d));
> }
> break;
>
> + case PCI_CLASS_STORAGE_EXPRESS:
> + if (flags & UNPLUG_NVME_DISKS) {
> + object_unparent(OBJECT(d));
> + }
> +
> default:
> break;
> }
> @@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
> switch (addr) {
> case 0: {
> PCIDevice *pci_dev = PCI_DEVICE(s);
> - /* Unplug devices. Value is a bitmask of which devices to
> - unplug, with bit 0 the disk devices, bit 1 the network
> - devices, and bit 2 the non-primary-master IDE devices. */
> - if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
> + /* Unplug devices. See comment above flag definitions */
> + if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
> + UNPLUG_NVME_DISKS)) {
> DPRINTF("unplug disks\n");
> pci_unplug_disks(pci_dev->bus, val);
> }
> @@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
> * If VMDP was to control both disk and LAN it would use 4.
> * If it controlled just disk or just LAN, it would use 8 below.
> */
> - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> pci_unplug_nics(pci_dev->bus);
> }
> break;
> case 8:
> switch (val) {
> case 1:
> - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> break;
> case 2:
> pci_unplug_nics(pci_dev->bus);
> --
> 2.1.4
>
--
Anthony PERARD
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] xen-platform: separate unplugging of NVMe disks
@ 2017-07-10 14:03 ` Anthony PERARD
0 siblings, 0 replies; 6+ messages in thread
From: Anthony PERARD @ 2017-07-10 14:03 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, Paul Durrant, qemu-devel
Hi Stefano,
Looks like this patch can be applied.
On Fri, Mar 24, 2017 at 01:40:25PM +0000, Paul Durrant wrote:
> Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
> existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
>
> The recent thread on the xen-devel mailing list [1] has highlighted that
> this is not desirable behaviour: PV frontends should be able to distinguish
> NVMe disks from other types of disk and should have separate control over
> whether they are unplugged.
>
> This patch defines a new bit in the unplug mask for this purpose (see Xen
> commit [2]) and also tidies up the definitions of, and improves the
> comments regarding, the previously exiting bits in the protocol.
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
> [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> --
> Cc: Anthony Perard <anthony.perard@citrix.com>
>
> v3:
> - Updated to reference Xen documentation patch
>
> v2:
> - Fix the commit comment
> ---
> hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> index 6010f35..983d532 100644
> --- a/hw/i386/xen/xen_platform.c
> +++ b/hw/i386/xen/xen_platform.c
> @@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
> }
> }
>
> -/* Xen Platform, Fixed IOPort */
> -#define UNPLUG_ALL_DISKS 1
> -#define UNPLUG_ALL_NICS 2
> -#define UNPLUG_AUX_IDE_DISKS 4
> +/*
> + * Unplug device flags.
> + *
> + * The logic got a little confused at some point in the past but this is
> + * what they do now.
> + *
> + * bit 0: Unplug all IDE and SCSI disks.
> + * bit 1: Unplug all NICs.
> + * bit 2: Unplug IDE disks except primary master. This is overridden if
> + * bit 0 is also present in the mask.
> + * bit 3: Unplug all NVMe disks.
> + *
> + */
> +#define _UNPLUG_IDE_SCSI_DISKS 0
> +#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
> +
> +#define _UNPLUG_ALL_NICS 1
> +#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
> +
> +#define _UNPLUG_AUX_IDE_DISKS 2
> +#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
> +
> +#define _UNPLUG_NVME_DISKS 3
> +#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
>
> static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
> {
> @@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> {
> uint32_t flags = *(uint32_t *)opaque;
> bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
> - !(flags & UNPLUG_ALL_DISKS);
> + !(flags & UNPLUG_IDE_SCSI_DISKS);
>
> /* We have to ignore passthrough devices */
> if (!strcmp(d->name, "xen-pci-passthrough")) {
> @@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> break;
>
> case PCI_CLASS_STORAGE_SCSI:
> - case PCI_CLASS_STORAGE_EXPRESS:
> if (!aux) {
> object_unparent(OBJECT(d));
> }
> break;
>
> + case PCI_CLASS_STORAGE_EXPRESS:
> + if (flags & UNPLUG_NVME_DISKS) {
> + object_unparent(OBJECT(d));
> + }
> +
> default:
> break;
> }
> @@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
> switch (addr) {
> case 0: {
> PCIDevice *pci_dev = PCI_DEVICE(s);
> - /* Unplug devices. Value is a bitmask of which devices to
> - unplug, with bit 0 the disk devices, bit 1 the network
> - devices, and bit 2 the non-primary-master IDE devices. */
> - if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
> + /* Unplug devices. See comment above flag definitions */
> + if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
> + UNPLUG_NVME_DISKS)) {
> DPRINTF("unplug disks\n");
> pci_unplug_disks(pci_dev->bus, val);
> }
> @@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
> * If VMDP was to control both disk and LAN it would use 4.
> * If it controlled just disk or just LAN, it would use 8 below.
> */
> - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> pci_unplug_nics(pci_dev->bus);
> }
> break;
> case 8:
> switch (val) {
> case 1:
> - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> break;
> case 2:
> pci_unplug_nics(pci_dev->bus);
> --
> 2.1.4
>
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v3] xen-platform: separate unplugging of NVMe disks
2017-07-10 14:03 ` Anthony PERARD
@ 2017-07-12 0:29 ` Stefano Stabellini
-1 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2017-07-12 0:29 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Stefano Stabellini, qemu-devel, xen-devel, Paul Durrant
On Mon, 10 Jul 2017, Anthony PERARD wrote:
> Hi Stefano,
>
> Looks like this patch can be applied.
Thank you for pointing it out to me, because this email has
inexplicably disappeared from my inbox. I have now marked it
appropriately to be committed.
>
> On Fri, Mar 24, 2017 at 01:40:25PM +0000, Paul Durrant wrote:
> > Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
> > existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
> >
> > The recent thread on the xen-devel mailing list [1] has highlighted that
> > this is not desirable behaviour: PV frontends should be able to distinguish
> > NVMe disks from other types of disk and should have separate control over
> > whether they are unplugged.
> >
> > This patch defines a new bit in the unplug mask for this purpose (see Xen
> > commit [2]) and also tidies up the definitions of, and improves the
> > comments regarding, the previously exiting bits in the protocol.
> >
> > [1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
> > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > --
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> >
> > v3:
> > - Updated to reference Xen documentation patch
> >
> > v2:
> > - Fix the commit comment
> > ---
> > hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 35 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> > index 6010f35..983d532 100644
> > --- a/hw/i386/xen/xen_platform.c
> > +++ b/hw/i386/xen/xen_platform.c
> > @@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
> > }
> > }
> >
> > -/* Xen Platform, Fixed IOPort */
> > -#define UNPLUG_ALL_DISKS 1
> > -#define UNPLUG_ALL_NICS 2
> > -#define UNPLUG_AUX_IDE_DISKS 4
> > +/*
> > + * Unplug device flags.
> > + *
> > + * The logic got a little confused at some point in the past but this is
> > + * what they do now.
> > + *
> > + * bit 0: Unplug all IDE and SCSI disks.
> > + * bit 1: Unplug all NICs.
> > + * bit 2: Unplug IDE disks except primary master. This is overridden if
> > + * bit 0 is also present in the mask.
> > + * bit 3: Unplug all NVMe disks.
> > + *
> > + */
> > +#define _UNPLUG_IDE_SCSI_DISKS 0
> > +#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
> > +
> > +#define _UNPLUG_ALL_NICS 1
> > +#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
> > +
> > +#define _UNPLUG_AUX_IDE_DISKS 2
> > +#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
> > +
> > +#define _UNPLUG_NVME_DISKS 3
> > +#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
> >
> > static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
> > {
> > @@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> > {
> > uint32_t flags = *(uint32_t *)opaque;
> > bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
> > - !(flags & UNPLUG_ALL_DISKS);
> > + !(flags & UNPLUG_IDE_SCSI_DISKS);
> >
> > /* We have to ignore passthrough devices */
> > if (!strcmp(d->name, "xen-pci-passthrough")) {
> > @@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> > break;
> >
> > case PCI_CLASS_STORAGE_SCSI:
> > - case PCI_CLASS_STORAGE_EXPRESS:
> > if (!aux) {
> > object_unparent(OBJECT(d));
> > }
> > break;
> >
> > + case PCI_CLASS_STORAGE_EXPRESS:
> > + if (flags & UNPLUG_NVME_DISKS) {
> > + object_unparent(OBJECT(d));
> > + }
> > +
> > default:
> > break;
> > }
> > @@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
> > switch (addr) {
> > case 0: {
> > PCIDevice *pci_dev = PCI_DEVICE(s);
> > - /* Unplug devices. Value is a bitmask of which devices to
> > - unplug, with bit 0 the disk devices, bit 1 the network
> > - devices, and bit 2 the non-primary-master IDE devices. */
> > - if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
> > + /* Unplug devices. See comment above flag definitions */
> > + if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
> > + UNPLUG_NVME_DISKS)) {
> > DPRINTF("unplug disks\n");
> > pci_unplug_disks(pci_dev->bus, val);
> > }
> > @@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
> > * If VMDP was to control both disk and LAN it would use 4.
> > * If it controlled just disk or just LAN, it would use 8 below.
> > */
> > - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> > + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> > pci_unplug_nics(pci_dev->bus);
> > }
> > break;
> > case 8:
> > switch (val) {
> > case 1:
> > - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> > + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> > break;
> > case 2:
> > pci_unplug_nics(pci_dev->bus);
> > --
> > 2.1.4
> >
>
> --
> Anthony PERARD
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] xen-platform: separate unplugging of NVMe disks
@ 2017-07-12 0:29 ` Stefano Stabellini
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Stabellini @ 2017-07-12 0:29 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel, Stefano Stabellini, qemu-devel, Paul Durrant
On Mon, 10 Jul 2017, Anthony PERARD wrote:
> Hi Stefano,
>
> Looks like this patch can be applied.
Thank you for pointing it out to me, because this email has
inexplicably disappeared from my inbox. I have now marked it
appropriately to be committed.
>
> On Fri, Mar 24, 2017 at 01:40:25PM +0000, Paul Durrant wrote:
> > Commit 090fa1c8 "add support for unplugging NVMe disks..." extended the
> > existing disk unplug flag to cover NVMe disks as well as IDE and SCSI.
> >
> > The recent thread on the xen-devel mailing list [1] has highlighted that
> > this is not desirable behaviour: PV frontends should be able to distinguish
> > NVMe disks from other types of disk and should have separate control over
> > whether they are unplugged.
> >
> > This patch defines a new bit in the unplug mask for this purpose (see Xen
> > commit [2]) and also tidies up the definitions of, and improves the
> > comments regarding, the previously exiting bits in the protocol.
> >
> > [1] https://lists.xen.org/archives/html/xen-devel/2017-03/msg02924.html
> > [2] http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=1096aa02
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > --
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> >
> > v3:
> > - Updated to reference Xen documentation patch
> >
> > v2:
> > - Fix the commit comment
> > ---
> > hw/i386/xen/xen_platform.c | 47 ++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 35 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
> > index 6010f35..983d532 100644
> > --- a/hw/i386/xen/xen_platform.c
> > +++ b/hw/i386/xen/xen_platform.c
> > @@ -87,10 +87,30 @@ static void log_writeb(PCIXenPlatformState *s, char val)
> > }
> > }
> >
> > -/* Xen Platform, Fixed IOPort */
> > -#define UNPLUG_ALL_DISKS 1
> > -#define UNPLUG_ALL_NICS 2
> > -#define UNPLUG_AUX_IDE_DISKS 4
> > +/*
> > + * Unplug device flags.
> > + *
> > + * The logic got a little confused at some point in the past but this is
> > + * what they do now.
> > + *
> > + * bit 0: Unplug all IDE and SCSI disks.
> > + * bit 1: Unplug all NICs.
> > + * bit 2: Unplug IDE disks except primary master. This is overridden if
> > + * bit 0 is also present in the mask.
> > + * bit 3: Unplug all NVMe disks.
> > + *
> > + */
> > +#define _UNPLUG_IDE_SCSI_DISKS 0
> > +#define UNPLUG_IDE_SCSI_DISKS (1u << _UNPLUG_IDE_SCSI_DISKS)
> > +
> > +#define _UNPLUG_ALL_NICS 1
> > +#define UNPLUG_ALL_NICS (1u << _UNPLUG_ALL_NICS)
> > +
> > +#define _UNPLUG_AUX_IDE_DISKS 2
> > +#define UNPLUG_AUX_IDE_DISKS (1u << _UNPLUG_AUX_IDE_DISKS)
> > +
> > +#define _UNPLUG_NVME_DISKS 3
> > +#define UNPLUG_NVME_DISKS (1u << _UNPLUG_NVME_DISKS)
> >
> > static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
> > {
> > @@ -111,7 +131,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> > {
> > uint32_t flags = *(uint32_t *)opaque;
> > bool aux = (flags & UNPLUG_AUX_IDE_DISKS) &&
> > - !(flags & UNPLUG_ALL_DISKS);
> > + !(flags & UNPLUG_IDE_SCSI_DISKS);
> >
> > /* We have to ignore passthrough devices */
> > if (!strcmp(d->name, "xen-pci-passthrough")) {
> > @@ -124,12 +144,16 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
> > break;
> >
> > case PCI_CLASS_STORAGE_SCSI:
> > - case PCI_CLASS_STORAGE_EXPRESS:
> > if (!aux) {
> > object_unparent(OBJECT(d));
> > }
> > break;
> >
> > + case PCI_CLASS_STORAGE_EXPRESS:
> > + if (flags & UNPLUG_NVME_DISKS) {
> > + object_unparent(OBJECT(d));
> > + }
> > +
> > default:
> > break;
> > }
> > @@ -147,10 +171,9 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
> > switch (addr) {
> > case 0: {
> > PCIDevice *pci_dev = PCI_DEVICE(s);
> > - /* Unplug devices. Value is a bitmask of which devices to
> > - unplug, with bit 0 the disk devices, bit 1 the network
> > - devices, and bit 2 the non-primary-master IDE devices. */
> > - if (val & (UNPLUG_ALL_DISKS | UNPLUG_AUX_IDE_DISKS)) {
> > + /* Unplug devices. See comment above flag definitions */
> > + if (val & (UNPLUG_IDE_SCSI_DISKS | UNPLUG_AUX_IDE_DISKS |
> > + UNPLUG_NVME_DISKS)) {
> > DPRINTF("unplug disks\n");
> > pci_unplug_disks(pci_dev->bus, val);
> > }
> > @@ -338,14 +361,14 @@ static void xen_platform_ioport_writeb(void *opaque, hwaddr addr,
> > * If VMDP was to control both disk and LAN it would use 4.
> > * If it controlled just disk or just LAN, it would use 8 below.
> > */
> > - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> > + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> > pci_unplug_nics(pci_dev->bus);
> > }
> > break;
> > case 8:
> > switch (val) {
> > case 1:
> > - pci_unplug_disks(pci_dev->bus, UNPLUG_ALL_DISKS);
> > + pci_unplug_disks(pci_dev->bus, UNPLUG_IDE_SCSI_DISKS);
> > break;
> > case 2:
> > pci_unplug_nics(pci_dev->bus);
> > --
> > 2.1.4
> >
>
> --
> Anthony PERARD
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-12 0:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-24 13:40 [Qemu-devel] [PATCH v3] xen-platform: separate unplugging of NVMe disks Paul Durrant
2017-03-24 13:40 ` Paul Durrant
2017-07-10 14:03 ` [Qemu-devel] " Anthony PERARD
2017-07-10 14:03 ` Anthony PERARD
2017-07-12 0:29 ` [Qemu-devel] " Stefano Stabellini
2017-07-12 0:29 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.