* [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support @ 2023-03-20 23:21 Viktor Prutyanov 2023-03-21 2:29 ` Jason Wang ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Viktor Prutyanov @ 2023-03-20 23:21 UTC (permalink / raw) To: mst, jasowang Cc: cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan, viktor According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature indicates that the driver passes extra data along with the queue notifications. In a split queue case, the extra data is 16-bit available index. In a packed queue case, the extra data is 1-bit wrap counter and 15-bit available index. Add support for this feature for MMIO and PCI transports. Channel I/O transport will not accept this feature. Signed-off-by: Viktor Prutyanov <viktor@daynix.com> --- v2: reject the feature in virtio_ccw, replace __le32 with u32 drivers/s390/virtio/virtio_ccw.c | 4 +--- drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- drivers/virtio/virtio_pci_common.c | 10 ++++++++++ drivers/virtio/virtio_pci_common.h | 4 ++++ drivers/virtio/virtio_pci_legacy.c | 2 +- drivers/virtio/virtio_pci_modern.c | 2 +- drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ include/linux/virtio_ring.h | 2 ++ include/uapi/linux/virtio_config.h | 6 ++++++ 9 files changed, 56 insertions(+), 6 deletions(-) diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index a10dbe632ef9..d72a59415527 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) static void ccw_transport_features(struct virtio_device *vdev) { - /* - * Currently nothing to do here. - */ + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); } static int virtio_ccw_finalize_features(struct virtio_device *vdev) diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c index 3ff746e3f24a..0e13da17fe0a 100644 --- a/drivers/virtio/virtio_mmio.c +++ b/drivers/virtio/virtio_mmio.c @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) return true; } +static bool vm_notify_with_data(struct virtqueue *vq) +{ + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); + u32 data = vring_fill_notification_data(vq); + + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); + + return true; +} + +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ + ? vm_notify_with_data : vm_notify) + /* Notify all virtqueues on an interrupt. */ static irqreturn_t vm_interrupt(int irq, void *opaque) { @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in /* Create the vring */ vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, - true, true, ctx, vm_notify, callback, name); + true, true, ctx, VM_NOTIFY(vdev), callback, name); if (!vq) { err = -ENOMEM; goto error_new_virtqueue; diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index a6c86f916dbd..535263abc2bd 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) /* we write the queue's selector into the notification register to * signal the other end */ iowrite16(vq->index, (void __iomem *)vq->priv); + + return true; +} + +bool vp_notify_with_data(struct virtqueue *vq) +{ + u32 data = vring_fill_notification_data(vq); + + iowrite32(data, (void __iomem *)vq->priv); + return true; } diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h index 23112d84218f..9a7212dcbb32 100644 --- a/drivers/virtio/virtio_pci_common.h +++ b/drivers/virtio/virtio_pci_common.h @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) void vp_synchronize_vectors(struct virtio_device *vdev); /* the notify function used when creating a virt queue */ bool vp_notify(struct virtqueue *vq); +bool vp_notify_with_data(struct virtqueue *vq); /* the config->del_vqs() implementation */ void vp_del_vqs(struct virtio_device *vdev); /* the config->find_vqs() implementation */ @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs, struct irq_affinity *desc); const char *vp_bus_name(struct virtio_device *vdev); +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ + ? vp_notify : vp_notify_with_data) + /* Setup the affinity for a virtqueue: * - force the affinity for per vq vector * - OR over all affinities for shared MSI diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index 2257f1b3d8ae..b98e994cae48 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, vq = vring_create_virtqueue(index, num, VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, true, false, ctx, - vp_notify, callback, name); + VP_NOTIFY(&vp_dev->vdev), callback, name); if (!vq) return ERR_PTR(-ENOMEM); diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index 9e496e288cfa..7fcd8af5af7e 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -321,7 +321,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, vq = vring_create_virtqueue(index, num, SMP_CACHE_BYTES, &vp_dev->vdev, true, true, ctx, - vp_notify, callback, name); + VP_NOTIFY(&vp_dev->vdev), callback, name); if (!vq) return ERR_PTR(-ENOMEM); diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 723c4e29e1d3..5e9e1800bb6e 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -2699,6 +2699,21 @@ void vring_del_virtqueue(struct virtqueue *_vq) } EXPORT_SYMBOL_GPL(vring_del_virtqueue); +u32 vring_fill_notification_data(struct virtqueue *_vq) +{ + struct vring_virtqueue *vq = to_vvq(_vq); + u16 next; + + if (vq->packed_ring) + next = (vq->packed.next_avail_idx & ~(1 << 15)) | + ((u16)vq->packed.avail_wrap_counter << 15); + else + next = virtio16_to_cpu(_vq->vdev, vq->split.vring.avail->idx); + + return ((u32)next << 16) | _vq->index; +} +EXPORT_SYMBOL_GPL(vring_fill_notification_data); + /* Manipulates transport-specific feature bits. */ void vring_transport_features(struct virtio_device *vdev) { @@ -2718,6 +2733,8 @@ void vring_transport_features(struct virtio_device *vdev) break; case VIRTIO_F_ORDER_PLATFORM: break; + case VIRTIO_F_NOTIFICATION_DATA: + break; default: /* We don't understand this bit. */ __virtio_clear_bit(vdev, i); diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h index 8b8af1a38991..1f65d2f77012 100644 --- a/include/linux/virtio_ring.h +++ b/include/linux/virtio_ring.h @@ -101,4 +101,6 @@ void vring_del_virtqueue(struct virtqueue *vq); void vring_transport_features(struct virtio_device *vdev); irqreturn_t vring_interrupt(int irq, void *_vq); + +u32 vring_fill_notification_data(struct virtqueue *_vq); #endif /* _LINUX_VIRTIO_RING_H */ diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h index 3c05162bc988..2c712c654165 100644 --- a/include/uapi/linux/virtio_config.h +++ b/include/uapi/linux/virtio_config.h @@ -99,6 +99,12 @@ */ #define VIRTIO_F_SR_IOV 37 +/* + * This feature indicates that the driver passes extra data (besides + * identifying the virtqueue) in its device notifications. + */ +#define VIRTIO_F_NOTIFICATION_DATA 38 + /* * This feature indicates that the driver can reset a queue individually. */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-20 23:21 [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support Viktor Prutyanov @ 2023-03-21 2:29 ` Jason Wang 2023-03-21 7:13 ` Michael S. Tsirkin 2023-03-21 9:00 ` Viktor Prutyanov 2023-03-21 9:23 ` Michael S. Tsirkin 2023-03-21 9:27 ` Michael S. Tsirkin 2 siblings, 2 replies; 10+ messages in thread From: Jason Wang @ 2023-03-21 2:29 UTC (permalink / raw) To: Viktor Prutyanov Cc: mst, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for MMIO and PCI transports. Channel I/O > transport will not accept this feature. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > drivers/virtio/virtio_pci_common.h | 4 ++++ > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > include/linux/virtio_ring.h | 2 ++ > include/uapi/linux/virtio_config.h | 6 ++++++ > 9 files changed, 56 insertions(+), 6 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index a10dbe632ef9..d72a59415527 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > static void ccw_transport_features(struct virtio_device *vdev) > { > - /* > - * Currently nothing to do here. > - */ > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); Is there any restriction that prevents us from implementing VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) > } > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..0e13da17fe0a 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + u32 data = vring_fill_notification_data(vq); Can we move this to the initialization? Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 2:29 ` Jason Wang @ 2023-03-21 7:13 ` Michael S. Tsirkin 2023-03-21 9:00 ` Viktor Prutyanov 1 sibling, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2023-03-21 7:13 UTC (permalink / raw) To: Jason Wang Cc: Viktor Prutyanov, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 10:29:11AM +0800, Jason Wang wrote: > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > > indicates that the driver passes extra data along with the queue > > notifications. > > > > In a split queue case, the extra data is 16-bit available index. In a > > packed queue case, the extra data is 1-bit wrap counter and 15-bit > > available index. > > > > Add support for this feature for MMIO and PCI transports. Channel I/O > > transport will not accept this feature. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > > drivers/virtio/virtio_pci_common.h | 4 ++++ > > drivers/virtio/virtio_pci_legacy.c | 2 +- > > drivers/virtio/virtio_pci_modern.c | 2 +- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 9 files changed, 56 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index a10dbe632ef9..d72a59415527 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > > > static void ccw_transport_features(struct virtio_device *vdev) > > { > > - /* > > - * Currently nothing to do here. > > - */ > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > > Is there any restriction that prevents us from implementing > VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) Right, spec actually tells you what to do. > > } > > > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > index 3ff746e3f24a..0e13da17fe0a 100644 > > --- a/drivers/virtio/virtio_mmio.c > > +++ b/drivers/virtio/virtio_mmio.c > > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > > return true; > > } > > > > +static bool vm_notify_with_data(struct virtqueue *vq) > > +{ > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > > + u32 data = vring_fill_notification_data(vq); > > Can we move this to the initialization? > > Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 2:29 ` Jason Wang 2023-03-21 7:13 ` Michael S. Tsirkin @ 2023-03-21 9:00 ` Viktor Prutyanov 2023-03-21 9:07 ` Michael S. Tsirkin 1 sibling, 1 reply; 10+ messages in thread From: Viktor Prutyanov @ 2023-03-21 9:00 UTC (permalink / raw) To: Jason Wang Cc: mst, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 5:29 AM Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > > indicates that the driver passes extra data along with the queue > > notifications. > > > > In a split queue case, the extra data is 16-bit available index. In a > > packed queue case, the extra data is 1-bit wrap counter and 15-bit > > available index. > > > > Add support for this feature for MMIO and PCI transports. Channel I/O > > transport will not accept this feature. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > > drivers/virtio/virtio_pci_common.h | 4 ++++ > > drivers/virtio/virtio_pci_legacy.c | 2 +- > > drivers/virtio/virtio_pci_modern.c | 2 +- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 9 files changed, 56 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index a10dbe632ef9..d72a59415527 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > > > static void ccw_transport_features(struct virtio_device *vdev) > > { > > - /* > > - * Currently nothing to do here. > > - */ > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > > Is there any restriction that prevents us from implementing > VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) Most likely, nothing. > > > } > > > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > index 3ff746e3f24a..0e13da17fe0a 100644 > > --- a/drivers/virtio/virtio_mmio.c > > +++ b/drivers/virtio/virtio_mmio.c > > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > > return true; > > } > > > > +static bool vm_notify_with_data(struct virtqueue *vq) > > +{ > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > > + u32 data = vring_fill_notification_data(vq); > > Can we move this to the initialization? This data is new for each notification, because it helps to identify the next available index. > > Thanks > Thanks, Viktor Prutyanov ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 9:00 ` Viktor Prutyanov @ 2023-03-21 9:07 ` Michael S. Tsirkin 2023-03-21 13:10 ` Cornelia Huck 0 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2023-03-21 9:07 UTC (permalink / raw) To: Viktor Prutyanov Cc: Jason Wang, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 12:00:42PM +0300, Viktor Prutyanov wrote: > On Tue, Mar 21, 2023 at 5:29 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <viktor@daynix.com> wrote: > > > > > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > > > indicates that the driver passes extra data along with the queue > > > notifications. > > > > > > In a split queue case, the extra data is 16-bit available index. In a > > > packed queue case, the extra data is 1-bit wrap counter and 15-bit > > > available index. > > > > > > Add support for this feature for MMIO and PCI transports. Channel I/O > > > transport will not accept this feature. > > > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > > --- > > > > > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > > > > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > > > drivers/virtio/virtio_pci_common.h | 4 ++++ > > > drivers/virtio/virtio_pci_legacy.c | 2 +- > > > drivers/virtio/virtio_pci_modern.c | 2 +- > > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > > include/linux/virtio_ring.h | 2 ++ > > > include/uapi/linux/virtio_config.h | 6 ++++++ > > > 9 files changed, 56 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > index a10dbe632ef9..d72a59415527 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > > > > > static void ccw_transport_features(struct virtio_device *vdev) > > > { > > > - /* > > > - * Currently nothing to do here. > > > - */ > > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > > > > Is there any restriction that prevents us from implementing > > VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) > > Most likely, nothing. So pls code it up. It's the same format. > > > > > } > > > > > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > > index 3ff746e3f24a..0e13da17fe0a 100644 > > > --- a/drivers/virtio/virtio_mmio.c > > > +++ b/drivers/virtio/virtio_mmio.c > > > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > > > return true; > > > } > > > > > > +static bool vm_notify_with_data(struct virtqueue *vq) > > > +{ > > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > > > + u32 data = vring_fill_notification_data(vq); > > > > Can we move this to the initialization? > > This data is new for each notification, because it helps to identify > the next available index. > > > > > Thanks > > > > Thanks, > Viktor Prutyanov ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 9:07 ` Michael S. Tsirkin @ 2023-03-21 13:10 ` Cornelia Huck 0 siblings, 0 replies; 10+ messages in thread From: Cornelia Huck @ 2023-03-21 13:10 UTC (permalink / raw) To: Michael S. Tsirkin, Viktor Prutyanov Cc: Jason Wang, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Mar 21, 2023 at 12:00:42PM +0300, Viktor Prutyanov wrote: >> On Tue, Mar 21, 2023 at 5:29 AM Jason Wang <jasowang@redhat.com> wrote: >> > >> > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <viktor@daynix.com> wrote: >> > > >> > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature >> > > indicates that the driver passes extra data along with the queue >> > > notifications. >> > > >> > > In a split queue case, the extra data is 16-bit available index. In a >> > > packed queue case, the extra data is 1-bit wrap counter and 15-bit >> > > available index. >> > > >> > > Add support for this feature for MMIO and PCI transports. Channel I/O >> > > transport will not accept this feature. >> > > >> > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> >> > > --- >> > > >> > > v2: reject the feature in virtio_ccw, replace __le32 with u32 >> > > >> > > drivers/s390/virtio/virtio_ccw.c | 4 +--- >> > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- >> > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ >> > > drivers/virtio/virtio_pci_common.h | 4 ++++ >> > > drivers/virtio/virtio_pci_legacy.c | 2 +- >> > > drivers/virtio/virtio_pci_modern.c | 2 +- >> > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ >> > > include/linux/virtio_ring.h | 2 ++ >> > > include/uapi/linux/virtio_config.h | 6 ++++++ >> > > 9 files changed, 56 insertions(+), 6 deletions(-) >> > > >> > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c >> > > index a10dbe632ef9..d72a59415527 100644 >> > > --- a/drivers/s390/virtio/virtio_ccw.c >> > > +++ b/drivers/s390/virtio/virtio_ccw.c >> > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) >> > > >> > > static void ccw_transport_features(struct virtio_device *vdev) >> > > { >> > > - /* >> > > - * Currently nothing to do here. >> > > - */ >> > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); >> > >> > Is there any restriction that prevents us from implementing >> > VIRTIO_F_NOTIFICATION_DATA? (Spec seems doesn't limit us from this) >> >> Most likely, nothing. > > So pls code it up. It's the same format. FWIW, the notification data needs to go via the third parameter of kvm_hypercall3() in virtio_ccw_kvm_notify(). ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-20 23:21 [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support Viktor Prutyanov 2023-03-21 2:29 ` Jason Wang @ 2023-03-21 9:23 ` Michael S. Tsirkin 2023-03-21 11:49 ` Viktor Prutyanov 2023-03-21 9:27 ` Michael S. Tsirkin 2 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2023-03-21 9:23 UTC (permalink / raw) To: Viktor Prutyanov Cc: jasowang, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 02:21:15AM +0300, Viktor Prutyanov wrote: > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for MMIO and PCI transports. Channel I/O > transport will not accept this feature. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > drivers/virtio/virtio_pci_common.h | 4 ++++ > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > include/linux/virtio_ring.h | 2 ++ > include/uapi/linux/virtio_config.h | 6 ++++++ > 9 files changed, 56 insertions(+), 6 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index a10dbe632ef9..d72a59415527 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > static void ccw_transport_features(struct virtio_device *vdev) > { > - /* > - * Currently nothing to do here. > - */ > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > } > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..0e13da17fe0a 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + u32 data = vring_fill_notification_data(vq); > + > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > + > + return true; > +} > + > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > + ? vm_notify_with_data : vm_notify) > + > /* Notify all virtqueues on an interrupt. */ > static irqreturn_t vm_interrupt(int irq, void *opaque) > { > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in > > /* Create the vring */ > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > - true, true, ctx, vm_notify, callback, name); > + true, true, ctx, VM_NOTIFY(vdev), callback, name); I don't see why is this macro useful. > if (!vq) { > err = -ENOMEM; > goto error_new_virtqueue; > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index a6c86f916dbd..535263abc2bd 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > /* we write the queue's selector into the notification register to > * signal the other end */ > iowrite16(vq->index, (void __iomem *)vq->priv); > + > + return true; > +} > + > +bool vp_notify_with_data(struct virtqueue *vq) > +{ > + u32 data = vring_fill_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > return true; > } > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > index 23112d84218f..9a7212dcbb32 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) > void vp_synchronize_vectors(struct virtio_device *vdev); > /* the notify function used when creating a virt queue */ > bool vp_notify(struct virtqueue *vq); > +bool vp_notify_with_data(struct virtqueue *vq); > /* the config->del_vqs() implementation */ > void vp_del_vqs(struct virtio_device *vdev); > /* the config->find_vqs() implementation */ > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs, > struct irq_affinity *desc); > const char *vp_bus_name(struct virtio_device *vdev); > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > + ? vp_notify : vp_notify_with_data) > + > /* Setup the affinity for a virtqueue: > * - force the affinity for per vq vector > * - OR over all affinities for shared MSI > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > index 2257f1b3d8ae..b98e994cae48 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > true, false, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 9e496e288cfa..7fcd8af5af7e 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -321,7 +321,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > SMP_CACHE_BYTES, &vp_dev->vdev, > true, true, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 723c4e29e1d3..5e9e1800bb6e 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -2699,6 +2699,21 @@ void vring_del_virtqueue(struct virtqueue *_vq) > } > EXPORT_SYMBOL_GPL(vring_del_virtqueue); > > +u32 vring_fill_notification_data(struct virtqueue *_vq) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + u16 next; > + > + if (vq->packed_ring) > + next = (vq->packed.next_avail_idx & ~(1 << 15)) | > + ((u16)vq->packed.avail_wrap_counter << 15); I don't think the cast is needed. Neither is () around << the second << here (first is needed because gcc chooses to complain: apparently it considers bitwise and a math operation for some obscure reason). > + else > + next = virtio16_to_cpu(_vq->vdev, vq->split.vring.avail->idx); > + > + return ((u32)next << 16) | _vq->index; I don't think the cast is needed. Neither is () around << needed. > +} > +EXPORT_SYMBOL_GPL(vring_fill_notification_data); > + I'd inline this - it's on data path ... > /* Manipulates transport-specific feature bits. */ > void vring_transport_features(struct virtio_device *vdev) > { > @@ -2718,6 +2733,8 @@ void vring_transport_features(struct virtio_device *vdev) > break; > case VIRTIO_F_ORDER_PLATFORM: > break; > + case VIRTIO_F_NOTIFICATION_DATA: > + break; > default: > /* We don't understand this bit. */ > __virtio_clear_bit(vdev, i); > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > index 8b8af1a38991..1f65d2f77012 100644 > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -101,4 +101,6 @@ void vring_del_virtqueue(struct virtqueue *vq); > void vring_transport_features(struct virtio_device *vdev); > > irqreturn_t vring_interrupt(int irq, void *_vq); > + > +u32 vring_fill_notification_data(struct virtqueue *_vq); > #endif /* _LINUX_VIRTIO_RING_H */ > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 3c05162bc988..2c712c654165 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.h > @@ -99,6 +99,12 @@ > */ > #define VIRTIO_F_SR_IOV 37 > > +/* > + * This feature indicates that the driver passes extra data (besides > + * identifying the virtqueue) in its device notifications. > + */ > +#define VIRTIO_F_NOTIFICATION_DATA 38 > + > /* > * This feature indicates that the driver can reset a queue individually. > */ > -- > 2.35.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 9:23 ` Michael S. Tsirkin @ 2023-03-21 11:49 ` Viktor Prutyanov 2023-03-21 12:21 ` Michael S. Tsirkin 0 siblings, 1 reply; 10+ messages in thread From: Viktor Prutyanov @ 2023-03-21 11:49 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 12:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Mar 21, 2023 at 02:21:15AM +0300, Viktor Prutyanov wrote: > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > > indicates that the driver passes extra data along with the queue > > notifications. > > > > In a split queue case, the extra data is 16-bit available index. In a > > packed queue case, the extra data is 1-bit wrap counter and 15-bit > > available index. > > > > Add support for this feature for MMIO and PCI transports. Channel I/O > > transport will not accept this feature. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > > drivers/virtio/virtio_pci_common.h | 4 ++++ > > drivers/virtio/virtio_pci_legacy.c | 2 +- > > drivers/virtio/virtio_pci_modern.c | 2 +- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 9 files changed, 56 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index a10dbe632ef9..d72a59415527 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > > > static void ccw_transport_features(struct virtio_device *vdev) > > { > > - /* > > - * Currently nothing to do here. > > - */ > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > > } > > > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > index 3ff746e3f24a..0e13da17fe0a 100644 > > --- a/drivers/virtio/virtio_mmio.c > > +++ b/drivers/virtio/virtio_mmio.c > > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > > return true; > > } > > > > +static bool vm_notify_with_data(struct virtqueue *vq) > > +{ > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > > + u32 data = vring_fill_notification_data(vq); > > + > > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > > + > > + return true; > > +} > > + > > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > > + ? vm_notify_with_data : vm_notify) > > + > > /* Notify all virtqueues on an interrupt. */ > > static irqreturn_t vm_interrupt(int irq, void *opaque) > > { > > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in > > > > /* Create the vring */ > > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > > - true, true, ctx, vm_notify, callback, name); > > + true, true, ctx, VM_NOTIFY(vdev), callback, name); > > I don't see why is this macro useful. > > > > > if (!vq) { > > err = -ENOMEM; > > goto error_new_virtqueue; > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > > index a6c86f916dbd..535263abc2bd 100644 > > --- a/drivers/virtio/virtio_pci_common.c > > +++ b/drivers/virtio/virtio_pci_common.c > > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > > /* we write the queue's selector into the notification register to > > * signal the other end */ > > iowrite16(vq->index, (void __iomem *)vq->priv); > > + > > + return true; > > +} > > + > > +bool vp_notify_with_data(struct virtqueue *vq) > > +{ > > + u32 data = vring_fill_notification_data(vq); > > + > > + iowrite32(data, (void __iomem *)vq->priv); > > + > > return true; > > } > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > > index 23112d84218f..9a7212dcbb32 100644 > > --- a/drivers/virtio/virtio_pci_common.h > > +++ b/drivers/virtio/virtio_pci_common.h > > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) > > void vp_synchronize_vectors(struct virtio_device *vdev); > > /* the notify function used when creating a virt queue */ > > bool vp_notify(struct virtqueue *vq); > > +bool vp_notify_with_data(struct virtqueue *vq); > > /* the config->del_vqs() implementation */ > > void vp_del_vqs(struct virtio_device *vdev); > > /* the config->find_vqs() implementation */ > > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs, > > struct irq_affinity *desc); > > const char *vp_bus_name(struct virtio_device *vdev); > > > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > > + ? vp_notify : vp_notify_with_data) > > + > > /* Setup the affinity for a virtqueue: > > * - force the affinity for per vq vector > > * - OR over all affinities for shared MSI > > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > > index 2257f1b3d8ae..b98e994cae48 100644 > > --- a/drivers/virtio/virtio_pci_legacy.c > > +++ b/drivers/virtio/virtio_pci_legacy.c > > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > vq = vring_create_virtqueue(index, num, > > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > > true, false, ctx, > > - vp_notify, callback, name); > > + VP_NOTIFY(&vp_dev->vdev), callback, name); > > if (!vq) > > return ERR_PTR(-ENOMEM); > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 9e496e288cfa..7fcd8af5af7e 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -321,7 +321,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > vq = vring_create_virtqueue(index, num, > > SMP_CACHE_BYTES, &vp_dev->vdev, > > true, true, ctx, > > - vp_notify, callback, name); > > + VP_NOTIFY(&vp_dev->vdev), callback, name); > > if (!vq) > > return ERR_PTR(-ENOMEM); > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 723c4e29e1d3..5e9e1800bb6e 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -2699,6 +2699,21 @@ void vring_del_virtqueue(struct virtqueue *_vq) > > } > > EXPORT_SYMBOL_GPL(vring_del_virtqueue); > > > > +u32 vring_fill_notification_data(struct virtqueue *_vq) > > +{ > > + struct vring_virtqueue *vq = to_vvq(_vq); > > + u16 next; > > + > > + if (vq->packed_ring) > > + next = (vq->packed.next_avail_idx & ~(1 << 15)) | > > + ((u16)vq->packed.avail_wrap_counter << 15); > > I don't think the cast is needed. Neither is () around << the second << > here (first is needed because gcc chooses to complain: apparently it > considers bitwise and a math operation for some obscure reason). > > > + else > > + next = virtio16_to_cpu(_vq->vdev, vq->split.vring.avail->idx); > > + > > + return ((u32)next << 16) | _vq->index; > > I don't think the cast is needed. Neither is () around << needed. > > > +} > > +EXPORT_SYMBOL_GPL(vring_fill_notification_data); > > + > > I'd inline this - it's on data path ... As far as I see, to be inlined in virtio_mmio.c, virtio_pci_common.c and virtio_ccw.c, the function should be defined in some header, but definitions such as vring_virtqueue, vring_virtqueue_split, vring_virtqueue_packed will not be available, because they are in virtio_ring.c. Looks like, they must be moved to a separate header in this case, isn't it? > > > /* Manipulates transport-specific feature bits. */ > > void vring_transport_features(struct virtio_device *vdev) > > { > > > @@ -2718,6 +2733,8 @@ void vring_transport_features(struct virtio_device *vdev) > > break; > > case VIRTIO_F_ORDER_PLATFORM: > > break; > > + case VIRTIO_F_NOTIFICATION_DATA: > > + break; > > default: > > /* We don't understand this bit. */ > > __virtio_clear_bit(vdev, i); > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > index 8b8af1a38991..1f65d2f77012 100644 > > --- a/include/linux/virtio_ring.h > > +++ b/include/linux/virtio_ring.h > > @@ -101,4 +101,6 @@ void vring_del_virtqueue(struct virtqueue *vq); > > void vring_transport_features(struct virtio_device *vdev); > > > > irqreturn_t vring_interrupt(int irq, void *_vq); > > + > > +u32 vring_fill_notification_data(struct virtqueue *_vq); > > #endif /* _LINUX_VIRTIO_RING_H */ > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > > index 3c05162bc988..2c712c654165 100644 > > --- a/include/uapi/linux/virtio_config.h > > +++ b/include/uapi/linux/virtio_config.h > > @@ -99,6 +99,12 @@ > > */ > > #define VIRTIO_F_SR_IOV 37 > > > > +/* > > + * This feature indicates that the driver passes extra data (besides > > + * identifying the virtqueue) in its device notifications. > > + */ > > +#define VIRTIO_F_NOTIFICATION_DATA 38 > > + > > /* > > * This feature indicates that the driver can reset a queue individually. > > */ > > -- > > 2.35.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-21 11:49 ` Viktor Prutyanov @ 2023-03-21 12:21 ` Michael S. Tsirkin 0 siblings, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2023-03-21 12:21 UTC (permalink / raw) To: Viktor Prutyanov Cc: jasowang, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 02:49:13PM +0300, Viktor Prutyanov wrote: > On Tue, Mar 21, 2023 at 12:23 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, Mar 21, 2023 at 02:21:15AM +0300, Viktor Prutyanov wrote: > > > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > > > indicates that the driver passes extra data along with the queue > > > notifications. > > > > > > In a split queue case, the extra data is 16-bit available index. In a > > > packed queue case, the extra data is 1-bit wrap counter and 15-bit > > > available index. > > > > > > Add support for this feature for MMIO and PCI transports. Channel I/O > > > transport will not accept this feature. > > > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > > --- > > > > > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > > > > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > > > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > > > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > > > drivers/virtio/virtio_pci_common.h | 4 ++++ > > > drivers/virtio/virtio_pci_legacy.c | 2 +- > > > drivers/virtio/virtio_pci_modern.c | 2 +- > > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > > include/linux/virtio_ring.h | 2 ++ > > > include/uapi/linux/virtio_config.h | 6 ++++++ > > > 9 files changed, 56 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > index a10dbe632ef9..d72a59415527 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > > > > > static void ccw_transport_features(struct virtio_device *vdev) > > > { > > > - /* > > > - * Currently nothing to do here. > > > - */ > > > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > > > } > > > > > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > > index 3ff746e3f24a..0e13da17fe0a 100644 > > > --- a/drivers/virtio/virtio_mmio.c > > > +++ b/drivers/virtio/virtio_mmio.c > > > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > > > return true; > > > } > > > > > > +static bool vm_notify_with_data(struct virtqueue *vq) > > > +{ > > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > > > + u32 data = vring_fill_notification_data(vq); > > > + > > > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > > > + > > > + return true; > > > +} > > > + > > > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > > > + ? vm_notify_with_data : vm_notify) > > > + > > > /* Notify all virtqueues on an interrupt. */ > > > static irqreturn_t vm_interrupt(int irq, void *opaque) > > > { > > > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in > > > > > > /* Create the vring */ > > > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > > > - true, true, ctx, vm_notify, callback, name); > > > + true, true, ctx, VM_NOTIFY(vdev), callback, name); > > > > I don't see why is this macro useful. > > > > > > > > > if (!vq) { > > > err = -ENOMEM; > > > goto error_new_virtqueue; > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > > > index a6c86f916dbd..535263abc2bd 100644 > > > --- a/drivers/virtio/virtio_pci_common.c > > > +++ b/drivers/virtio/virtio_pci_common.c > > > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > > > /* we write the queue's selector into the notification register to > > > * signal the other end */ > > > iowrite16(vq->index, (void __iomem *)vq->priv); > > > + > > > + return true; > > > +} > > > + > > > +bool vp_notify_with_data(struct virtqueue *vq) > > > +{ > > > + u32 data = vring_fill_notification_data(vq); > > > + > > > + iowrite32(data, (void __iomem *)vq->priv); > > > + > > > return true; > > > } > > > > > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > > > index 23112d84218f..9a7212dcbb32 100644 > > > --- a/drivers/virtio/virtio_pci_common.h > > > +++ b/drivers/virtio/virtio_pci_common.h > > > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) > > > void vp_synchronize_vectors(struct virtio_device *vdev); > > > /* the notify function used when creating a virt queue */ > > > bool vp_notify(struct virtqueue *vq); > > > +bool vp_notify_with_data(struct virtqueue *vq); > > > /* the config->del_vqs() implementation */ > > > void vp_del_vqs(struct virtio_device *vdev); > > > /* the config->find_vqs() implementation */ > > > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs, > > > struct irq_affinity *desc); > > > const char *vp_bus_name(struct virtio_device *vdev); > > > > > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > > > + ? vp_notify : vp_notify_with_data) > > > + > > > /* Setup the affinity for a virtqueue: > > > * - force the affinity for per vq vector > > > * - OR over all affinities for shared MSI > > > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > > > index 2257f1b3d8ae..b98e994cae48 100644 > > > --- a/drivers/virtio/virtio_pci_legacy.c > > > +++ b/drivers/virtio/virtio_pci_legacy.c > > > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > vq = vring_create_virtqueue(index, num, > > > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > > > true, false, ctx, > > > - vp_notify, callback, name); > > > + VP_NOTIFY(&vp_dev->vdev), callback, name); > > > if (!vq) > > > return ERR_PTR(-ENOMEM); > > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > > > index 9e496e288cfa..7fcd8af5af7e 100644 > > > --- a/drivers/virtio/virtio_pci_modern.c > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > @@ -321,7 +321,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > vq = vring_create_virtqueue(index, num, > > > SMP_CACHE_BYTES, &vp_dev->vdev, > > > true, true, ctx, > > > - vp_notify, callback, name); > > > + VP_NOTIFY(&vp_dev->vdev), callback, name); > > > if (!vq) > > > return ERR_PTR(-ENOMEM); > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 723c4e29e1d3..5e9e1800bb6e 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -2699,6 +2699,21 @@ void vring_del_virtqueue(struct virtqueue *_vq) > > > } > > > EXPORT_SYMBOL_GPL(vring_del_virtqueue); > > > > > > +u32 vring_fill_notification_data(struct virtqueue *_vq) > > > +{ > > > + struct vring_virtqueue *vq = to_vvq(_vq); > > > + u16 next; > > > + > > > + if (vq->packed_ring) > > > + next = (vq->packed.next_avail_idx & ~(1 << 15)) | > > > + ((u16)vq->packed.avail_wrap_counter << 15); > > > > I don't think the cast is needed. Neither is () around << the second << > > here (first is needed because gcc chooses to complain: apparently it > > considers bitwise and a math operation for some obscure reason). > > > > > + else > > > + next = virtio16_to_cpu(_vq->vdev, vq->split.vring.avail->idx); > > > + > > > + return ((u32)next << 16) | _vq->index; > > > > I don't think the cast is needed. Neither is () around << needed. > > > > > +} > > > +EXPORT_SYMBOL_GPL(vring_fill_notification_data); > > > + > > > > I'd inline this - it's on data path ... > > As far as I see, to be inlined in virtio_mmio.c, virtio_pci_common.c > and virtio_ccw.c, the function should be defined in some header, but > definitions such as vring_virtqueue, vring_virtqueue_split, > vring_virtqueue_packed will not be available, because they are in > virtio_ring.c. Looks like, they must be moved to a separate header > in this case, isn't it? Oh you are right. OK, sorry. > > > > > /* Manipulates transport-specific feature bits. */ > > > void vring_transport_features(struct virtio_device *vdev) > > > { > > > > > @@ -2718,6 +2733,8 @@ void vring_transport_features(struct virtio_device *vdev) > > > break; > > > case VIRTIO_F_ORDER_PLATFORM: > > > break; > > > + case VIRTIO_F_NOTIFICATION_DATA: > > > + break; > > > default: > > > /* We don't understand this bit. */ > > > __virtio_clear_bit(vdev, i); > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > > index 8b8af1a38991..1f65d2f77012 100644 > > > --- a/include/linux/virtio_ring.h > > > +++ b/include/linux/virtio_ring.h > > > @@ -101,4 +101,6 @@ void vring_del_virtqueue(struct virtqueue *vq); > > > void vring_transport_features(struct virtio_device *vdev); > > > > > > irqreturn_t vring_interrupt(int irq, void *_vq); > > > + > > > +u32 vring_fill_notification_data(struct virtqueue *_vq); > > > #endif /* _LINUX_VIRTIO_RING_H */ > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > > > index 3c05162bc988..2c712c654165 100644 > > > --- a/include/uapi/linux/virtio_config.h > > > +++ b/include/uapi/linux/virtio_config.h > > > @@ -99,6 +99,12 @@ > > > */ > > > #define VIRTIO_F_SR_IOV 37 > > > > > > +/* > > > + * This feature indicates that the driver passes extra data (besides > > > + * identifying the virtqueue) in its device notifications. > > > + */ > > > +#define VIRTIO_F_NOTIFICATION_DATA 38 > > > + > > > /* > > > * This feature indicates that the driver can reset a queue individually. > > > */ > > > -- > > > 2.35.1 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support 2023-03-20 23:21 [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support Viktor Prutyanov 2023-03-21 2:29 ` Jason Wang 2023-03-21 9:23 ` Michael S. Tsirkin @ 2023-03-21 9:27 ` Michael S. Tsirkin 2 siblings, 0 replies; 10+ messages in thread From: Michael S. Tsirkin @ 2023-03-21 9:27 UTC (permalink / raw) To: Viktor Prutyanov Cc: jasowang, cohuck, pasic, farman, linux-s390, kvm, virtualization, linux-kernel, yan On Tue, Mar 21, 2023 at 02:21:15AM +0300, Viktor Prutyanov wrote: > According to VirtIO spec v1.2, VIRTIO_F_NOTIFICATION_DATA feature > indicates that the driver passes extra data along with the queue > notifications. > > In a split queue case, the extra data is 16-bit available index. In a > packed queue case, the extra data is 1-bit wrap counter and 15-bit > available index. > > Add support for this feature for MMIO and PCI transports. Channel I/O > transport will not accept this feature. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > > v2: reject the feature in virtio_ccw, replace __le32 with u32 > > drivers/s390/virtio/virtio_ccw.c | 4 +--- > drivers/virtio/virtio_mmio.c | 15 ++++++++++++++- > drivers/virtio/virtio_pci_common.c | 10 ++++++++++ > drivers/virtio/virtio_pci_common.h | 4 ++++ > drivers/virtio/virtio_pci_legacy.c | 2 +- > drivers/virtio/virtio_pci_modern.c | 2 +- > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > include/linux/virtio_ring.h | 2 ++ > include/uapi/linux/virtio_config.h | 6 ++++++ > 9 files changed, 56 insertions(+), 6 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index a10dbe632ef9..d72a59415527 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -789,9 +789,7 @@ static u64 virtio_ccw_get_features(struct virtio_device *vdev) > > static void ccw_transport_features(struct virtio_device *vdev) > { > - /* > - * Currently nothing to do here. > - */ > + __virtio_clear_bit(vdev, VIRTIO_F_NOTIFICATION_DATA); > } > > static int virtio_ccw_finalize_features(struct virtio_device *vdev) > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > index 3ff746e3f24a..0e13da17fe0a 100644 > --- a/drivers/virtio/virtio_mmio.c > +++ b/drivers/virtio/virtio_mmio.c > @@ -285,6 +285,19 @@ static bool vm_notify(struct virtqueue *vq) > return true; > } > > +static bool vm_notify_with_data(struct virtqueue *vq) > +{ > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev); > + u32 data = vring_fill_notification_data(vq); > + > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY); > + > + return true; > +} > + > +#define VM_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > + ? vm_notify_with_data : vm_notify) > + > /* Notify all virtqueues on an interrupt. */ > static irqreturn_t vm_interrupt(int irq, void *opaque) > { > @@ -397,7 +410,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in > > /* Create the vring */ > vq = vring_create_virtqueue(index, num, VIRTIO_MMIO_VRING_ALIGN, vdev, > - true, true, ctx, vm_notify, callback, name); > + true, true, ctx, VM_NOTIFY(vdev), callback, name); > if (!vq) { > err = -ENOMEM; > goto error_new_virtqueue; > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index a6c86f916dbd..535263abc2bd 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -43,6 +43,16 @@ bool vp_notify(struct virtqueue *vq) > /* we write the queue's selector into the notification register to > * signal the other end */ > iowrite16(vq->index, (void __iomem *)vq->priv); > + > + return true; > +} > + > +bool vp_notify_with_data(struct virtqueue *vq) > +{ > + u32 data = vring_fill_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > return true; > } > > diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h > index 23112d84218f..9a7212dcbb32 100644 > --- a/drivers/virtio/virtio_pci_common.h > +++ b/drivers/virtio/virtio_pci_common.h > @@ -105,6 +105,7 @@ static struct virtio_pci_device *to_vp_device(struct virtio_device *vdev) > void vp_synchronize_vectors(struct virtio_device *vdev); > /* the notify function used when creating a virt queue */ > bool vp_notify(struct virtqueue *vq); > +bool vp_notify_with_data(struct virtqueue *vq); > /* the config->del_vqs() implementation */ > void vp_del_vqs(struct virtio_device *vdev); > /* the config->find_vqs() implementation */ > @@ -114,6 +115,9 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs, > struct irq_affinity *desc); > const char *vp_bus_name(struct virtio_device *vdev); > > +#define VP_NOTIFY(vdev) (__virtio_test_bit((vdev), VIRTIO_F_NOTIFICATION_DATA) \ > + ? vp_notify : vp_notify_with_data) > + > /* Setup the affinity for a virtqueue: > * - force the affinity for per vq vector > * - OR over all affinities for shared MSI > diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c > index 2257f1b3d8ae..b98e994cae48 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -131,7 +131,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > VIRTIO_PCI_VRING_ALIGN, &vp_dev->vdev, > true, false, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c > index 9e496e288cfa..7fcd8af5af7e 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -321,7 +321,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > vq = vring_create_virtqueue(index, num, > SMP_CACHE_BYTES, &vp_dev->vdev, > true, true, ctx, > - vp_notify, callback, name); > + VP_NOTIFY(&vp_dev->vdev), callback, name); > if (!vq) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 723c4e29e1d3..5e9e1800bb6e 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -2699,6 +2699,21 @@ void vring_del_virtqueue(struct virtqueue *_vq) > } > EXPORT_SYMBOL_GPL(vring_del_virtqueue); > > +u32 vring_fill_notification_data(struct virtqueue *_vq) btw what does "fill" mean here? why not just vring_notification_data? > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + u16 next; > + > + if (vq->packed_ring) > + next = (vq->packed.next_avail_idx & ~(1 << 15)) | > + ((u16)vq->packed.avail_wrap_counter << 15); > + else > + next = virtio16_to_cpu(_vq->vdev, vq->split.vring.avail->idx); BTW it's annoying to poke at avail->idx here - will cause a bunch of cache misses. And the byte-swap is a waste. Isn't this why we have avail_idx_shadow? > + > + return ((u32)next << 16) | _vq->index; > +} > +EXPORT_SYMBOL_GPL(vring_fill_notification_data); > + > /* Manipulates transport-specific feature bits. */ > void vring_transport_features(struct virtio_device *vdev) > { > @@ -2718,6 +2733,8 @@ void vring_transport_features(struct virtio_device *vdev) > break; > case VIRTIO_F_ORDER_PLATFORM: > break; > + case VIRTIO_F_NOTIFICATION_DATA: > + break; > default: > /* We don't understand this bit. */ > __virtio_clear_bit(vdev, i); > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > index 8b8af1a38991..1f65d2f77012 100644 > --- a/include/linux/virtio_ring.h > +++ b/include/linux/virtio_ring.h > @@ -101,4 +101,6 @@ void vring_del_virtqueue(struct virtqueue *vq); > void vring_transport_features(struct virtio_device *vdev); > > irqreturn_t vring_interrupt(int irq, void *_vq); > + > +u32 vring_fill_notification_data(struct virtqueue *_vq); > #endif /* _LINUX_VIRTIO_RING_H */ > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h > index 3c05162bc988..2c712c654165 100644 > --- a/include/uapi/linux/virtio_config.h > +++ b/include/uapi/linux/virtio_config.h > @@ -99,6 +99,12 @@ > */ > #define VIRTIO_F_SR_IOV 37 > > +/* > + * This feature indicates that the driver passes extra data (besides > + * identifying the virtqueue) in its device notifications. > + */ > +#define VIRTIO_F_NOTIFICATION_DATA 38 > + > /* > * This feature indicates that the driver can reset a queue individually. > */ > -- > 2.35.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-21 13:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-20 23:21 [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support Viktor Prutyanov 2023-03-21 2:29 ` Jason Wang 2023-03-21 7:13 ` Michael S. Tsirkin 2023-03-21 9:00 ` Viktor Prutyanov 2023-03-21 9:07 ` Michael S. Tsirkin 2023-03-21 13:10 ` Cornelia Huck 2023-03-21 9:23 ` Michael S. Tsirkin 2023-03-21 11:49 ` Viktor Prutyanov 2023-03-21 12:21 ` Michael S. Tsirkin 2023-03-21 9:27 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).