Re: [virtio-dev] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3

From: Jason Wang
Date: Thu Jan 02 2020 - 01:34:21 EST

On 2019/12/27 äå5:37, Liu, Jing2 wrote:
Hi Jason,

Thanks for reviewing the patches!

+#include <linux/msi.h>
+#include <asm/irqdomain.h>
  /* The alignment to use between consumer and producer parts of vring.
ÂÂ * Currently hardcoded to the page size. */
@@ -90,6 +90,12 @@ struct virtio_mmio_device {
ÂÂÂÂÂ /* a list of queues so we can dispatch IRQs */
ÂÂÂÂÂ spinlock_t lock;
ÂÂÂÂÂ struct list_head virtqueues;
+ÂÂÂ int doorbell_base;
+ÂÂÂ int doorbell_scale;

It's better to use the terminology defined by spec, e.g notify_base/notify_multiplexer etc.

And we usually use unsigned type for offset.

We will fix this in next version.

+ÂÂÂ bool msi_enabled;
+ÂÂÂ /* Name strings for interrupts. */
+ÂÂÂ char (*vm_vq_names)[256];
  struct virtio_mmio_vq_info {
@@ -101,6 +107,8 @@ struct virtio_mmio_vq_info {
  +static void vm_free_msi_irqs(struct virtio_device *vdev);
+static int vm_request_msi_vectors(struct virtio_device *vdev, int nirqs);
  /* Configuration interface */
 @@ -273,12 +281,28 @@ static bool vm_notify(struct virtqueue *vq)
ÂÂÂÂÂ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
 + if (vm_dev->version == 3) {
+ÂÂÂÂÂÂÂ int offset = vm_dev->doorbell_base +
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ vm_dev->doorbell_scale * vq->index;
+ÂÂÂÂÂÂÂ writel(vq->index, vm_dev->base + offset);

In virtio-pci we store the doorbell address in vq->priv to avoid doing multiplication in fast path.

Good suggestion. We will fix this in next version.


+static int vm_request_msi_vectors(struct virtio_device *vdev, int nirqs)
+ÂÂÂ struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+ÂÂÂ int irq, err;
+ÂÂÂ u16 csr = readw(vm_dev->base + VIRTIO_MMIO_MSI_STATUS);
+ÂÂÂ if (vm_dev->msi_enabled || (csr & VIRTIO_MMIO_MSI_ENABLE_MASK) == 0)
+ÂÂÂ vm_dev->vm_vq_names = kmalloc_array(nirqs, sizeof(*vm_dev->vm_vq_names),
+ÂÂÂ if (!vm_dev->vm_vq_names)
+ÂÂÂ if (!vdev->dev.msi_domain)
+ÂÂÂÂÂÂÂ vdev->dev.msi_domain = platform_msi_get_def_irq_domain();
+ÂÂÂ err = platform_msi_domain_alloc_irqs(&vdev->dev, nirqs,
+ÂÂÂÂÂÂÂÂÂÂÂ mmio_write_msi_msg);

Can platform_msi_domain_alloc_irqs check msi_domain vs NULL?

Actually here, we need to firstly consider the cases that @dev doesn't have @msi_domain,

according to the report by lkp.

For the platform_msi_domain_alloc_irqs, it can help check inside.

 Â rc = register_virtio_device(&vm_dev->vdev);
ÂÂÂÂÂ if (rc)
@@ -619,8 +819,6 @@ static int virtio_mmio_remove(struct platform_device *pdev)
ÂÂÂÂÂ return 0;

Unnecessary changes.

Got it. Will remove it later.

 +/* MSI related registers */
+/* MSI status register */
+/* MSI command register */
+/* MSI low 32 bit address, 64 bits in two halves */
+/* MSI high 32 bit address, 64 bits in two halves */
+/* MSI data */

I wonder what's the advantage of using registers instead of memory mapped tables as PCI did. Is this because MMIO doesn't have capability list which makes it hard to be extended if we have variable size of tables?

Yes, mmio doesn't have capability which limits the extension.

We may want to revisit and add something like this for future virtio MMIO versions.

It need some registers to specify the msi table/mask table/pending table offsets if using what pci did.

As comments previously, mask/pending can be easily extended by MSI command.

+/* RO: MSI feature enabled mask */
+/* RO: Maximum queue size available */
+/* Reserved */

I believe we need a command to read the number of vectors supported by the device, or 2048 is assumed to be a fixed size here?

For not bringing much complexity, we proposed vector per queue and fixed relationship between events and vectors.

It's a about the number of MSIs not the mapping between queues to MSIs. And it looks to me it won't bring obvious complexity, just need a register to read the #MSIs. Device implementation may stick to a fixed size.

Having few pages for a device that only have one queue is kind of a waste.


So the number of vectors supported by device is equal to the total number of vqs and config.

We will try to explicitly highlight this point in spec for later version.



To unsubscribe, e-mail: virtio-dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
For additional commands, e-mail: virtio-dev-help@xxxxxxxxxxxxxxxxxxxx