[RFC] virtio-mmio: Update the device to OASIS spec version

[RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

This patch add a support for second version of the virtio-mmio device,
which follows OASIS "Virtual I/O Device (VIRTIO) Version 1.0"
specification.

Main changes:

1. The control register symbolic names use the new device/driver
   nomenclature rather than the old guest/host one.

2. The driver detect the device version (version 1 is the pre-OASIS
   spec, version 2 is compatible with fist revision of the OASIS spec)
   and drives the device accordingly.

3. New version uses direct addressing (64 bit address split into two
   low/high register) instead of the guest page size based one,
   and addresses each part of the queue (descriptors, available, used)
   separately.

4. The device activity is now explicitly triggered by writing to the
   "queue ready" register.

5. The platform device got a sysfs attribute with the version number.

6. Whole 64 bit features are properly handled now (both ways).

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
I had the code typed for months now, but finally (just before
disappearing for the end-of-year break) got time to test it (and
fix the bugs), so I though I'd share it at least as RFC.

It's based on Linus tree still in merge window, but as far as I can
see all virtio changes have been already pulled, so I don't expect
any changes in rc1.

Tested with our custom models (*not* qemu).

Regards and till next year!

Pawel

 drivers/virtio/virtio_mmio.c | 132 +++++++++++++++++++++++++++----------------
 include/linux/virtio_mmio.h  |  46 +++++++++++----
 2 files changed, 120 insertions(+), 58 deletions(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 00d115b..d60675a 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -1,7 +1,7 @@
 /*
  * Virtio memory mapped device driver
  *
- * Copyright 2011, ARM Ltd.
+ * Copyright 2011-2014, ARM Ltd.
  *
  * This module allows virtio devices to be used over a virtual, memory mapped
  * platform device.
@@ -50,36 +50,6 @@
  *
  *
  *
- * Registers layout (all 32-bit wide):
- *
- * offset d. name             description
- * ------ -- ---------------- -----------------
- *
- * 0x000  R  MagicValue       Magic value "virt"
- * 0x004  R  Version          Device version (current max. 1)
- * 0x008  R  DeviceID         Virtio device ID
- * 0x00c  R  VendorID         Virtio vendor ID
- *
- * 0x010  R  HostFeatures     Features supported by the host
- * 0x014  W  HostFeaturesSel  Set of host features to access via HostFeatures
- *
- * 0x020  W  GuestFeatures    Features activated by the guest
- * 0x024  W  GuestFeaturesSel Set of activated features to set via GuestFeatures
- * 0x028  W  GuestPageSize    Size of guest's memory page in bytes
- *
- * 0x030  W  QueueSel         Queue selector
- * 0x034  R  QueueNumMax      Maximum size of the currently selected queue
- * 0x038  W  QueueNum         Queue size for the currently selected queue
- * 0x03c  W  QueueAlign       Used Ring alignment for the current queue
- * 0x040  RW QueuePFN         PFN for the currently selected queue
- *
- * 0x050  W  QueueNotify      Queue notifier
- * 0x060  R  InterruptStatus  Interrupt status register
- * 0x064  W  InterruptACK     Interrupt acknowledge register
- * 0x070  RW Status           Device status register
- *
- * 0x100+ RW                  Device-specific configuration space
- *
  * Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
@@ -145,11 +115,16 @@ struct virtio_mmio_vq_info {
 static u64 vm_get_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	u64 features;
+
+	writel(1, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+	features = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
+	features <<= 32;
 
-	/* TODO: Features > 32 bits */
-	writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
+	writel(0, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
+	features |= readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
 
-	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
+	return features;
 }
 
 static int vm_finalize_features(struct virtio_device *vdev)
@@ -159,11 +134,13 @@ static int vm_finalize_features(struct virtio_device *vdev)
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	/* Make sure we don't have any features > 32 bits! */
-	BUG_ON((u32)vdev->features != vdev->features);
+	writel(1, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+	writel((vdev->features >> 32) & 0xffffffff,
+			vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
 
-	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
-	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
+	writel(0, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
+	writel(vdev->features & 0xffffffff,
+			vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
 
 	return 0;
 }
@@ -275,7 +252,12 @@ static void vm_del_vq(struct virtqueue *vq)
 
 	/* Select and deactivate the queue */
 	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
-	writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	if (vm_dev->version == 1) {
+		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	} else {
+		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+		WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
+	}
 
 	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_MMIO_VRING_ALIGN));
 	free_pages_exact(info->queue, size);
@@ -312,7 +294,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
 
 	/* Queue shouldn't already be set up. */
-	if (readl(vm_dev->base + VIRTIO_MMIO_QUEUE_PFN)) {
+	if (readl(vm_dev->base + (vm_dev->version == 1 ?
+			VIRTIO_MMIO_QUEUE_PFN : VIRTIO_MMIO_QUEUE_READY))) {
 		err = -ENOENT;
 		goto error_available;
 	}
@@ -358,10 +341,35 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 
 	/* Activate the queue */
 	writel(info->num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
-	writel(VIRTIO_MMIO_VRING_ALIGN,
-			vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
-	writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
-			vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	if (vm_dev->version == 1) {
+		writel(VIRTIO_MMIO_VRING_ALIGN,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
+		writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	} else {
+		uint64_t addr = virt_to_phys(info->queue);
+
+		writel(addr & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
+		writel((addr >> 32) & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
+
+		addr += info->num * sizeof(struct vring_desc);
+		writel(addr & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
+		writel((addr >> 32) & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
+
+		addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
+		addr += VIRTIO_MMIO_VRING_ALIGN - 1;
+		addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);
+		writel(addr & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
+		writel((addr >> 32) & 0xffffffff,
+				vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
+
+		writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+	}
 
 	/* Create the vring */
 	vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
@@ -381,7 +389,12 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 error_new_virtqueue:
-	writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	if (vm_dev->version == 1) {
+		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
+	} else {
+		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
+		WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
+	}
 	free_pages_exact(info->queue, size);
 error_alloc_pages:
 	kfree(info);
@@ -439,6 +452,18 @@ static const struct virtio_config_ops virtio_mmio_config_ops = {
 
 /* Platform device */
 
+static ssize_t vm_dev_attr_version_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+
+	return snprintf(buf, PAGE_SIZE, "%lu", vm_dev->version);
+}
+
+static struct device_attribute vm_dev_attr_version =
+		__ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
+
 static int virtio_mmio_probe(struct platform_device *pdev)
 {
 	struct virtio_mmio_device *vm_dev;
@@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 
 	/* Check device version */
 	vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
-	if (vm_dev->version != 1) {
+	if (vm_dev->version < 1 || vm_dev->version > 2) {
 		dev_err(&pdev->dev, "Version %ld not supported!\n",
 				vm_dev->version);
 		return -ENXIO;
 	}
 
 	vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
+	if (vm_dev->vdev.id.device == 0) {
+		/*
+		 * ID 0 means a dummy (placeholder) device, skip quietly
+		 * (as in: no error) with no further actions
+		 */
+		return 0;
+	}
 	vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
 
-	writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+	if (vm_dev->version == 1)
+		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
+
+	device_create_file(&pdev->dev, &vm_dev_attr_version);
 
 	platform_set_drvdata(pdev, vm_dev);
 
@@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
 {
 	struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
 
-	unregister_virtio_device(&vm_dev->vdev);
+	if (vm_dev)
+		unregister_virtio_device(&vm_dev->vdev);
 
 	return 0;
 }
diff --git a/include/linux/virtio_mmio.h b/include/linux/virtio_mmio.h
index 5c7b6f0..d5f3634 100644
--- a/include/linux/virtio_mmio.h
+++ b/include/linux/virtio_mmio.h
@@ -51,21 +51,22 @@
 /* Virtio vendor ID - Read Only */
 #define VIRTIO_MMIO_VENDOR_ID		0x00c
 
-/* Bitmask of the features supported by the host
+/* Bitmask of the features supported by the device (host)
  * (32 bits per set) - Read Only */
-#define VIRTIO_MMIO_HOST_FEATURES	0x010
+#define VIRTIO_MMIO_DEVICE_FEATURES	0x010
 
-/* Host features set selector - Write Only */
-#define VIRTIO_MMIO_HOST_FEATURES_SEL	0x014
+/* Device (host) features set selector - Write Only */
+#define VIRTIO_MMIO_DEVICE_FEATURES_SEL	0x014
 
-/* Bitmask of features activated by the guest
+/* Bitmask of features activated by the driver (guest)
  * (32 bits per set) - Write Only */
-#define VIRTIO_MMIO_GUEST_FEATURES	0x020
+#define VIRTIO_MMIO_DRIVER_FEATURES	0x020
 
 /* Activated features set selector - Write Only */
-#define VIRTIO_MMIO_GUEST_FEATURES_SEL	0x024
+#define VIRTIO_MMIO_DRIVER_FEATURES_SEL	0x024
 
-/* Guest's memory page size in bytes - Write Only */
+/* Guest's memory page size in bytes - Write Only
+ * LEGACY DEVICES ONLY! */
 #define VIRTIO_MMIO_GUEST_PAGE_SIZE	0x028
 
 /* Queue selector - Write Only */
@@ -77,12 +78,18 @@
 /* Queue size for the currently selected queue - Write Only */
 #define VIRTIO_MMIO_QUEUE_NUM		0x038
 
-/* Used Ring alignment for the currently selected queue - Write Only */
+/* Used Ring alignment for the currently selected queue - Write Only
+ * LEGACY DEVICES ONLY! */
 #define VIRTIO_MMIO_QUEUE_ALIGN		0x03c
 
-/* Guest's PFN for the currently selected queue - Read Write */
+/* Guest's PFN for the currently selected queue - Read Write
+ * LEGACY DEVICES ONLY! */
 #define VIRTIO_MMIO_QUEUE_PFN		0x040
 
+/* Ready bit for the currently selected queue - Read Write
+ * NOT FOR LEGACY DEVICES! */
+#define VIRTIO_MMIO_QUEUE_READY		0x044
+
 /* Queue notifier - Write Only */
 #define VIRTIO_MMIO_QUEUE_NOTIFY	0x050
 
@@ -95,6 +102,25 @@
 /* Device status register - Read Write */
 #define VIRTIO_MMIO_STATUS		0x070
 
+/* Selected queue's Descriptor Table address, 64 bits in two halves
+ * NOT FOR LEGACY DEVICES! */
+#define VIRTIO_MMIO_QUEUE_DESC_LOW	0x080
+#define VIRTIO_MMIO_QUEUE_DESC_HIGH	0x084
+
+/* Selected queue's Available Ring address, 64 bits in two halves
+ * NOT FOR LEGACY DEVICES! */
+#define VIRTIO_MMIO_QUEUE_AVAIL_LOW	0x090
+#define VIRTIO_MMIO_QUEUE_AVAIL_HIGH	0x094
+
+/* Selected queue's Used Ring address, 64 bits in two halves
+ * NOT FOR LEGACY DEVICES! */
+#define VIRTIO_MMIO_QUEUE_USED_LOW	0x0a0
+#define VIRTIO_MMIO_QUEUE_USED_HIGH	0x0a4
+
+/* Configuration atomicity value
+ * NOT FOR LEGACY DEVICES! */
+#define VIRTIO_MMIO_CONFIG_GENERATION	0x0fc
+
 /* The config space is defined by each driver as
  * the per-driver configuration space - Read Write */
 #define VIRTIO_MMIO_CONFIG		0x100
-- 
2.1.0

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Fri, Dec 19, 2014 at 06:38:04PM +0000, Pawel Moll wrote:
> This patch add a support for second version of the virtio-mmio device,
> which follows OASIS "Virtual I/O Device (VIRTIO) Version 1.0"
> specification.
> 
> Main changes:
> 
> 1. The control register symbolic names use the new device/driver
>    nomenclature rather than the old guest/host one.
> 
> 2. The driver detect the device version (version 1 is the pre-OASIS
>    spec, version 2 is compatible with fist revision of the OASIS spec)
>    and drives the device accordingly.
> 
> 3. New version uses direct addressing (64 bit address split into two
>    low/high register) instead of the guest page size based one,
>    and addresses each part of the queue (descriptors, available, used)
>    separately.
> 
> 4. The device activity is now explicitly triggered by writing to the
>    "queue ready" register.
> 
> 5. The platform device got a sysfs attribute with the version number.
> 
> 6. Whole 64 bit features are properly handled now (both ways).
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> I had the code typed for months now, but finally (just before
> disappearing for the end-of-year break) got time to test it (and
> fix the bugs), so I though I'd share it at least as RFC.
> 
> It's based on Linus tree still in merge window, but as far as I can
> see all virtio changes have been already pulled, so I don't expect
> any changes in rc1.
> 
> Tested with our custom models (*not* qemu).
> 
> Regards and till next year!
> 
> Pawel
> 
>  drivers/virtio/virtio_mmio.c | 132 +++++++++++++++++++++++++++----------------
>  include/linux/virtio_mmio.h  |  46 +++++++++++----
>  2 files changed, 120 insertions(+), 58 deletions(-)

Thanks!  Looks good overall. Some comments below.


> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 00d115b..d60675a 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -1,7 +1,7 @@
>  /*
>   * Virtio memory mapped device driver
>   *
> - * Copyright 2011, ARM Ltd.
> + * Copyright 2011-2014, ARM Ltd.
>   *
>   * This module allows virtio devices to be used over a virtual, memory mapped
>   * platform device.
> @@ -50,36 +50,6 @@
>   *
>   *
>   *
> - * Registers layout (all 32-bit wide):
> - *
> - * offset d. name             description
> - * ------ -- ---------------- -----------------
> - *
> - * 0x000  R  MagicValue       Magic value "virt"
> - * 0x004  R  Version          Device version (current max. 1)
> - * 0x008  R  DeviceID         Virtio device ID
> - * 0x00c  R  VendorID         Virtio vendor ID
> - *
> - * 0x010  R  HostFeatures     Features supported by the host
> - * 0x014  W  HostFeaturesSel  Set of host features to access via HostFeatures
> - *
> - * 0x020  W  GuestFeatures    Features activated by the guest
> - * 0x024  W  GuestFeaturesSel Set of activated features to set via GuestFeatures
> - * 0x028  W  GuestPageSize    Size of guest's memory page in bytes
> - *
> - * 0x030  W  QueueSel         Queue selector
> - * 0x034  R  QueueNumMax      Maximum size of the currently selected queue
> - * 0x038  W  QueueNum         Queue size for the currently selected queue
> - * 0x03c  W  QueueAlign       Used Ring alignment for the current queue
> - * 0x040  RW QueuePFN         PFN for the currently selected queue
> - *
> - * 0x050  W  QueueNotify      Queue notifier
> - * 0x060  R  InterruptStatus  Interrupt status register
> - * 0x064  W  InterruptACK     Interrupt acknowledge register
> - * 0x070  RW Status           Device status register
> - *
> - * 0x100+ RW                  Device-specific configuration space
> - *
>   * Based on Virtio PCI driver by Anthony Liguori, copyright IBM Corp. 2007
>   *
>   * This work is licensed under the terms of the GNU GPL, version 2 or later.
> @@ -145,11 +115,16 @@ struct virtio_mmio_vq_info {
>  static u64 vm_get_features(struct virtio_device *vdev)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +	u64 features;
> +
> +	writel(1, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
> +	features = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
> +	features <<= 32;
>  
> -	/* TODO: Features > 32 bits */
> -	writel(0, vm_dev->base + VIRTIO_MMIO_HOST_FEATURES_SEL);
> +	writel(0, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
> +	features |= readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
>  
> -	return readl(vm_dev->base + VIRTIO_MMIO_HOST_FEATURES);
> +	return features;
>  }
>  
>  static int vm_finalize_features(struct virtio_device *vdev)
> @@ -159,11 +134,13 @@ static int vm_finalize_features(struct virtio_device *vdev)
>  	/* Give virtio_ring a chance to accept features. */
>  	vring_transport_features(vdev);
>  
> -	/* Make sure we don't have any features > 32 bits! */
> -	BUG_ON((u32)vdev->features != vdev->features);
> +	writel(1, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
> +	writel((vdev->features >> 32) & 0xffffffff,
> +			vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
>  
> -	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
> -	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
> +	writel(0, vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES_SEL);
> +	writel(vdev->features & 0xffffffff,
> +			vm_dev->base + VIRTIO_MMIO_DRIVER_FEATURES);
>  
>  	return 0;
>  }
> @@ -275,7 +252,12 @@ static void vm_del_vq(struct virtqueue *vq)
>  
>  	/* Select and deactivate the queue */
>  	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
> -	writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	if (vm_dev->version == 1) {
> +		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	} else {
> +		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> +		WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
> +	}
>  
>  	size = PAGE_ALIGN(vring_size(info->num, VIRTIO_MMIO_VRING_ALIGN));
>  	free_pages_exact(info->queue, size);
> @@ -312,7 +294,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
>  	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
>  
>  	/* Queue shouldn't already be set up. */
> -	if (readl(vm_dev->base + VIRTIO_MMIO_QUEUE_PFN)) {
> +	if (readl(vm_dev->base + (vm_dev->version == 1 ?
> +			VIRTIO_MMIO_QUEUE_PFN : VIRTIO_MMIO_QUEUE_READY))) {
>  		err = -ENOENT;
>  		goto error_available;
>  	}
> @@ -358,10 +341,35 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
>  
>  	/* Activate the queue */
>  	writel(info->num, vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
> -	writel(VIRTIO_MMIO_VRING_ALIGN,
> -			vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
> -	writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
> -			vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	if (vm_dev->version == 1) {
> +		writel(VIRTIO_MMIO_VRING_ALIGN,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN);
> +		writel(virt_to_phys(info->queue) >> PAGE_SHIFT,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	} else {
> +		uint64_t addr = virt_to_phys(info->queue);

Kernel normally uses u64 for this type.

> +
> +		writel(addr & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> +		writel((addr >> 32) & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> +
> +		addr += info->num * sizeof(struct vring_desc);
> +		writel(addr & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> +		writel((addr >> 32) & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);

0xffffffff isn't really needed, is it?

> +
> +		addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
> +		addr += VIRTIO_MMIO_VRING_ALIGN - 1;
> +		addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);


Host no longer knows the alignment, so why is it needed?
In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong:
it corresponds to legacy devices, and it does not
say what "align" is.

I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
it's a legacy thing.




> +		writel(addr & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> +		writel((addr >> 32) & 0xffffffff,
> +				vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> +
> +		writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> +	}
>  
>  	/* Create the vring */
>  	vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> @@ -381,7 +389,12 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
>  	return vq;
>  
>  error_new_virtqueue:
> -	writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	if (vm_dev->version == 1) {
> +		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
> +	} else {
> +		writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> +		WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
> +	}
>  	free_pages_exact(info->queue, size);
>  error_alloc_pages:
>  	kfree(info);
> @@ -439,6 +452,18 @@ static const struct virtio_config_ops virtio_mmio_config_ops = {
>  
>  /* Platform device */
>  
> +static ssize_t vm_dev_attr_version_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%lu", vm_dev->version);
> +}
> +
> +static struct device_attribute vm_dev_attr_version =
> +		__ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> +
>  static int virtio_mmio_probe(struct platform_device *pdev)
>  {
>  	struct virtio_mmio_device *vm_dev;

We already expose feature bits - this one really necessary?

> @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>  
>  	/* Check device version */
>  	vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> -	if (vm_dev->version != 1) {
> +	if (vm_dev->version < 1 || vm_dev->version > 2) {
>  		dev_err(&pdev->dev, "Version %ld not supported!\n",
>  				vm_dev->version);
>  		return -ENXIO;
>  	}
>  
>  	vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> +	if (vm_dev->vdev.id.device == 0) {
> +		/*
> +		 * ID 0 means a dummy (placeholder) device, skip quietly
> +		 * (as in: no error) with no further actions
> +		 */
> +		return 0;

Necessary?
We don't have drivers for this id anyway.

> +	}

Need to also
	1. validate that feature bit VIRTIO_1 is set
	2. validate that ID is not for a legacy device

otherwise device specific drivers might get invoked
on future devices (e.g. when we update balloon for 1.0)
and they not do the right thing.


>  	vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
>  
> -	writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> +	if (vm_dev->version == 1)
> +		writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_GUEST_PAGE_SIZE);
> +
> +	device_create_file(&pdev->dev, &vm_dev_attr_version);
>  
>  	platform_set_drvdata(pdev, vm_dev);
>  
> @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
>  {
>  	struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
>  
> -	unregister_virtio_device(&vm_dev->vdev);
> +	if (vm_dev)
> +		unregister_virtio_device(&vm_dev->vdev);
>  

Will remove ever be called if probe fails?

>  	return 0;
>  }
> diff --git a/include/linux/virtio_mmio.h b/include/linux/virtio_mmio.h
> index 5c7b6f0..d5f3634 100644
> --- a/include/linux/virtio_mmio.h
> +++ b/include/linux/virtio_mmio.h
> @@ -51,21 +51,22 @@
>  /* Virtio vendor ID - Read Only */
>  #define VIRTIO_MMIO_VENDOR_ID		0x00c
>  
> -/* Bitmask of the features supported by the host
> +/* Bitmask of the features supported by the device (host)
>   * (32 bits per set) - Read Only */
> -#define VIRTIO_MMIO_HOST_FEATURES	0x010
> +#define VIRTIO_MMIO_DEVICE_FEATURES	0x010
>  
> -/* Host features set selector - Write Only */
> -#define VIRTIO_MMIO_HOST_FEATURES_SEL	0x014
> +/* Device (host) features set selector - Write Only */
> +#define VIRTIO_MMIO_DEVICE_FEATURES_SEL	0x014
>  
> -/* Bitmask of features activated by the guest
> +/* Bitmask of features activated by the driver (guest)
>   * (32 bits per set) - Write Only */
> -#define VIRTIO_MMIO_GUEST_FEATURES	0x020
> +#define VIRTIO_MMIO_DRIVER_FEATURES	0x020
>  
>  /* Activated features set selector - Write Only */
> -#define VIRTIO_MMIO_GUEST_FEATURES_SEL	0x024
> +#define VIRTIO_MMIO_DRIVER_FEATURES_SEL	0x024
>  
> -/* Guest's memory page size in bytes - Write Only */
> +/* Guest's memory page size in bytes - Write Only
> + * LEGACY DEVICES ONLY! */

This is not the preferred style for multi-line comments :)
Also - maybe add a flag to selectively disable legacy
or modern macros?
Might be clearer than comments that, after all, never compile.

>  #define VIRTIO_MMIO_GUEST_PAGE_SIZE	0x028
>  
>  /* Queue selector - Write Only */
> @@ -77,12 +78,18 @@
>  /* Queue size for the currently selected queue - Write Only */
>  #define VIRTIO_MMIO_QUEUE_NUM		0x038
>  
> -/* Used Ring alignment for the currently selected queue - Write Only */
> +/* Used Ring alignment for the currently selected queue - Write Only
> + * LEGACY DEVICES ONLY! */
>  #define VIRTIO_MMIO_QUEUE_ALIGN		0x03c
>  
> -/* Guest's PFN for the currently selected queue - Read Write */
> +/* Guest's PFN for the currently selected queue - Read Write
> + * LEGACY DEVICES ONLY! */
>  #define VIRTIO_MMIO_QUEUE_PFN		0x040
>  
> +/* Ready bit for the currently selected queue - Read Write
> + * NOT FOR LEGACY DEVICES! */
> +#define VIRTIO_MMIO_QUEUE_READY		0x044
> +
>  /* Queue notifier - Write Only */
>  #define VIRTIO_MMIO_QUEUE_NOTIFY	0x050
>  
> @@ -95,6 +102,25 @@
>  /* Device status register - Read Write */
>  #define VIRTIO_MMIO_STATUS		0x070
>  
> +/* Selected queue's Descriptor Table address, 64 bits in two halves
> + * NOT FOR LEGACY DEVICES! */
> +#define VIRTIO_MMIO_QUEUE_DESC_LOW	0x080
> +#define VIRTIO_MMIO_QUEUE_DESC_HIGH	0x084
> +
> +/* Selected queue's Available Ring address, 64 bits in two halves
> + * NOT FOR LEGACY DEVICES! */
> +#define VIRTIO_MMIO_QUEUE_AVAIL_LOW	0x090
> +#define VIRTIO_MMIO_QUEUE_AVAIL_HIGH	0x094
> +
> +/* Selected queue's Used Ring address, 64 bits in two halves
> + * NOT FOR LEGACY DEVICES! */
> +#define VIRTIO_MMIO_QUEUE_USED_LOW	0x0a0
> +#define VIRTIO_MMIO_QUEUE_USED_HIGH	0x0a4
> +
> +/* Configuration atomicity value
> + * NOT FOR LEGACY DEVICES! */
> +#define VIRTIO_MMIO_CONFIG_GENERATION	0x0fc
> +
>  /* The config space is defined by each driver as
>   * the per-driver configuration space - Read Write */
>  #define VIRTIO_MMIO_CONFIG		0x100
> -- 
> 2.1.0
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 06:51:01PM +0200, Michael S. Tsirkin wrote:
> Host no longer knows the alignment, so why is it needed?
> In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong:
> it corresponds to legacy devices, and it does not
> say what "align" is.

I created
https://issues.oasis-open.org/browse/VIRTIO-129
to track this. Can you write up a patch please?

-- 
MST

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 17:12 +0000, Michael S. Tsirkin wrote:
> On Thu, Jan 15, 2015 at 06:51:01PM +0200, Michael S. Tsirkin wrote:
> > Host no longer knows the alignment, so why is it needed?
> > In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong:
> > it corresponds to legacy devices, and it does not
> > say what "align" is.
> 
> I created
> https://issues.oasis-open.org/browse/VIRTIO-129
> to track this. Can you write up a patch please?

"4.3 Virtio Over Channel I/O"

(hint: not MMIO)

Paweł




_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 05:15:30PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 17:12 +0000, Michael S. Tsirkin wrote:
> > On Thu, Jan 15, 2015 at 06:51:01PM +0200, Michael S. Tsirkin wrote:
> > > Host no longer knows the alignment, so why is it needed?
> > > In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong:
> > > it corresponds to legacy devices, and it does not
> > > say what "align" is.
> > 
> > I created
> > https://issues.oasis-open.org/browse/VIRTIO-129
> > to track this. Can you write up a patch please?
> 
> "4.3 Virtio Over Channel I/O"
> 
> (hint: not MMIO)
> 
> Paweł
> 
> 

Ow. So it's s390 actually.
Fixed and assigned to Cornelia.

All the more reason not to have the ALIGN
macro in MMIO version 1 code.

-- 
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 16:51 +0000, Michael S. Tsirkin wrote:
> > +             uint64_t addr = virt_to_phys(info->queue);
> 
> Kernel normally uses u64 for this type.

Sure, well spotted.

> > +
> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> > +
> > +             addr += info->num * sizeof(struct vring_desc);
> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
> 
> 0xffffffff isn't really needed, is it?

I admit I'm never sure what are the narrowing side effects. You are
probably right that u64 >> 32 will be always 32 bit.

> > +
> > +             addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
> > +             addr += VIRTIO_MMIO_VRING_ALIGN - 1;
> > +             addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);
> 
> 
> Host no longer knows the alignment, so why is it needed?

[skipped the spec reference, it's a separate discussion]

> I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> it's a legacy thing.

But I still need to pass something to vring_new_virtqueue() below, don't
I? And it will allocate the queue based on some alignment value. I can't
see anything that would create the layout for me, neither in mainline
nor in next. Have I missed something? (wouldn't be surprised if I have)

> > +             writel(addr & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> > +             writel((addr >> 32) & 0xffffffff,
> > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> > +
> > +             writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> > +     }
> >
> >       /* Create the vring */
> >       vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,

[...]

> > +static struct device_attribute vm_dev_attr_version =
> > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > +
> >  static int virtio_mmio_probe(struct platform_device *pdev)
> >  {
> >       struct virtio_mmio_device *vm_dev;
> 
> We already expose feature bits - this one really necessary?

Necessary? Of course not, just a debugging feature, really, to see what
version of control registers are available. Useful - I strongly believe
so.

> > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >
> >       /* Check device version */
> >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > -     if (vm_dev->version != 1) {
> > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> >                               vm_dev->version);
> >               return -ENXIO;
> >       }
> >
> >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > +     if (vm_dev->vdev.id.device == 0) {
> > +             /*
> > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > +              * (as in: no error) with no further actions
> > +              */
> > +             return 0;
> 
> Necessary?
> We don't have drivers for this id anyway.

I'm not sure if you are joking or not, after the battle we fought over
it.

The short answer is: yes. Necessary.

"4.2.2 MMIO Device Register Layout

[...]

Virtio Subsystem Device ID
See 5 Device Types for possible values. Value zero (0x0) is used to de-
fine a system memory map with placeholder devices at static, well known
addresses, assigning functions to them depending on user’s needs.

[...]

4.2.2.2 Driver Requirements: MMIO Device Register Layout

The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
any error."

> > +     }
> 
> Need to also
>         1. validate that feature bit VIRTIO_1 is set
>         2. validate that ID is not for a legacy device
> 
> otherwise device specific drivers might get invoked
> on future devices (e.g. when we update balloon for 1.0)
> and they not do the right thing.

I'm not following you, but I admit I haven't though this problem
thoroughly. If you can volunteer an example of things going on, it would
be useful. Either way, I'll think about it again.

> @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> >  {
> >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> >
> > -     unregister_virtio_device(&vm_dev->vdev);
> > +     if (vm_dev)
> > +             unregister_virtio_device(&vm_dev->vdev);
> >
> 
> Will remove ever be called if probe fails?

No.

> > -/* Guest's memory page size in bytes - Write Only */
> > +/* Guest's memory page size in bytes - Write Only
> > + * LEGACY DEVICES ONLY! */
> 
> This is not the preferred style for multi-line comments :)

Fact. Will fix.

> Also - maybe add a flag to selectively disable legacy
> or modern macros?
> Might be clearer than comments that, after all, never compile.

As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
although I'm not sure how beautiful would that be. Will have a look, but
it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.

Paweł

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 05:32:38PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 16:51 +0000, Michael S. Tsirkin wrote:
> > > +             uint64_t addr = virt_to_phys(info->queue);
> > 
> > Kernel normally uses u64 for this type.
> 
> Sure, well spotted.
> 
> > > +
> > > +             writel(addr & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> > > +             writel((addr >> 32) & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> > > +
> > > +             addr += info->num * sizeof(struct vring_desc);
> > > +             writel(addr & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> > > +             writel((addr >> 32) & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
> > 
> > 0xffffffff isn't really needed, is it?
> 
> I admit I'm never sure what are the narrowing side effects. You are
> probably right that u64 >> 32 will be always 32 bit.
> 
> > > +
> > > +             addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
> > > +             addr += VIRTIO_MMIO_VRING_ALIGN - 1;
> > > +             addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);
> > 
> > 
> > Host no longer knows the alignment, so why is it needed?
> 
> [skipped the spec reference, it's a separate discussion]
> 
> > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > it's a legacy thing.
> 
> But I still need to pass something to vring_new_virtqueue() below, don't
> I? And it will allocate the queue based on some alignment value. I can't
> see anything that would create the layout for me, neither in mainline
> nor in next. Have I missed something? (wouldn't be surprised if I have)

No, but it's no longer a virtio thing - just an optimization
choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
And maybe add a TODO item - we can optimize by allocating chunks
separately.

> > > +             writel(addr & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> > > +             writel((addr >> 32) & 0xffffffff,
> > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> > > +
> > > +             writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> > > +     }
> > >
> > >       /* Create the vring */
> > >       vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> 
> [...]
> 
> > > +static struct device_attribute vm_dev_attr_version =
> > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > +
> > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > >  {
> > >       struct virtio_mmio_device *vm_dev;
> > 
> > We already expose feature bits - this one really necessary?
> 
> Necessary? Of course not, just a debugging feature, really, to see what
> version of control registers are available. Useful - I strongly believe
> so.

Yes but the point is the same info is already available
in core: just look at feature bit 31.
If you think it's important enough to expose in a decoded
way, let's add this in core?

> > > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> > >
> > >       /* Check device version */
> > >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > > -     if (vm_dev->version != 1) {
> > > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> > >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> > >                               vm_dev->version);
> > >               return -ENXIO;
> > >       }
> > >
> > >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > > +     if (vm_dev->vdev.id.device == 0) {
> > > +             /*
> > > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > > +              * (as in: no error) with no further actions
> > > +              */
> > > +             return 0;
> > 
> > Necessary?
> > We don't have drivers for this id anyway.
> 
> I'm not sure if you are joking or not, after the battle we fought over
> it.

Sorry, I don't remember anymore. Just asking.

> The short answer is: yes. Necessary.
> 
> "4.2.2 MMIO Device Register Layout
> 
> [...]
> 
> Virtio Subsystem Device ID
> See 5 Device Types for possible values. Value zero (0x0) is used to de-
> fine a system memory map with placeholder devices at static, well known
> addresses, assigning functions to them depending on user’s needs.
> 
> [...]
> 
> 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> 
> The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
> any error."


Absolutely. So what happens if you drop these code lines?
There's no driver registered for this ID, so it's just ignored.
Seems like what spec is asking for, no?

> > > +     }
> > 
> > Need to also
> >         1. validate that feature bit VIRTIO_1 is set
> >         2. validate that ID is not for a legacy device
> > 
> > otherwise device specific drivers might get invoked
> > on future devices (e.g. when we update balloon for 1.0)
> > and they not do the right thing.
> 
> I'm not following you, but I admit I haven't though this problem
> thoroughly. If you can volunteer an example of things going on, it would
> be useful. Either way, I'll think about it again.

1. you need to check ID 0, and assume rev 0. If device also
   says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
        if (virtio_device_is_legacy_only(vp_dev->vdev.id))
                return -ENODEV;

   you can find the code in my tree, see below.


2. it's easy - just get features on probe and validate VIRTIO_1
   bit is set.

   s390 does it differently since same device supports version 1 and 0.
   No example yet - I forgot to code this up for virtio pci.  I'll copy you
   on patch.




> > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > >  {
> > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > >
> > > -     unregister_virtio_device(&vm_dev->vdev);
> > > +     if (vm_dev)
> > > +             unregister_virtio_device(&vm_dev->vdev);
> > >
> > 
> > Will remove ever be called if probe fails?
> 
> No.

Then this if isn't necessary: vm_dev is always set.

> > > -/* Guest's memory page size in bytes - Write Only */
> > > +/* Guest's memory page size in bytes - Write Only
> > > + * LEGACY DEVICES ONLY! */
> > 
> > This is not the preferred style for multi-line comments :)
> 
> Fact. Will fix.
> 
> > Also - maybe add a flag to selectively disable legacy
> > or modern macros?
> > Might be clearer than comments that, after all, never compile.
> 
> As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
> although I'm not sure how beautiful would that be. Will have a look, but
> it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.
> 
> Paweł

Not necessarily - the point is for userspace to be able to
avoid getting useless legacy macros by means of a simple #define.
Again, take a look at my tree:

git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next

-- 
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 17:51 +0000, Michael S. Tsirkin wrote:
> > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > it's a legacy thing.
> > 
> > But I still need to pass something to vring_new_virtqueue() below, don't
> > I? And it will allocate the queue based on some alignment value. I can't
> > see anything that would create the layout for me, neither in mainline
> > nor in next. Have I missed something? (wouldn't be surprised if I have)
> 
> No, but it's no longer a virtio thing - just an optimization
> choice by a specific driver. So please just stick e.g. PAGE_SIZE there.

#define VIRTIO_MMIO_VRING_ALIGN         PAGE_SIZE

> And maybe add a TODO item - we can optimize by allocating chunks
> separately.

I'll wait and see how do deal with

        vq = vring_new_virtqueue(index, info->num,                                                                     
                                 VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,                                                

> > > > +static struct device_attribute vm_dev_attr_version =
> > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > +
> > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > >  {
> > > >       struct virtio_mmio_device *vm_dev;
> > > 
> > > We already expose feature bits - this one really necessary?
> > 
> > Necessary? Of course not, just a debugging feature, really, to see what
> > version of control registers are available. Useful - I strongly believe
> > so.
> 
> Yes but the point is the same info is already available
> in core: just look at feature bit 31.
> If you think it's important enough to expose in a decoded
> way, let's add this in core?

How do you mean "in core"? It's a mmio-specific value. Content of the
VIRTIO_MMIO_VERSION control register.

> > > > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> > > >
> > > >       /* Check device version */
> > > >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > > > -     if (vm_dev->version != 1) {
> > > > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> > > >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> > > >                               vm_dev->version);
> > > >               return -ENXIO;
> > > >       }
> > > >
> > > >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > > > +     if (vm_dev->vdev.id.device == 0) {
> > > > +             /*
> > > > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > > > +              * (as in: no error) with no further actions
> > > > +              */
> > > > +             return 0;
> > > 
> > > Necessary?
> > > We don't have drivers for this id anyway.
> > 
> > I'm not sure if you are joking or not, after the battle we fought over
> > it.
> 
> Sorry, I don't remember anymore. Just asking.
> 
> > The short answer is: yes. Necessary.
> > 
> > "4.2.2 MMIO Device Register Layout
> > 
> > [...]
> > 
> > Virtio Subsystem Device ID
> > See 5 Device Types for possible values. Value zero (0x0) is used to de-
> > fine a system memory map with placeholder devices at static, well known
> > addresses, assigning functions to them depending on user’s needs.
> > 
> > [...]
> > 
> > 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> > 
> > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
> > any error."
> 
> 
> Absolutely. So what happens if you drop these code lines?
> There's no driver registered for this ID, so it's just ignored.
> Seems like what spec is asking for, no?

Not to me, no. There will be a vm_dev registered with an _illegal_ ID.

> > > > +     }
> > > 
> > > Need to also
> > >         1. validate that feature bit VIRTIO_1 is set
> > >         2. validate that ID is not for a legacy device
> > > 
> > > otherwise device specific drivers might get invoked
> > > on future devices (e.g. when we update balloon for 1.0)
> > > and they not do the right thing.
> > 
> > I'm not following you, but I admit I haven't though this problem
> > thoroughly. If you can volunteer an example of things going on, it would
> > be useful. Either way, I'll think about it again.
> 
> 1. you need to check ID 0, and assume rev 0. If device also
>    says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
>         if (virtio_device_is_legacy_only(vp_dev->vdev.id))
>                 return -ENODEV;
> 
>    you can find the code in my tree, see below.
> 
> 
> 2. it's easy - just get features on probe and validate VIRTIO_1
>    bit is set.
> 
>    s390 does it differently since same device supports version 1 and 0.
>    No example yet - I forgot to code this up for virtio pci.  I'll copy you
>    on patch.

Ok, will have a look.

> > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > >  {
> > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > >
> > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > +     if (vm_dev)
> > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > >
> > > 
> > > Will remove ever be called if probe fails?
> > 
> > No.
> 
> Then this if isn't necessary: vm_dev is always set.

Not (in the current code) if ID is 0.

> > > > -/* Guest's memory page size in bytes - Write Only */
> > > > +/* Guest's memory page size in bytes - Write Only
> > > > + * LEGACY DEVICES ONLY! */
> > > 
> > > This is not the preferred style for multi-line comments :)
> > 
> > Fact. Will fix.
> > 
> > > Also - maybe add a flag to selectively disable legacy
> > > or modern macros?
> > > Might be clearer than comments that, after all, never compile.
> > 
> > As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
> > although I'm not sure how beautiful would that be. Will have a look, but
> > it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.
> > 
> > Paweł
> 
> Not necessarily - the point is for userspace to be able to
> avoid getting useless legacy macros by means of a simple #define.
> Again, take a look at my tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next

I have no idea what are you talking about here. Why would userspace ever
access the memory mapped control registers? That's all what this header
describes. Actually, if I was typing the driver today, I'd define the
offsets in virtio_mmio.c file, not in a separate header. If fact, I may
move them there...

Paweł

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 07:51:32PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jan 15, 2015 at 05:32:38PM +0000, Pawel Moll wrote:
> > On Thu, 2015-01-15 at 16:51 +0000, Michael S. Tsirkin wrote:
> > > > +             uint64_t addr = virt_to_phys(info->queue);
> > > 
> > > Kernel normally uses u64 for this type.
> > 
> > Sure, well spotted.
> > 
> > > > +
> > > > +             writel(addr & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW);
> > > > +             writel((addr >> 32) & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH);
> > > > +
> > > > +             addr += info->num * sizeof(struct vring_desc);
> > > > +             writel(addr & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_LOW);
> > > > +             writel((addr >> 32) & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_AVAIL_HIGH);
> > > 
> > > 0xffffffff isn't really needed, is it?
> > 
> > I admit I'm never sure what are the narrowing side effects. You are
> > probably right that u64 >> 32 will be always 32 bit.
> > 
> > > > +
> > > > +             addr += sizeof(struct vring_avail) + info->num * sizeof(__u16);
> > > > +             addr += VIRTIO_MMIO_VRING_ALIGN - 1;
> > > > +             addr &= ~(VIRTIO_MMIO_VRING_ALIGN - 1);
> > > 
> > > 
> > > Host no longer knows the alignment, so why is it needed?
> > 
> > [skipped the spec reference, it's a separate discussion]
> > 
> > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > it's a legacy thing.
> > 
> > But I still need to pass something to vring_new_virtqueue() below, don't
> > I? And it will allocate the queue based on some alignment value. I can't
> > see anything that would create the layout for me, neither in mainline
> > nor in next. Have I missed something? (wouldn't be surprised if I have)
> 
> No, but it's no longer a virtio thing - just an optimization
> choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
> And maybe add a TODO item - we can optimize by allocating chunks
> separately.
> 
> > > > +             writel(addr & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_LOW);
> > > > +             writel((addr >> 32) & 0xffffffff,
> > > > +                             vm_dev->base + VIRTIO_MMIO_QUEUE_USED_HIGH);
> > > > +
> > > > +             writel(1, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
> > > > +     }
> > > >
> > > >       /* Create the vring */
> > > >       vq = vring_new_virtqueue(index, info->num, VIRTIO_MMIO_VRING_ALIGN, vdev,
> > 
> > [...]
> > 
> > > > +static struct device_attribute vm_dev_attr_version =
> > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > +
> > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > >  {
> > > >       struct virtio_mmio_device *vm_dev;
> > > 
> > > We already expose feature bits - this one really necessary?
> > 
> > Necessary? Of course not, just a debugging feature, really, to see what
> > version of control registers are available. Useful - I strongly believe
> > so.
> 
> Yes but the point is the same info is already available
> in core: just look at feature bit 31.
> If you think it's important enough to expose in a decoded
> way, let's add this in core?
> 
> > > > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> > > >
> > > >       /* Check device version */
> > > >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > > > -     if (vm_dev->version != 1) {
> > > > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> > > >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> > > >                               vm_dev->version);
> > > >               return -ENXIO;
> > > >       }
> > > >
> > > >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > > > +     if (vm_dev->vdev.id.device == 0) {
> > > > +             /*
> > > > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > > > +              * (as in: no error) with no further actions
> > > > +              */
> > > > +             return 0;
> > > 
> > > Necessary?
> > > We don't have drivers for this id anyway.
> > 
> > I'm not sure if you are joking or not, after the battle we fought over
> > it.
> 
> Sorry, I don't remember anymore. Just asking.
> 
> > The short answer is: yes. Necessary.
> > 
> > "4.2.2 MMIO Device Register Layout
> > 
> > [...]
> > 
> > Virtio Subsystem Device ID
> > See 5 Device Types for possible values. Value zero (0x0) is used to de-
> > fine a system memory map with placeholder devices at static, well known
> > addresses, assigning functions to them depending on user’s needs.
> > 
> > [...]
> > 
> > 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> > 
> > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
> > any error."
> 
> 
> Absolutely. So what happens if you drop these code lines?
> There's no driver registered for this ID, so it's just ignored.
> Seems like what spec is asking for, no?
> 
> > > > +     }
> > > 
> > > Need to also
> > >         1. validate that feature bit VIRTIO_1 is set
> > >         2. validate that ID is not for a legacy device
> > > 
> > > otherwise device specific drivers might get invoked
> > > on future devices (e.g. when we update balloon for 1.0)
> > > and they not do the right thing.
> > 
> > I'm not following you, but I admit I haven't though this problem
> > thoroughly. If you can volunteer an example of things going on, it would
> > be useful. Either way, I'll think about it again.
> 
> 1. you need to check ID 0, and assume rev 0. If device also
>    says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
>         if (virtio_device_is_legacy_only(vp_dev->vdev.id))
>                 return -ENODEV;
> 
>    you can find the code in my tree, see below.
> 
> 
> 2. it's easy - just get features on probe and validate VIRTIO_1
>    bit is set.
> 
>    s390 does it differently since same device supports version 1 and 0.
>    No example yet - I forgot to code this up for virtio pci.  I'll copy you
>    on patch.

I forgot: s390 does have this code actually:

        if (vcdev->revision >= 1 &&
            !__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
                dev_err(&vdev->dev, "virtio: device uses revision 1 "
                        "but does not have VIRTIO_F_VERSION_1\n");
                return -EINVAL;
        }

I think that's an easier way to do it for PCI as well,
will send patch.


> 
> 
> 
> > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > >  {
> > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > >
> > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > +     if (vm_dev)
> > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > >
> > > 
> > > Will remove ever be called if probe fails?
> > 
> > No.
> 
> Then this if isn't necessary: vm_dev is always set.
> 
> > > > -/* Guest's memory page size in bytes - Write Only */
> > > > +/* Guest's memory page size in bytes - Write Only
> > > > + * LEGACY DEVICES ONLY! */
> > > 
> > > This is not the preferred style for multi-line comments :)
> > 
> > Fact. Will fix.
> > 
> > > Also - maybe add a flag to selectively disable legacy
> > > or modern macros?
> > > Might be clearer than comments that, after all, never compile.
> > 
> > As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
> > although I'm not sure how beautiful would that be. Will have a look, but
> > it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.
> > 
> > Paweł
> 
> Not necessarily - the point is for userspace to be able to
> avoid getting useless legacy macros by means of a simple #define.
> Again, take a look at my tree:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
> 
> -- 
> MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 06:11:17PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 17:51 +0000, Michael S. Tsirkin wrote:
> > > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > > it's a legacy thing.
> > > 
> > > But I still need to pass something to vring_new_virtqueue() below, don't
> > > I? And it will allocate the queue based on some alignment value. I can't
> > > see anything that would create the layout for me, neither in mainline
> > > nor in next. Have I missed something? (wouldn't be surprised if I have)
> > 
> > No, but it's no longer a virtio thing - just an optimization
> > choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
> 
> #define VIRTIO_MMIO_VRING_ALIGN         PAGE_SIZE
> 
> > And maybe add a TODO item - we can optimize by allocating chunks
> > separately.
> 
> I'll wait and see how do deal with
> 
>         vq = vring_new_virtqueue(index, info->num,                                                                     
>                                  VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,                                                

Check out the code in my tree.
It really does, fundamentally:

	/* TODO: don't allocate contigiously */
         vq = vring_new_virtqueue(index, info->num,                                                                     
                                  SMP_CACHE_BYTES, &vp_dev->vdev,                                                


> > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > +
> > > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > > >  {
> > > > >       struct virtio_mmio_device *vm_dev;
> > > > 
> > > > We already expose feature bits - this one really necessary?
> > > 
> > > Necessary? Of course not, just a debugging feature, really, to see what
> > > version of control registers are available. Useful - I strongly believe
> > > so.
> > 
> > Yes but the point is the same info is already available
> > in core: just look at feature bit 31.
> > If you think it's important enough to expose in a decoded
> > way, let's add this in core?
> 
> How do you mean "in core"? It's a mmio-specific value. Content of the
> VIRTIO_MMIO_VERSION control register.

Yes but if driver loaded, then revision is always in sync
with the feature bit.
If driver failed to attach, attribute is not there
so can't be used for debugging.



> > > > > @@ -476,16 +501,26 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> > > > >
> > > > >       /* Check device version */
> > > > >       vm_dev->version = readl(vm_dev->base + VIRTIO_MMIO_VERSION);
> > > > > -     if (vm_dev->version != 1) {
> > > > > +     if (vm_dev->version < 1 || vm_dev->version > 2) {
> > > > >               dev_err(&pdev->dev, "Version %ld not supported!\n",
> > > > >                               vm_dev->version);
> > > > >               return -ENXIO;
> > > > >       }
> > > > >
> > > > >       vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> > > > > +     if (vm_dev->vdev.id.device == 0) {
> > > > > +             /*
> > > > > +              * ID 0 means a dummy (placeholder) device, skip quietly
> > > > > +              * (as in: no error) with no further actions
> > > > > +              */
> > > > > +             return 0;
> > > > 
> > > > Necessary?
> > > > We don't have drivers for this id anyway.
> > > 
> > > I'm not sure if you are joking or not, after the battle we fought over
> > > it.
> > 
> > Sorry, I don't remember anymore. Just asking.
> > 
> > > The short answer is: yes. Necessary.
> > > 
> > > "4.2.2 MMIO Device Register Layout
> > > 
> > > [...]
> > > 
> > > Virtio Subsystem Device ID
> > > See 5 Device Types for possible values. Value zero (0x0) is used to de-
> > > fine a system memory map with placeholder devices at static, well known
> > > addresses, assigning functions to them depending on user’s needs.
> > > 
> > > [...]
> > > 
> > > 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> > > 
> > > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report
> > > any error."
> > 
> > 
> > Absolutely. So what happens if you drop these code lines?
> > There's no driver registered for this ID, so it's just ignored.
> > Seems like what spec is asking for, no?
> 
> Not to me, no. There will be a vm_dev registered with an _illegal_ ID.

Yes - there will be a device, but no driver will drive it.

> > > > > +     }
> > > > 
> > > > Need to also
> > > >         1. validate that feature bit VIRTIO_1 is set
> > > >         2. validate that ID is not for a legacy device
> > > > 
> > > > otherwise device specific drivers might get invoked
> > > > on future devices (e.g. when we update balloon for 1.0)
> > > > and they not do the right thing.
> > > 
> > > I'm not following you, but I admit I haven't though this problem
> > > thoroughly. If you can volunteer an example of things going on, it would
> > > be useful. Either way, I'll think about it again.
> > 
> > 1. you need to check ID 0, and assume rev 0. If device also
> >    says it needs rev 1, fail. E.g. see my patch for virtio_pci_modern:
> >         if (virtio_device_is_legacy_only(vp_dev->vdev.id))
> >                 return -ENODEV;
> > 
> >    you can find the code in my tree, see below.
> > 
> > 
> > 2. it's easy - just get features on probe and validate VIRTIO_1
> >    bit is set.
> > 
> >    s390 does it differently since same device supports version 1 and 0.
> >    No example yet - I forgot to code this up for virtio pci.  I'll copy you
> >    on patch.
> 
> Ok, will have a look.

Sent corrections - can be done like this, but easier to do
in finalize_features.


> > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > >  {
> > > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > >
> > > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > > +     if (vm_dev)
> > > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > > >
> > > > 
> > > > Will remove ever be called if probe fails?
> > > 
> > > No.
> > 
> > Then this if isn't necessary: vm_dev is always set.
> 
> Not (in the current code) if ID is 0.

So just return -ENODEV, then probe will be considered
failed and remove won't be called.
Or - better - just register the device, it's harmless
as no driver will try to attach to it, and there
won't be any need to special-case it.


> > > > > -/* Guest's memory page size in bytes - Write Only */
> > > > > +/* Guest's memory page size in bytes - Write Only
> > > > > + * LEGACY DEVICES ONLY! */
> > > > 
> > > > This is not the preferred style for multi-line comments :)
> > > 
> > > Fact. Will fix.
> > > 
> > > > Also - maybe add a flag to selectively disable legacy
> > > > or modern macros?
> > > > Might be clearer than comments that, after all, never compile.
> > > 
> > > As in, a bunch of #ifdefs disabling the legacy lines of code? Doable,
> > > although I'm not sure how beautiful would that be. Will have a look, but
> > > it probably would only make sense with CONFIG_VIRTIO_MMIO_LEGACY option.
> > > 
> > > Paweł
> > 
> > Not necessarily - the point is for userspace to be able to
> > avoid getting useless legacy macros by means of a simple #define.
> > Again, take a look at my tree:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
> 
> I have no idea what are you talking about here. Why would userspace ever
> access the memory mapped control registers?

Userspace being qemu which implements these.

> That's all what this header
> describes. Actually, if I was typing the driver today, I'd define the
> offsets in virtio_mmio.c file, not in a separate header. If fact, I may
> move them there...
> 
> Paweł

And then you will have to copy and paste them in qemu.

Which we do ATM for silly reasons like
we include linux/types.h, but I fully intend to get rid of that,
just tweak the linux ones slightly for portability.

-- 
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Fri, Dec 19, 2014 at 06:38:04PM +0000, Pawel Moll wrote:
> Tested with our custom models (*not* qemu).

Could you look into updating qemu as well please?
I'd like to make sure patches are widely testable
before pushing them, to reduce the chances of
finding issues after kernel is released.

You can find draft patches for pci and s390 here:
git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git virtio-1.0

-- 
MST

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 18:29 +0000, Michael S. Tsirkin wrote:
> On Thu, Jan 15, 2015 at 06:11:17PM +0000, Pawel Moll wrote:
> > On Thu, 2015-01-15 at 17:51 +0000, Michael S. Tsirkin wrote:
> > > > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > > > it's a legacy thing.
> > > > 
> > > > But I still need to pass something to vring_new_virtqueue() below, don't
> > > > I? And it will allocate the queue based on some alignment value. I can't
> > > > see anything that would create the layout for me, neither in mainline
> > > > nor in next. Have I missed something? (wouldn't be surprised if I have)
> > > 
> > > No, but it's no longer a virtio thing - just an optimization
> > > choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
> > 
> > #define VIRTIO_MMIO_VRING_ALIGN         PAGE_SIZE
> > 
> > > And maybe add a TODO item - we can optimize by allocating chunks
> > > separately.
> > 
> > I'll wait and see how do deal with
> > 
> >         vq = vring_new_virtqueue(index, info->num,                                                                     
> >                                  VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,                                                
> 
> Check out the code in my tree.
> It really does, fundamentally:
> 
> 	/* TODO: don't allocate contigiously */
>          vq = vring_new_virtqueue(index, info->num,                                                                     
>                                   SMP_CACHE_BYTES, &vp_dev->vdev,                                                

Will have a look.

> > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > +
> > > > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > >  {
> > > > > >       struct virtio_mmio_device *vm_dev;
> > > > > 
> > > > > We already expose feature bits - this one really necessary?
> > > > 
> > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > version of control registers are available. Useful - I strongly believe
> > > > so.
> > > 
> > > Yes but the point is the same info is already available
> > > in core: just look at feature bit 31.
> > > If you think it's important enough to expose in a decoded
> > > way, let's add this in core?
> > 
> > How do you mean "in core"? It's a mmio-specific value. Content of the
> > VIRTIO_MMIO_VERSION control register.
> 
> Yes but if driver loaded, then revision is always in sync
> with the feature bit.

Well, not quite: as of now I've got legacy block device driver happily
working on top of compliant (so v2 in MMIO speech) MMIO device - the
transport if completely transparent here.

> If driver failed to attach, attribute is not there
> so can't be used for debugging.

Interesting point on its own, haven't thought of that. This is an issue
with platform devices, no standard set of attributes, always available.
Will have a look at this.

> > > Absolutely. So what happens if you drop these code lines?
> > > There's no driver registered for this ID, so it's just ignored.
> > > Seems like what spec is asking for, no?
> > 
> > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> 
> Yes - there will be a device, but no driver will drive it.

A device with an *illegal* ID.

> > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > >  {
> > > > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > >
> > > > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > > > +     if (vm_dev)
> > > > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > > > >
> > > > > 
> > > > > Will remove ever be called if probe fails?
> > > > 
> > > > No.
> > > 
> > > Then this if isn't necessary: vm_dev is always set.
> > 
> > Not (in the current code) if ID is 0.
> 
> So just return -ENODEV, then probe will be considered
> failed and remove won't be called.

"4.2.2.2 Driver Requirements: MMIO Device Register Layout 

The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."

Returning -ENODEV *reports* an error.

> Or - better - just register the device, it's harmless
> as no driver will try to attach to it, and there
> won't be any need to special-case it.

Really, you may want to refresh your memory. We've been there. This *is*
a special case. Intentionally.

> > > Not necessarily - the point is for userspace to be able to
> > > avoid getting useless legacy macros by means of a simple #define.
> > > Again, take a look at my tree:
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
> > 
> > I have no idea what are you talking about here. Why would userspace ever
> > access the memory mapped control registers?
> 
> Userspace being qemu which implements these.

Right, qemu is a valid userspace case.

> > That's all what this header
> > describes. Actually, if I was typing the driver today, I'd define the
> > offsets in virtio_mmio.c file, not in a separate header. If fact, I may
> > move them there...
>
> And then you will have to copy and paste them in qemu.
> 
> Which we do ATM for silly reasons like
> we include linux/types.h, but I fully intend to get rid of that,
> just tweak the linux ones slightly for portability.

Ok, more than happy to do whatever will make qemu happy. An example?

Paweł

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 18:39 +0000, Michael S. Tsirkin wrote:
> On Fri, Dec 19, 2014 at 06:38:04PM +0000, Pawel Moll wrote:
> > Tested with our custom models (*not* qemu).
> 
> Could you look into updating qemu as well please?

No, sorry. I'm officially not allowed to touch qemu, due to some legal
issues.

I'll talk to Peter Maydell.

Paweł

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 06:42:02PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 18:29 +0000, Michael S. Tsirkin wrote:
> > On Thu, Jan 15, 2015 at 06:11:17PM +0000, Pawel Moll wrote:
> > > On Thu, 2015-01-15 at 17:51 +0000, Michael S. Tsirkin wrote:
> > > > > > I think you shouldn't use VIRTIO_MMIO_VRING_ALIGN in non-legacy code:
> > > > > > it's a legacy thing.
> > > > > 
> > > > > But I still need to pass something to vring_new_virtqueue() below, don't
> > > > > I? And it will allocate the queue based on some alignment value. I can't
> > > > > see anything that would create the layout for me, neither in mainline
> > > > > nor in next. Have I missed something? (wouldn't be surprised if I have)
> > > > 
> > > > No, but it's no longer a virtio thing - just an optimization
> > > > choice by a specific driver. So please just stick e.g. PAGE_SIZE there.
> > > 
> > > #define VIRTIO_MMIO_VRING_ALIGN         PAGE_SIZE
> > > 
> > > > And maybe add a TODO item - we can optimize by allocating chunks
> > > > separately.
> > > 
> > > I'll wait and see how do deal with
> > > 
> > >         vq = vring_new_virtqueue(index, info->num,                                                                     
> > >                                  VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev,                                                
> > 
> > Check out the code in my tree.
> > It really does, fundamentally:
> > 
> > 	/* TODO: don't allocate contigiously */
> >          vq = vring_new_virtqueue(index, info->num,                                                                     
> >                                   SMP_CACHE_BYTES, &vp_dev->vdev,                                                
> 
> Will have a look.
> 
> > > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > > +
> > > > > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > > >  {
> > > > > > >       struct virtio_mmio_device *vm_dev;
> > > > > > 
> > > > > > We already expose feature bits - this one really necessary?
> > > > > 
> > > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > > version of control registers are available. Useful - I strongly believe
> > > > > so.
> > > > 
> > > > Yes but the point is the same info is already available
> > > > in core: just look at feature bit 31.
> > > > If you think it's important enough to expose in a decoded
> > > > way, let's add this in core?
> > > 
> > > How do you mean "in core"? It's a mmio-specific value. Content of the
> > > VIRTIO_MMIO_VERSION control register.
> > 
> > Yes but if driver loaded, then revision is always in sync
> > with the feature bit.
> 
> Well, not quite: as of now I've got legacy block device driver happily
> working on top of compliant (so v2 in MMIO speech) MMIO device - the
> transport if completely transparent here.

Spec says explicitly it's an illegal configuration.

Once driver code is upstream, we won't be able to drop it
because someone, somewhere will use it.

So *if* there's some reason to support a legacy device on top of
a modern transport, then let's document that,
but unless we do let's not create more configurations
that we later need to support.


> > If driver failed to attach, attribute is not there
> > so can't be used for debugging.
> 
> Interesting point on its own, haven't thought of that. This is an issue
> with platform devices, no standard set of attributes, always available.
> Will have a look at this.
> 
> > > > Absolutely. So what happens if you drop these code lines?
> > > > There's no driver registered for this ID, so it's just ignored.
> > > > Seems like what spec is asking for, no?
> > > 
> > > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> > 
> > Yes - there will be a device, but no driver will drive it.
> 
> A device with an *illegal* ID.

So? The "device" is just a bunch of allocated memory. There's no driver
and no one touches is, which is what the spec requires.

> > > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > > >  {
> > > > > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > > >
> > > > > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > > > > +     if (vm_dev)
> > > > > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > > > > >
> > > > > > 
> > > > > > Will remove ever be called if probe fails?
> > > > > 
> > > > > No.
> > > > 
> > > > Then this if isn't necessary: vm_dev is always set.
> > > 
> > > Not (in the current code) if ID is 0.
> > 
> > So just return -ENODEV, then probe will be considered
> > failed and remove won't be called.
> 
> "4.2.2.2 Driver Requirements: MMIO Device Register Layout 
> 
> The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."
> 
> Returning -ENODEV *reports* an error.

Reports to whom? Do you refer to http://xkcd.com/838/ ?
ENODEV is not "reported". It's just a function return value,
it tells kernel not to call remove.
Users don't know: module probe succeeds, core will not print any errors, user is not
queried.

What happens with your patch? Driver is attached to
device (check where does driver attribute points to!),
but doesn't do anything.

Looks like if you just keep going, you'll achieve
the same result.


> > Or - better - just register the device, it's harmless
> > as no driver will try to attach to it, and there
> > won't be any need to special-case it.
> 
> Really, you may want to refresh your memory. We've been there. This *is*
> a special case. Intentionally.

Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
but the resolution there is merely asking that no driver
matches ID 0. Seems OK.
The MMIO changes were all made as part of
https://issues.oasis-open.org/browse/VIRTIO-44
Anyway, my memory is irrelevant, we need to document motivation
for kernel code changes for future readers. It seems that dropping this
chunk satisfies the spec, so if not true, let's add a comment in code
that explains why.

Assume you stick in device with ID 0.
kernel probes the device, and then sees there is no driver
and leaves it alone.
Seems perfect, matches the spec.

Do you have a config that's not handled well here?

> > > > Not necessarily - the point is for userspace to be able to
> > > > avoid getting useless legacy macros by means of a simple #define.
> > > > Again, take a look at my tree:
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-next
> > > 
> > > I have no idea what are you talking about here. Why would userspace ever
> > > access the memory mapped control registers?
> > 
> > Userspace being qemu which implements these.
> 
> Right, qemu is a valid userspace case.
> 
> > > That's all what this header
> > > describes. Actually, if I was typing the driver today, I'd define the
> > > offsets in virtio_mmio.c file, not in a separate header. If fact, I may
> > > move them there...
> >
> > And then you will have to copy and paste them in qemu.
> > 
> > Which we do ATM for silly reasons like
> > we include linux/types.h, but I fully intend to get rid of that,
> > just tweak the linux ones slightly for portability.
> 
> Ok, more than happy to do whatever will make qemu happy. An example?
> 
> Paweł

See include/uapi/linux/virtio_pci.h in my tree above.

-- 
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Thu, Jan 15, 2015 at 06:51:37PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 18:39 +0000, Michael S. Tsirkin wrote:
> > On Fri, Dec 19, 2014 at 06:38:04PM +0000, Pawel Moll wrote:
> > > Tested with our custom models (*not* qemu).
> > 
> > Could you look into updating qemu as well please?
> 
> No, sorry. I'm officially not allowed to touch qemu, due to some legal
> issues.

look only, don't touch :)

> I'll talk to Peter Maydell.
> 
> Paweł
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Cornelia Huck]

On Thu, 15 Jan 2015 19:19:50 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 15, 2015 at 05:15:30PM +0000, Pawel Moll wrote:
> > On Thu, 2015-01-15 at 17:12 +0000, Michael S. Tsirkin wrote:
> > > On Thu, Jan 15, 2015 at 06:51:01PM +0200, Michael S. Tsirkin wrote:
> > > > Host no longer knows the alignment, so why is it needed?
> > > > In fact, I notice that 4.3.2.3 Virtqueue Layout seems completely wrong:
> > > > it corresponds to legacy devices, and it does not
> > > > say what "align" is.
> > > 
> > > I created
> > > https://issues.oasis-open.org/browse/VIRTIO-129
> > > to track this. Can you write up a patch please?
> > 
> > "4.3 Virtio Over Channel I/O"
> > 
> > (hint: not MMIO)
> > 
> > Paweł
> > 
> > 
> 
> Ow. So it's s390 actually.
> Fixed and assigned to Cornelia.

I'll handle that one.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Thu, 2015-01-15 at 19:12 +0000, Michael S. Tsirkin wrote:
> > > > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > > > +
> > > > > > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > >       struct virtio_mmio_device *vm_dev;
> > > > > > > 
> > > > > > > We already expose feature bits - this one really necessary?
> > > > > > 
> > > > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > > > version of control registers are available. Useful - I strongly believe
> > > > > > so.
> > > > > 
> > > > > Yes but the point is the same info is already available
> > > > > in core: just look at feature bit 31.
> > > > > If you think it's important enough to expose in a decoded
> > > > > way, let's add this in core?
> > > > 
> > > > How do you mean "in core"? It's a mmio-specific value. Content of the
> > > > VIRTIO_MMIO_VERSION control register.
> > > 
> > > Yes but if driver loaded, then revision is always in sync
> > > with the feature bit.
> > 
> > Well, not quite: as of now I've got legacy block device driver happily
> > working on top of compliant (so v2 in MMIO speech) MMIO device - the
> > transport if completely transparent here.
> 
> Spec says explicitly it's an illegal configuration.

What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
negotiated or not.

Make no mistake - I don't know why would anyone wanted to do this kind
of mishmash (other than for testing purposes), but from the MMIO
transport point of view, it's not a problem.

Does the check in finalize_features() solves the problem? If I
understand correctly it should, so there's nothing more to be done,
either in the MMIO spec or in the driver.

> > > If driver failed to attach, attribute is not there
> > > so can't be used for debugging.
> > 
> > Interesting point on its own, haven't thought of that. This is an issue
> > with platform devices, no standard set of attributes, always available.
> > Will have a look at this.
> > 
> > > > > Absolutely. So what happens if you drop these code lines?
> > > > > There's no driver registered for this ID, so it's just ignored.
> > > > > Seems like what spec is asking for, no?
> > > > 
> > > > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> > > 
> > > Yes - there will be a device, but no driver will drive it.
> > 
> > A device with an *illegal* ID.
> 
> So? The "device" is just a bunch of allocated memory. There's no driver
> and no one touches is, which is what the spec requires.

So what exactly is wrong with the "if (id == 0) ignore" case handling? I
bet a compare-to-zero and a branch in two places takes less memory than
the allocated struct virtio_device. And certainly takes less cycles (not
that this matters at all :-). And it's pretty well documented in the
spec.

> > > > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > > > >  {
> > > > > > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > > > >
> > > > > > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > > > > > +     if (vm_dev)
> > > > > > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > > > > > >
> > > > > > > 
> > > > > > > Will remove ever be called if probe fails?
> > > > > > 
> > > > > > No.
> > > > > 
> > > > > Then this if isn't necessary: vm_dev is always set.
> > > > 
> > > > Not (in the current code) if ID is 0.
> > > 
> > > So just return -ENODEV, then probe will be considered
> > > failed and remove won't be called.
> > 
> > "4.2.2.2 Driver Requirements: MMIO Device Register Layout 
> > 
> > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."
> > 
> > Returning -ENODEV *reports* an error.
> 
> Reports to whom? Do you refer to http://xkcd.com/838/ ?
> ENODEV is not "reported". 

No, you're right - -ENODEV doesn't print out "probe of xxx failed with
error yyy" message, I was wrong. I'm pretty sure I tried it before (I
was hoping to find an error which would block probing in a silent way)
and I saw something, but it was probably the pr_debug() level message.

> It's just a function return value,
> it tells kernel not to call remove.
> Users don't know: module probe succeeds, core will not print any errors, user is not
> queried.
> 
> What happens with your patch? Driver is attached to
> device (check where does driver attribute points to!),
> but doesn't do anything.
> 
> Looks like if you just keep going, you'll achieve
> the same result.

Yes, returning -ENODEV when ID == 0 seems perfectly fine for me.

> > > Or - better - just register the device, it's harmless
> > > as no driver will try to attach to it, and there
> > > won't be any need to special-case it.
> > 
> > Really, you may want to refresh your memory. We've been there. This *is*
> > a special case. Intentionally.
> 
> Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
> but the resolution there is merely asking that no driver
> matches ID 0. Seems OK.
> The MMIO changes were all made as part of
> https://issues.oasis-open.org/browse/VIRTIO-44
> Anyway, my memory is irrelevant, we need to document motivation
> for kernel code changes for future readers.

As a refresher:

https://lists.oasis-open.org/archives/virtio/201307/msg00035.html

and in particular:

> On Wed, 2013-07-31 at 15:04 +0100, Michael S. Tsirkin wrote: 
> > I meant using standard bus specific things where we are describing bus
> > specific behaviour.
> > In this case, I think you really want a "no device" flag for
> > virtio-mmio which originally lacked device presence detection.
> > I think for this purpose, it is best to to:
> > 1. declare device ID 0 (or e.g. FFFF) as reserved and illegal for devices,
> >    explicitly in core spec
> > 2. in the mmio appendix, say that if ID 0 (or FFFF) is detected, this
> >    means device not present
> > I think this is the cleaner approach than saying there's
> > a dummy "null device" in particular because
> > 1. there won't be dummy devices on the bus, in sysfs etc
> > 2. down the road you will be able to support hotplug.

It seems that you personally weren't happy about "dummy devices on the
bus"...

>  It seems that dropping this
> chunk satisfies the spec, so if not true, let's add a comment in code
> that explains why.
> 
> Assume you stick in device with ID 0.
> kernel probes the device, and then sees there is no driver
> and leaves it alone.
> Seems perfect, matches the spec.
> 
> Do you have a config that's not handled well here?

The end effect will be the same, I agree. I simply disagree with your
claim that it matches the spec and I see no point of discussing this
subject further. I'm going to keep the special case with -ENODEV in the
driver and apply other changes you suggested. If you want you can NAK
the updated patch and we'll find someone to resolve the argument.

Pawel

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Mon, Jan 19, 2015 at 05:45:54PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 19:12 +0000, Michael S. Tsirkin wrote:
> > > > > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > > > > +             __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > > > > +
> > > > > > > > >  static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > > > > >  {
> > > > > > > > >       struct virtio_mmio_device *vm_dev;
> > > > > > > > 
> > > > > > > > We already expose feature bits - this one really necessary?
> > > > > > > 
> > > > > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > > > > version of control registers are available. Useful - I strongly believe
> > > > > > > so.
> > > > > > 
> > > > > > Yes but the point is the same info is already available
> > > > > > in core: just look at feature bit 31.
> > > > > > If you think it's important enough to expose in a decoded
> > > > > > way, let's add this in core?
> > > > > 
> > > > > How do you mean "in core"? It's a mmio-specific value. Content of the
> > > > > VIRTIO_MMIO_VERSION control register.
> > > > 
> > > > Yes but if driver loaded, then revision is always in sync
> > > > with the feature bit.
> > > 
> > > Well, not quite: as of now I've got legacy block device driver happily
> > > working on top of compliant (so v2 in MMIO speech) MMIO device - the
> > > transport if completely transparent here.
> > 
> > Spec says explicitly it's an illegal configuration.
> 
> What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
> and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
> requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> negotiated or not.

The parts that say VIRTIO_F_VERSION_1 MUST be set.
I'll look up the chapter and verse later if you like.

> Make no mistake - I don't know why would anyone wanted to do this kind
> of mishmash (other than for testing purposes), but from the MMIO
> transport point of view, it's not a problem.
> 
> Does the check in finalize_features() solves the problem? If I
> understand correctly it should, so there's nothing more to be done,
> either in the MMIO spec or in the driver.

It does but it must be in driver since there are
transitional drivers so we can't just fail finalize_features
in core.

> > > > If driver failed to attach, attribute is not there
> > > > so can't be used for debugging.
> > > 
> > > Interesting point on its own, haven't thought of that. This is an issue
> > > with platform devices, no standard set of attributes, always available.
> > > Will have a look at this.
> > > 
> > > > > > Absolutely. So what happens if you drop these code lines?
> > > > > > There's no driver registered for this ID, so it's just ignored.
> > > > > > Seems like what spec is asking for, no?
> > > > > 
> > > > > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> > > > 
> > > > Yes - there will be a device, but no driver will drive it.
> > > 
> > > A device with an *illegal* ID.
> > 
> > So? The "device" is just a bunch of allocated memory. There's no driver
> > and no one touches is, which is what the spec requires.
> 
> So what exactly is wrong with the "if (id == 0) ignore" case handling? I
> bet a compare-to-zero and a branch in two places takes less memory than
> the allocated struct virtio_device. And certainly takes less cycles (not
> that this matters at all :-). And it's pretty well documented in the
> spec.
> > > > > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > > > > >  {
> > > > > > > > >       struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > > > > >
> > > > > > > > > -     unregister_virtio_device(&vm_dev->vdev);
> > > > > > > > > +     if (vm_dev)
> > > > > > > > > +             unregister_virtio_device(&vm_dev->vdev);
> > > > > > > > >
> > > > > > > > 
> > > > > > > > Will remove ever be called if probe fails?
> > > > > > > 
> > > > > > > No.
> > > > > > 
> > > > > > Then this if isn't necessary: vm_dev is always set.
> > > > > 
> > > > > Not (in the current code) if ID is 0.
> > > > 
> > > > So just return -ENODEV, then probe will be considered
> > > > failed and remove won't be called.
> > > 
> > > "4.2.2.2 Driver Requirements: MMIO Device Register Layout 
> > > 
> > > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."
> > > 
> > > Returning -ENODEV *reports* an error.
> > 
> > Reports to whom? Do you refer to http://xkcd.com/838/ ?
> > ENODEV is not "reported". 
> 
> No, you're right - -ENODEV doesn't print out "probe of xxx failed with
> error yyy" message, I was wrong. I'm pretty sure I tried it before (I
> was hoping to find an error which would block probing in a silent way)
> and I saw something, but it was probably the pr_debug() level message.
> 
> > It's just a function return value,
> > it tells kernel not to call remove.
> > Users don't know: module probe succeeds, core will not print any errors, user is not
> > queried.
> > 
> > What happens with your patch? Driver is attached to
> > device (check where does driver attribute points to!),
> > but doesn't do anything.
> > 
> > Looks like if you just keep going, you'll achieve
> > the same result.
> 
> Yes, returning -ENODEV when ID == 0 seems perfectly fine for me.
>
> > > > Or - better - just register the device, it's harmless
> > > > as no driver will try to attach to it, and there
> > > > won't be any need to special-case it.
> > > 
> > > Really, you may want to refresh your memory. We've been there. This *is*
> > > a special case. Intentionally.
> > 
> > Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
> > but the resolution there is merely asking that no driver
> > matches ID 0. Seems OK.
> > The MMIO changes were all made as part of
> > https://issues.oasis-open.org/browse/VIRTIO-44
> > Anyway, my memory is irrelevant, we need to document motivation
> > for kernel code changes for future readers.
> 
> As a refresher:
> 
> https://lists.oasis-open.org/archives/virtio/201307/msg00035.html
> 
> and in particular:
> 
> > On Wed, 2013-07-31 at 15:04 +0100, Michael S. Tsirkin wrote: 
> > > I meant using standard bus specific things where we are describing bus
> > > specific behaviour.
> > > In this case, I think you really want a "no device" flag for
> > > virtio-mmio which originally lacked device presence detection.
> > > I think for this purpose, it is best to to:
> > > 1. declare device ID 0 (or e.g. FFFF) as reserved and illegal for devices,
> > >    explicitly in core spec
> > > 2. in the mmio appendix, say that if ID 0 (or FFFF) is detected, this
> > >    means device not present
> > > I think this is the cleaner approach than saying there's
> > > a dummy "null device" in particular because
> > > 1. there won't be dummy devices on the bus, in sysfs etc
> > > 2. down the road you will be able to support hotplug.

Okay, I see, thanks!

> It seems that you personally weren't happy about "dummy devices on the
> bus"...
> 
> >  It seems that dropping this
> > chunk satisfies the spec, so if not true, let's add a comment in code
> > that explains why.
> > 
> > Assume you stick in device with ID 0.
> > kernel probes the device, and then sees there is no driver
> > and leaves it alone.
> > Seems perfect, matches the spec.
> > 
> > Do you have a config that's not handled well here?
> 
> The end effect will be the same, I agree. I simply disagree with your
> claim that it matches the spec and I see no point of discussing this
> subject further. I'm going to keep the special case with -ENODEV in the
> driver and apply other changes you suggested. If you want you can NAK
> the updated patch and we'll find someone to resolve the argument.
> 
> Pawel

Can you please also add a comment?
E.g. 
     /*
      * MMIO uses ID 0 to mean device not present.  Avoid probing
      * the device further: it's sure to fail anyway.
      */

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Mon, 2015-01-19 at 18:36 +0000, Michael S. Tsirkin wrote:
> > > > Well, not quite: as of now I've got legacy block device driver happily
> > > > working on top of compliant (so v2 in MMIO speech) MMIO device - the
> > > > transport if completely transparent here.
> > > 
> > > Spec says explicitly it's an illegal configuration.
> > 
> > What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
> > and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
> > requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> > negotiated or not.
> 
> The parts that say VIRTIO_F_VERSION_1 MUST be set.
> I'll look up the chapter and verse later if you like.

That would be:

"6.1 Driver Requirements: Reserved Feature Bits
A driver MUST accept VIRTIO_F_VERSION_1 if it is offered. A driver MAY
fail to operate further if VIRTIO_F_VERSION_1 is not offered."

"6.2 Device Requirements: Reserved Feature Bits
A device MUST offer VIRTIO_F_VERSION_1. A device MAY fail to operate
further if VIRTIO_F_VERSION_1 is not accepted."

Both are perfectly clear and reasonable for me. And the MMIO transport
in both versions (pre-spec and the spec one) will meet those
requirements, as it was able to pass more than 32 features bits from the
very beginning.

> Can you please also add a comment?
> E.g. 
>      /*
>       * MMIO uses ID 0 to mean device not present.  Avoid probing
>       * the device further: it's sure to fail anyway.
>       */

There is a comment:

> On Fri, 2014-12-19 at 18:38 +0000, Pawel Moll wrote: 
> > +		/*
> > +		 * ID 0 means a dummy (placeholder) device, skip quietly
> > +		 * (as in: no error) with no further actions
> > +		 */
> 
But I'm can use your wording if you find it better.

Pawel

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Tue, Jan 20, 2015 at 05:18:23PM +0000, Pawel Moll wrote:
> On Mon, 2015-01-19 at 18:36 +0000, Michael S. Tsirkin wrote:
> > > > > Well, not quite: as of now I've got legacy block device driver happily
> > > > > working on top of compliant (so v2 in MMIO speech) MMIO device - the
> > > > > transport if completely transparent here.
> > > > 
> > > > Spec says explicitly it's an illegal configuration.
> > > 
> > > What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
> > > and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
> > > requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> > > negotiated or not.
> > 
> > The parts that say VIRTIO_F_VERSION_1 MUST be set.
> > I'll look up the chapter and verse later if you like.
> 
> That would be:
> 
> "6.1 Driver Requirements: Reserved Feature Bits
> A driver MUST accept VIRTIO_F_VERSION_1 if it is offered. A driver MAY
> fail to operate further if VIRTIO_F_VERSION_1 is not offered."
> 
> "6.2 Device Requirements: Reserved Feature Bits
> A device MUST offer VIRTIO_F_VERSION_1. A device MAY fail to operate
> further if VIRTIO_F_VERSION_1 is not accepted."

That, and there are a bunch of other changes we made in devices
as compared to virtio 0.9.

> 
> Both are perfectly clear and reasonable for me. And the MMIO transport
> in both versions (pre-spec and the spec one) will meet those
> requirements, as it was able to pass more than 32 features bits from the
> very beginning.

Good. But your current code will also try to support a mix: device that
uses the new transport underneath, but old layout and semantics on top.

This has no value: the spec does *not* define such devices
and it also does not match any hosts, so this just adds to support matrix.
And if linux supports it, someone *will* ship such a
device and we'll be stuck with it forever.

So please, detect out of spec devices and fail gracefully.
pci and s390 do this already.


> > Can you please also add a comment?
> > E.g. 
> >      /*
> >       * MMIO uses ID 0 to mean device not present.  Avoid probing
> >       * the device further: it's sure to fail anyway.
> >       */
> 
> There is a comment:
> 
> > On Fri, 2014-12-19 at 18:38 +0000, Pawel Moll wrote: 
> > > +		/*
> > > +		 * ID 0 means a dummy (placeholder) device, skip quietly
> > > +		 * (as in: no error) with no further actions
> > > +		 */
> > 
> But I'm can use your wording if you find it better.
> 
> Pawel

Either that or combine them in some way.

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Pawel Moll]

On Tue, 2015-01-20 at 17:44 +0000, Michael S. Tsirkin wrote:
> Good. But your current code will also try to support a mix: device that
> uses the new transport underneath, but old layout and semantics on top.

It will not try to support the mix, but rather will not stop it from
working.

> This has no value: the spec does *not* define such devices
> and it also does not match any hosts, so this just adds to support matrix.
> And if linux supports it, someone *will* ship such a
> device and we'll be stuck with it forever.
>
> So please, detect out of spec devices and fail gracefully.
> pci and s390 do this already.

Ok, let it be. Will send v2 in a second.

Pawel

Re: [RFC] virtio-mmio: Update the device to OASIS spec version

[Michael S. Tsirkin]

On Tue, Jan 20, 2015 at 05:51:11PM +0000, Pawel Moll wrote:
> On Tue, 2015-01-20 at 17:44 +0000, Michael S. Tsirkin wrote:
> > Good. But your current code will also try to support a mix: device that
> > uses the new transport underneath, but old layout and semantics on top.
> 
> It will not try to support the mix, but rather will not stop it from
> working.
>
> > This has no value: the spec does *not* define such devices
> > and it also does not match any hosts, so this just adds to support matrix.
> > And if linux supports it, someone *will* ship such a
> > device and we'll be stuck with it forever.
> >
> > So please, detect out of spec devices and fail gracefully.
> > pci and s390 do this already.
> 
> Ok, let it be. Will send v2 in a second.
> 
> Pawel

Thanks!
The relevant functions are finalize_features
(to check feature bit) and probe (to check is_legacy).

-- 
MST