Messages in this thread | | | Subject | Re: [virtio-dev] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3 | From | Jason Wang <> | Date | Thu, 2 Jan 2020 14:33:20 +0800 |
| |
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) >>> + 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.
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 */ >>> +#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.
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.
Thanks
> > > 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@lists.oasis-open.org >> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org >> >
| |