From: Jason Wang <jasowang@redhat.com> To: "Liu, Jing2" <jing2.liu@linux.intel.com>, Zha Bin <zhabin@linux.alibaba.com>, linux-kernel@vger.kernel.org Cc: mst@redhat.com, slp@redhat.com, virtio-dev@lists.oasis-open.org, gerry@linux.alibaba.com, jing2.liu@intel.com, chao.p.peng@intel.com Subject: Re: [virtio-dev] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3 Date: Thu, 2 Jan 2020 14:33:20 +0800 [thread overview] Message-ID: <683cac51-853d-c8c8-24c6-b01886978ca4@redhat.com> (raw) In-Reply-To: <28da67db-73ab-f772-fb00-5a471b746fc5@linux.intel.com> 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 >> >
WARNING: multiple messages have this Message-ID (diff)
From: Jason Wang <jasowang@redhat.com> To: "Liu, Jing2" <jing2.liu@linux.intel.com>, Zha Bin <zhabin@linux.alibaba.com>, linux-kernel@vger.kernel.org Cc: mst@redhat.com, slp@redhat.com, virtio-dev@lists.oasis-open.org, gerry@linux.alibaba.com, jing2.liu@intel.com, chao.p.peng@intel.com Subject: Re: [virtio-dev] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3 Date: Thu, 2 Jan 2020 14:33:20 +0800 [thread overview] Message-ID: <683cac51-853d-c8c8-24c6-b01886978ca4@redhat.com> (raw) In-Reply-To: <28da67db-73ab-f772-fb00-5a471b746fc5@linux.intel.com> 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 >> > --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2020-01-02 6:33 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-25 2:50 [PATCH v1 0/2] support virtio mmio specification Version 3 Zha Bin 2019-12-25 2:50 ` [PATCH v1 1/2] x86/msi: Enhance x86 to support platform_msi Zha Bin 2019-12-26 8:21 ` Jason Wang 2019-12-26 8:21 ` [virtio-dev] " Jason Wang 2020-01-17 13:58 ` Thomas Gleixner 2020-01-17 14:06 ` Liu, Jiang 2020-01-19 2:27 ` Liu, Jing2 2020-01-19 2:27 ` [virtio-dev] " Liu, Jing2 2020-01-19 14:57 ` Thomas Gleixner 2019-12-25 2:50 ` [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3 Zha Bin 2019-12-25 10:20 ` Jason Wang 2019-12-25 10:20 ` [virtio-dev] " Jason Wang 2019-12-25 15:20 ` Liu, Jiang 2019-12-26 8:09 ` Jason Wang 2019-12-26 8:09 ` [virtio-dev] " Jason Wang 2019-12-26 12:35 ` Liu, Jiang 2019-12-26 13:16 ` Liu, Jiang 2020-01-02 6:28 ` Jason Wang 2020-01-02 6:28 ` [virtio-dev] " Jason Wang 2020-01-03 6:50 ` Liu, Jiang 2020-01-05 10:42 ` Michael S. Tsirkin 2020-01-05 10:42 ` [virtio-dev] " Michael S. Tsirkin 2020-01-06 7:24 ` Liu, Jing2 2020-01-06 7:24 ` [virtio-dev] " Liu, Jing2 2020-01-09 16:06 ` Liu, Jiang 2020-01-09 16:47 ` Michael S. Tsirkin 2020-01-09 16:47 ` [virtio-dev] " Michael S. Tsirkin 2019-12-25 22:28 ` kbuild test robot 2019-12-25 22:28 ` kbuild test robot 2019-12-26 1:44 ` kbuild test robot 2019-12-26 1:44 ` kbuild test robot 2019-12-26 8:40 ` Jason Wang 2019-12-26 8:40 ` [virtio-dev] " Jason Wang 2019-12-27 9:37 ` Liu, Jing2 2019-12-27 9:37 ` Liu, Jing2 2020-01-02 6:33 ` Jason Wang [this message] 2020-01-02 6:33 ` Jason Wang 2020-01-02 9:13 ` Liu, Jing2 2020-01-02 9:13 ` Liu, Jing2 2020-01-03 3:24 ` Jason Wang 2020-01-03 3:24 ` Jason Wang 2020-01-03 6:14 ` Liu, Jiang 2020-01-03 9:12 ` Jason Wang 2020-01-03 9:12 ` Jason Wang 2020-01-05 11:25 ` Michael S. Tsirkin 2020-01-05 11:25 ` Michael S. Tsirkin 2020-01-06 2:51 ` Jason Wang 2020-01-06 2:51 ` Jason Wang 2020-01-05 11:04 ` Michael S. Tsirkin 2020-01-05 11:04 ` [virtio-dev] " Michael S. Tsirkin 2020-01-09 6:15 ` Liu, Jing2 2020-01-09 6:15 ` [virtio-dev] " Liu, Jing2 2020-01-09 13:26 ` Michael S. Tsirkin 2020-01-09 13:26 ` [virtio-dev] " Michael S. Tsirkin 2020-01-15 7:06 ` Liu, Jing2 2020-01-15 7:06 ` [virtio-dev] " Liu, Jing2 2020-01-21 5:03 ` Liu, Jing2 2020-01-21 5:03 ` Liu, Jing2 2020-01-02 6:55 ` [PATCH v1 0/2] support virtio mmio specification Version 3 Jason Wang 2020-01-02 6:55 ` [virtio-dev] " Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=683cac51-853d-c8c8-24c6-b01886978ca4@redhat.com \ --to=jasowang@redhat.com \ --cc=chao.p.peng@intel.com \ --cc=gerry@linux.alibaba.com \ --cc=jing2.liu@intel.com \ --cc=jing2.liu@linux.intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=slp@redhat.com \ --cc=virtio-dev@lists.oasis-open.org \ --cc=zhabin@linux.alibaba.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.