[v2,1/5] virtio-mmio: add notify feature for per-queue

Commit Message

Zha Bin Feb. 10, 2020, 9:05 a.m. UTC

From: Liu Jiang <[e-mail]>

The standard virtio-mmio devices use notification register to signal
backend. This will cause vmexits and slow down the performance when we
passthrough the virtio-mmio devices to guest virtual machines.
We proposed to update virtio over MMIO spec to add the per-queue
notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
configure notify location for each queue.

[1] https://lkml.org/lkml/2020/1/21/31

Signed-off-by: Liu Jiang <[e-mail]>
Co-developed-by: Zha Bin <[e-mail]>
Signed-off-by: Zha Bin <[e-mail]>
Co-developed-by: Jing Liu <[e-mail]>
Signed-off-by: Jing Liu <[e-mail]>
Co-developed-by: Chao Peng <[e-mail]>
Signed-off-by: Chao Peng <[e-mail]>
---
 drivers/virtio/virtio_mmio.c       | 37 +++++++++++++++++++++++++++++++++++--
 include/uapi/linux/virtio_config.h |  8 +++++++-
 2 files changed, 42 insertions(+), 3 deletions(-)

Comments

Michael S. Tsirkin Feb. 11, 2020, 10:50 a.m. UTC | #1

On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
> From: Liu Jiang <[e-mail]>
> 
> The standard virtio-mmio devices use notification register to signal
> backend. This will cause vmexits and slow down the performance when we
> passthrough the virtio-mmio devices to guest virtual machines.
> We proposed to update virtio over MMIO spec to add the per-queue
> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
> configure notify location for each queue.
> 
> [1] https://lkml.org/lkml/2020/1/21/31
> 
> Signed-off-by: Liu Jiang <[e-mail]>
> Co-developed-by: Zha Bin <[e-mail]>
> Signed-off-by: Zha Bin <[e-mail]>
> Co-developed-by: Jing Liu <[e-mail]>
> Signed-off-by: Jing Liu <[e-mail]>
> Co-developed-by: Chao Peng <[e-mail]>
> Signed-off-by: Chao Peng <[e-mail]>

So this is for pass-through for nested virt?
OK but let's split this out please, and benchmark separately.


> ---
>  drivers/virtio/virtio_mmio.c       | 37 +++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/virtio_config.h |  8 +++++++-
>  2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 97d5725..1733ab97 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -90,6 +90,9 @@ struct virtio_mmio_device {
>  	/* a list of queues so we can dispatch IRQs */
>  	spinlock_t lock;
>  	struct list_head virtqueues;
> +
> +	unsigned short notify_base;
> +	unsigned short notify_multiplier;
>  };
>  
>  struct virtio_mmio_vq_info {
> @@ -98,6 +101,9 @@ struct virtio_mmio_vq_info {
>  
>  	/* the list node for the virtqueues list */
>  	struct list_head node;
> +
> +	/* Notify Address*/
> +	unsigned int notify_addr;
>  };
>  
>  
> @@ -119,13 +125,23 @@ static u64 vm_get_features(struct virtio_device *vdev)
>  	return features;
>  }
>  
> +static void vm_transport_features(struct virtio_device *vdev, u64 features)
> +{
> +	if (features & BIT_ULL(VIRTIO_F_MMIO_NOTIFICATION))
> +		__virtio_set_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION);
> +}
> +
>  static int vm_finalize_features(struct virtio_device *vdev)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +	u64 features = vdev->features;
>  
>  	/* Give virtio_ring a chance to accept features. */
>  	vring_transport_features(vdev);
>  
> +	/* Give virtio_mmio a chance to accept features. */
> +	vm_transport_features(vdev, features);
> +
>  	/* Make sure there is are no mixed devices */
>  	if (vm_dev->version == 2 &&
>  			!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> @@ -272,10 +288,13 @@ static void vm_reset(struct virtio_device *vdev)
>  static bool vm_notify(struct virtqueue *vq)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> +	struct virtio_mmio_vq_info *info = vq->priv;
>  
> -	/* We write the queue's selector into the notification register to
> +	/* We write the queue's selector into the Notify Address to
>  	 * signal the other end */
> -	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> +	if (info)
> +		writel(vq->index, vm_dev->base + info->notify_addr);
> +
>  	return true;
>  }
>  
> @@ -434,6 +453,12 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
>  	vq->priv = info;
>  	info->vq = vq;
>  
> +	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION))
> +		info->notify_addr = vm_dev->notify_base +
> +				vm_dev->notify_multiplier * vq->index;
> +	else
> +		info->notify_addr = VIRTIO_MMIO_QUEUE_NOTIFY;
> +
>  	spin_lock_irqsave(&vm_dev->lock, flags);
>  	list_add(&info->node, &vm_dev->virtqueues);
>  	spin_unlock_irqrestore(&vm_dev->lock, flags);
> @@ -471,6 +496,14 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		return irq;
>  	}
>  
> +	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION)) {
> +		unsigned int notify = readl(vm_dev->base +
> +				VIRTIO_MMIO_QUEUE_NOTIFY);
> +
> +		vm_dev->notify_base = notify & 0xffff;
> +		vm_dev->notify_multiplier = (notify >> 16) & 0xffff;
> +	}
> +
>  	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
>  			dev_name(&vdev->dev), vm_dev);
>  	if (err)
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index ff8e7dc..5d93c01 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -52,7 +52,7 @@
>   * rest are per-device feature bits.
>   */
>  #define VIRTIO_TRANSPORT_F_START	28
> -#define VIRTIO_TRANSPORT_F_END		38
> +#define VIRTIO_TRANSPORT_F_END		40
>  
>  #ifndef VIRTIO_CONFIG_NO_LEGACY
>  /* Do we get callbacks when the ring is completely used, even if we've
> @@ -88,4 +88,10 @@
>   * Does the device support Single Root I/O Virtualization?
>   */
>  #define VIRTIO_F_SR_IOV			37
> +
> +/*
> + * This feature indicates the enhanced notification support on MMIO transport
> + * layer.
> + */
> +#define VIRTIO_F_MMIO_NOTIFICATION	39
>  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> -- 
> 1.8.3.1

Michael S. Tsirkin Feb. 11, 2020, 11:33 a.m. UTC | #2

On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
> From: Liu Jiang <[e-mail]>
> 
> The standard virtio-mmio devices use notification register to signal
> backend. This will cause vmexits and slow down the performance when we
> passthrough the virtio-mmio devices to guest virtual machines.
> We proposed to update virtio over MMIO spec to add the per-queue
> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
> configure notify location for each queue.
> 
> [1] https://lkml.org/lkml/2020/1/21/31
> 
> Signed-off-by: Liu Jiang <[e-mail]>
> Co-developed-by: Zha Bin <[e-mail]>
> Signed-off-by: Zha Bin <[e-mail]>
> Co-developed-by: Jing Liu <[e-mail]>
> Signed-off-by: Jing Liu <[e-mail]>
> Co-developed-by: Chao Peng <[e-mail]>
> Signed-off-by: Chao Peng <[e-mail]>


Hmm. Any way to make this static so we don't need
base and multiplier?

> ---
>  drivers/virtio/virtio_mmio.c       | 37 +++++++++++++++++++++++++++++++++++--
>  include/uapi/linux/virtio_config.h |  8 +++++++-
>  2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 97d5725..1733ab97 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -90,6 +90,9 @@ struct virtio_mmio_device {
>  	/* a list of queues so we can dispatch IRQs */
>  	spinlock_t lock;
>  	struct list_head virtqueues;
> +
> +	unsigned short notify_base;
> +	unsigned short notify_multiplier;
>  };
>  
>  struct virtio_mmio_vq_info {
> @@ -98,6 +101,9 @@ struct virtio_mmio_vq_info {
>  
>  	/* the list node for the virtqueues list */
>  	struct list_head node;
> +
> +	/* Notify Address*/
> +	unsigned int notify_addr;
>  };
>  
>  
> @@ -119,13 +125,23 @@ static u64 vm_get_features(struct virtio_device *vdev)
>  	return features;
>  }
>  
> +static void vm_transport_features(struct virtio_device *vdev, u64 features)
> +{
> +	if (features & BIT_ULL(VIRTIO_F_MMIO_NOTIFICATION))
> +		__virtio_set_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION);
> +}
> +
>  static int vm_finalize_features(struct virtio_device *vdev)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> +	u64 features = vdev->features;
>  
>  	/* Give virtio_ring a chance to accept features. */
>  	vring_transport_features(vdev);
>  
> +	/* Give virtio_mmio a chance to accept features. */
> +	vm_transport_features(vdev, features);
> +
>  	/* Make sure there is are no mixed devices */
>  	if (vm_dev->version == 2 &&
>  			!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> @@ -272,10 +288,13 @@ static void vm_reset(struct virtio_device *vdev)
>  static bool vm_notify(struct virtqueue *vq)
>  {
>  	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> +	struct virtio_mmio_vq_info *info = vq->priv;
>  
> -	/* We write the queue's selector into the notification register to
> +	/* We write the queue's selector into the Notify Address to
>  	 * signal the other end */
> -	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> +	if (info)
> +		writel(vq->index, vm_dev->base + info->notify_addr);
> +
>  	return true;
>  }
>  
> @@ -434,6 +453,12 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
>  	vq->priv = info;
>  	info->vq = vq;
>  
> +	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION))
> +		info->notify_addr = vm_dev->notify_base +
> +				vm_dev->notify_multiplier * vq->index;
> +	else
> +		info->notify_addr = VIRTIO_MMIO_QUEUE_NOTIFY;
> +
>  	spin_lock_irqsave(&vm_dev->lock, flags);
>  	list_add(&info->node, &vm_dev->virtqueues);
>  	spin_unlock_irqrestore(&vm_dev->lock, flags);
> @@ -471,6 +496,14 @@ static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		return irq;
>  	}
>  
> +	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION)) {
> +		unsigned int notify = readl(vm_dev->base +
> +				VIRTIO_MMIO_QUEUE_NOTIFY);


that register is documented as:

/* Queue notifier - Write Only */
#define VIRTIO_MMIO_QUEUE_NOTIFY        0x050

so at least you need to update the doc.

> +
> +		vm_dev->notify_base = notify & 0xffff;
> +		vm_dev->notify_multiplier = (notify >> 16) & 0xffff;

are 16 bit base/limit always enough?
In fact won't we be short on 16 bit address space
in a rather short order if queues use up a page
of space at a time?


> +	}
> +
>  	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
>  			dev_name(&vdev->dev), vm_dev);
>  	if (err)
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index ff8e7dc..5d93c01 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -52,7 +52,7 @@
>   * rest are per-device feature bits.
>   */
>  #define VIRTIO_TRANSPORT_F_START	28
> -#define VIRTIO_TRANSPORT_F_END		38
> +#define VIRTIO_TRANSPORT_F_END		40
>  
>  #ifndef VIRTIO_CONFIG_NO_LEGACY
>  /* Do we get callbacks when the ring is completely used, even if we've
> @@ -88,4 +88,10 @@
>   * Does the device support Single Root I/O Virtualization?
>   */
>  #define VIRTIO_F_SR_IOV			37
> +
> +/*
> + * This feature indicates the enhanced notification support on MMIO transport
> + * layer.

Let's replace this with an actual description of the enhancement please
otherwise it will not make sense in a couple of months.

e.g. "Per queue notification address"?


> + */
> +#define VIRTIO_F_MMIO_NOTIFICATION	39
>  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> -- 
> 1.8.3.1

Jason Wang Feb. 12, 2020, 3:39 a.m. UTC | #3

On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
> On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
>> From: Liu Jiang<[e-mail]>
>>
>> The standard virtio-mmio devices use notification register to signal
>> backend. This will cause vmexits and slow down the performance when we
>> passthrough the virtio-mmio devices to guest virtual machines.
>> We proposed to update virtio over MMIO spec to add the per-queue
>> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
>> configure notify location for each queue.
>>
>> [1]https://lkml.org/lkml/2020/1/21/31
>>
>> Signed-off-by: Liu Jiang<[e-mail]>
>> Co-developed-by: Zha Bin<[e-mail]>
>> Signed-off-by: Zha Bin<[e-mail]>
>> Co-developed-by: Jing Liu<[e-mail]>
>> Signed-off-by: Jing Liu<[e-mail]>
>> Co-developed-by: Chao Peng<[e-mail]>
>> Signed-off-by: Chao Peng<[e-mail]>
> Hmm. Any way to make this static so we don't need
> base and multiplier?


E.g page per vq?

Thanks

Michael S. Tsirkin Feb. 12, 2020, 8:18 a.m. UTC | #4

On Wed, Feb 12, 2020 at 11:39:54AM +0800, Jason Wang wrote:
> 
> On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
> > On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
> > > From: Liu Jiang<[e-mail]>
> > > 
> > > The standard virtio-mmio devices use notification register to signal
> > > backend. This will cause vmexits and slow down the performance when we
> > > passthrough the virtio-mmio devices to guest virtual machines.
> > > We proposed to update virtio over MMIO spec to add the per-queue
> > > notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
> > > configure notify location for each queue.
> > > 
> > > [1]https://lkml.org/lkml/2020/1/21/31
> > > 
> > > Signed-off-by: Liu Jiang<[e-mail]>
> > > Co-developed-by: Zha Bin<[e-mail]>
> > > Signed-off-by: Zha Bin<[e-mail]>
> > > Co-developed-by: Jing Liu<[e-mail]>
> > > Signed-off-by: Jing Liu<[e-mail]>
> > > Co-developed-by: Chao Peng<[e-mail]>
> > > Signed-off-by: Chao Peng<[e-mail]>
> > Hmm. Any way to make this static so we don't need
> > base and multiplier?
> 
> 
> E.g page per vq?
> 
> Thanks

Problem is, is page size well defined enough?
Are there cases where guest and host page sizes differ?
I suspect there might be.

But I also think this whole patch is unproven. Is someone actually
working on QEMU code to support pass-trough of virtio-pci
as virtio-mmio for nested guests? What's the performance
gain like?

Jason Wang Feb. 12, 2020, 8:53 a.m. UTC | #5

On 2020/2/12 下午4:18, Michael S. Tsirkin wrote:
> On Wed, Feb 12, 2020 at 11:39:54AM +0800, Jason Wang wrote:
>> On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
>>> On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
>>>> From: Liu Jiang<[e-mail]>
>>>>
>>>> The standard virtio-mmio devices use notification register to signal
>>>> backend. This will cause vmexits and slow down the performance when we
>>>> passthrough the virtio-mmio devices to guest virtual machines.
>>>> We proposed to update virtio over MMIO spec to add the per-queue
>>>> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
>>>> configure notify location for each queue.
>>>>
>>>> [1]https://lkml.org/lkml/2020/1/21/31
>>>>
>>>> Signed-off-by: Liu Jiang<[e-mail]>
>>>> Co-developed-by: Zha Bin<[e-mail]>
>>>> Signed-off-by: Zha Bin<[e-mail]>
>>>> Co-developed-by: Jing Liu<[e-mail]>
>>>> Signed-off-by: Jing Liu<[e-mail]>
>>>> Co-developed-by: Chao Peng<[e-mail]>
>>>> Signed-off-by: Chao Peng<[e-mail]>
>>> Hmm. Any way to make this static so we don't need
>>> base and multiplier?
>> E.g page per vq?
>>
>> Thanks
> Problem is, is page size well defined enough?
> Are there cases where guest and host page sizes differ?
> I suspect there might be.


Right, so it looks better to keep base and multiplier, e.g for vDPA.


>
> But I also think this whole patch is unproven. Is someone actually
> working on QEMU code to support pass-trough of virtio-pci
> as virtio-mmio for nested guests? What's the performance
> gain like?


I don't know.

Thanks


>
> -- MST

Jason Wang Feb. 12, 2020, 9:33 a.m. UTC | #6

On 2020/2/12 下午4:53, Jason Wang wrote:
>
> On 2020/2/12 下午4:18, Michael S. Tsirkin wrote:
>> On Wed, Feb 12, 2020 at 11:39:54AM +0800, Jason Wang wrote:
>>> On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
>>>> On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
>>>>> From: Liu Jiang<[e-mail]>
>>>>>
>>>>> The standard virtio-mmio devices use notification register to signal
>>>>> backend. This will cause vmexits and slow down the performance 
>>>>> when we
>>>>> passthrough the virtio-mmio devices to guest virtual machines.
>>>>> We proposed to update virtio over MMIO spec to add the per-queue
>>>>> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
>>>>> configure notify location for each queue.
>>>>>
>>>>> [1]https://lkml.org/lkml/2020/1/21/31
>>>>>
>>>>> Signed-off-by: Liu Jiang<[e-mail]>
>>>>> Co-developed-by: Zha Bin<[e-mail]>
>>>>> Signed-off-by: Zha Bin<[e-mail]>
>>>>> Co-developed-by: Jing Liu<[e-mail]>
>>>>> Signed-off-by: Jing Liu<[e-mail]>
>>>>> Co-developed-by: Chao Peng<[e-mail]>
>>>>> Signed-off-by: Chao Peng<[e-mail]>
>>>> Hmm. Any way to make this static so we don't need
>>>> base and multiplier?
>>> E.g page per vq?
>>>
>>> Thanks
>> Problem is, is page size well defined enough?
>> Are there cases where guest and host page sizes differ?
>> I suspect there might be.
>
>
> Right, so it looks better to keep base and multiplier, e.g for vDPA.
>
>
>>
>> But I also think this whole patch is unproven. Is someone actually
>> working on QEMU code to support pass-trough of virtio-pci
>> as virtio-mmio for nested guests? What's the performance
>> gain like?
>
>
> I don't know.
>
> Thanks


Btw, I think there's no need for a nested environment to test. Current 
eventfd hook to MSIX should still work for MMIO.

Thanks

Michael S. Tsirkin Feb. 12, 2020, 9:55 a.m. UTC | #7

On Wed, Feb 12, 2020 at 05:33:06PM +0800, Jason Wang wrote:
> 
> On 2020/2/12 下午4:53, Jason Wang wrote:
> > 
> > On 2020/2/12 下午4:18, Michael S. Tsirkin wrote:
> > > On Wed, Feb 12, 2020 at 11:39:54AM +0800, Jason Wang wrote:
> > > > On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
> > > > > On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
> > > > > > From: Liu Jiang<[e-mail]>
> > > > > > 
> > > > > > The standard virtio-mmio devices use notification register to signal
> > > > > > backend. This will cause vmexits and slow down the
> > > > > > performance when we
> > > > > > passthrough the virtio-mmio devices to guest virtual machines.
> > > > > > We proposed to update virtio over MMIO spec to add the per-queue
> > > > > > notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
> > > > > > configure notify location for each queue.
> > > > > > 
> > > > > > [1]https://lkml.org/lkml/2020/1/21/31
> > > > > > 
> > > > > > Signed-off-by: Liu Jiang<[e-mail]>
> > > > > > Co-developed-by: Zha Bin<[e-mail]>
> > > > > > Signed-off-by: Zha Bin<[e-mail]>
> > > > > > Co-developed-by: Jing Liu<[e-mail]>
> > > > > > Signed-off-by: Jing Liu<[e-mail]>
> > > > > > Co-developed-by: Chao Peng<[e-mail]>
> > > > > > Signed-off-by: Chao Peng<[e-mail]>
> > > > > Hmm. Any way to make this static so we don't need
> > > > > base and multiplier?
> > > > E.g page per vq?
> > > > 
> > > > Thanks
> > > Problem is, is page size well defined enough?
> > > Are there cases where guest and host page sizes differ?
> > > I suspect there might be.
> > 
> > 
> > Right, so it looks better to keep base and multiplier, e.g for vDPA.
> > 
> > 
> > > 
> > > But I also think this whole patch is unproven. Is someone actually
> > > working on QEMU code to support pass-trough of virtio-pci
> > > as virtio-mmio for nested guests? What's the performance
> > > gain like?
> > 
> > 
> > I don't know.
> > 
> > Thanks
> 
> 
> Btw, I think there's no need for a nested environment to test. Current
> eventfd hook to MSIX should still work for MMIO.
> 
> Thanks


Oh yes it's the wildcard thingy but how much extra performance does one get
from it with MMIO? A couple % might not be worth the trouble for MMIO.

Jason Wang Feb. 13, 2020, 3:38 a.m. UTC | #8

On 2020/2/12 下午5:55, Michael S. Tsirkin wrote:
> On Wed, Feb 12, 2020 at 05:33:06PM +0800, Jason Wang wrote:
>> On 2020/2/12 下午4:53, Jason Wang wrote:
>>> On 2020/2/12 下午4:18, Michael S. Tsirkin wrote:
>>>> On Wed, Feb 12, 2020 at 11:39:54AM +0800, Jason Wang wrote:
>>>>> On 2020/2/11 下午7:33, Michael S. Tsirkin wrote:
>>>>>> On Mon, Feb 10, 2020 at 05:05:17PM +0800, Zha Bin wrote:
>>>>>>> From: Liu Jiang<[e-mail]>
>>>>>>>
>>>>>>> The standard virtio-mmio devices use notification register to signal
>>>>>>> backend. This will cause vmexits and slow down the
>>>>>>> performance when we
>>>>>>> passthrough the virtio-mmio devices to guest virtual machines.
>>>>>>> We proposed to update virtio over MMIO spec to add the per-queue
>>>>>>> notify feature VIRTIO_F_MMIO_NOTIFICATION[1]. It can allow the VMM to
>>>>>>> configure notify location for each queue.
>>>>>>>
>>>>>>> [1]https://lkml.org/lkml/2020/1/21/31
>>>>>>>
>>>>>>> Signed-off-by: Liu Jiang<[e-mail]>
>>>>>>> Co-developed-by: Zha Bin<[e-mail]>
>>>>>>> Signed-off-by: Zha Bin<[e-mail]>
>>>>>>> Co-developed-by: Jing Liu<[e-mail]>
>>>>>>> Signed-off-by: Jing Liu<[e-mail]>
>>>>>>> Co-developed-by: Chao Peng<[e-mail]>
>>>>>>> Signed-off-by: Chao Peng<[e-mail]>
>>>>>> Hmm. Any way to make this static so we don't need
>>>>>> base and multiplier?
>>>>> E.g page per vq?
>>>>>
>>>>> Thanks
>>>> Problem is, is page size well defined enough?
>>>> Are there cases where guest and host page sizes differ?
>>>> I suspect there might be.
>>>
>>> Right, so it looks better to keep base and multiplier, e.g for vDPA.
>>>
>>>
>>>> But I also think this whole patch is unproven. Is someone actually
>>>> working on QEMU code to support pass-trough of virtio-pci
>>>> as virtio-mmio for nested guests? What's the performance
>>>> gain like?
>>>
>>> I don't know.
>>>
>>> Thanks
>>
>> Btw, I think there's no need for a nested environment to test. Current
>> eventfd hook to MSIX should still work for MMIO.
>>
>> Thanks
>
> Oh yes it's the wildcard thingy but how much extra performance does one get
> from it with MMIO? A couple % might not be worth the trouble for MMIO.


The cover letter have some numbers but I'm not sure whether or not it 
was measured by vhost or other which needs some clarification.

Thanks


>

Patch

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 97d5725..1733ab97 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -90,6 +90,9 @@  struct virtio_mmio_device {
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
 	struct list_head virtqueues;
+
+	unsigned short notify_base;
+	unsigned short notify_multiplier;
 };
 
 struct virtio_mmio_vq_info {
@@ -98,6 +101,9 @@  struct virtio_mmio_vq_info {
 
 	/* the list node for the virtqueues list */
 	struct list_head node;
+
+	/* Notify Address*/
+	unsigned int notify_addr;
 };
 
 
@@ -119,13 +125,23 @@  static u64 vm_get_features(struct virtio_device *vdev)
 	return features;
 }
 
+static void vm_transport_features(struct virtio_device *vdev, u64 features)
+{
+	if (features & BIT_ULL(VIRTIO_F_MMIO_NOTIFICATION))
+		__virtio_set_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION);
+}
+
 static int vm_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
+	u64 features = vdev->features;
 
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
+	/* Give virtio_mmio a chance to accept features. */
+	vm_transport_features(vdev, features);
+
 	/* Make sure there is are no mixed devices */
 	if (vm_dev->version == 2 &&
 			!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
@@ -272,10 +288,13 @@  static void vm_reset(struct virtio_device *vdev)
 static bool vm_notify(struct virtqueue *vq)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
+	struct virtio_mmio_vq_info *info = vq->priv;
 
-	/* We write the queue's selector into the notification register to
+	/* We write the queue's selector into the Notify Address to
 	 * signal the other end */
-	writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+	if (info)
+		writel(vq->index, vm_dev->base + info->notify_addr);
+
 	return true;
 }
 
@@ -434,6 +453,12 @@  static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 	vq->priv = info;
 	info->vq = vq;
 
+	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION))
+		info->notify_addr = vm_dev->notify_base +
+				vm_dev->notify_multiplier * vq->index;
+	else
+		info->notify_addr = VIRTIO_MMIO_QUEUE_NOTIFY;
+
 	spin_lock_irqsave(&vm_dev->lock, flags);
 	list_add(&info->node, &vm_dev->virtqueues);
 	spin_unlock_irqrestore(&vm_dev->lock, flags);
@@ -471,6 +496,14 @@  static int vm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		return irq;
 	}
 
+	if (__virtio_test_bit(vdev, VIRTIO_F_MMIO_NOTIFICATION)) {
+		unsigned int notify = readl(vm_dev->base +
+				VIRTIO_MMIO_QUEUE_NOTIFY);
+
+		vm_dev->notify_base = notify & 0xffff;
+		vm_dev->notify_multiplier = (notify >> 16) & 0xffff;
+	}
+
 	err = request_irq(irq, vm_interrupt, IRQF_SHARED,
 			dev_name(&vdev->dev), vm_dev);
 	if (err)
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index ff8e7dc..5d93c01 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -52,7 +52,7 @@ 
  * rest are per-device feature bits.
  */
 #define VIRTIO_TRANSPORT_F_START	28
-#define VIRTIO_TRANSPORT_F_END		38
+#define VIRTIO_TRANSPORT_F_END		40
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -88,4 +88,10 @@ 
  * Does the device support Single Root I/O Virtualization?
  */
 #define VIRTIO_F_SR_IOV			37
+
+/*
+ * This feature indicates the enhanced notification support on MMIO transport
+ * layer.
+ */
+#define VIRTIO_F_MMIO_NOTIFICATION	39
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */