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

[Liu, Jing2]

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)
> > +ÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂ vm_dev->vm_vq_names = kmalloc_array(nirqs, 
> > sizeof(*vm_dev->vm_vq_names),
> > +ÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
> > +ÂÂÂ if (!vm_dev->vm_vq_names)
> > +ÂÂÂÂÂÂÂ return -ENOMEM;
> > +
> > +ÂÂÂ 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 */
> > +#define VIRTIO_MMIO_MSI_STATUSÂÂÂÂÂÂÂ 0x0c0
> > +/* MSI command register */
> > +#define VIRTIO_MMIO_MSI_COMMANDÂÂÂÂÂÂÂ 0x0c2
> > +/* MSI low 32 bit address, 64 bits in two halves */
> > +#define VIRTIO_MMIO_MSI_ADDRESS_LOWÂÂÂ 0x0c4
> > +/* MSI high 32 bit address, 64 bits in two halves */
> > +#define VIRTIO_MMIO_MSI_ADDRESS_HIGHÂÂÂ 0x0c8
> > +/* MSI data */
> > +#define VIRTIO_MMIO_MSI_DATAÂÂÂÂÂÂÂ 0x0cc
>
>
> 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.

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 */
> > +#define VIRTIO_MMIO_MSI_ENABLE_MASKÂÂÂ 0x8000
> > +/* RO: Maximum queue size available */
> > +#define VIRTIO_MMIO_MSI_STATUS_QMASKÂÂÂ 0x07ff
> > +/* Reserved */
> > +#define VIRTIO_MMIO_MSI_STATUS_RESERVEDÂÂÂ 0x7800
> > +
> > +#define VIRTIO_MMIO_MSI_CMD_UPDATEÂÂÂ 0x1
>
>
> 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.

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.


Thanks!

Jing

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