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

[Jason Wang <jasowang@redhat.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
>>
>