2023-03-20 23:21:33

by Viktor Prutyanov

Subject: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

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 <[email protected]>
---

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

2023-03-21 02:30:16

by Jason Wang

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <[email protected]> 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 <[email protected]>
> ---
>
> 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

2023-03-21 07:14:53

by Michael S. Tsirkin

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21, 2023 at 10:29:11AM +0800, Jason Wang wrote:
> On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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

2023-03-21 09:02:54

by Viktor Prutyanov

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21, 2023 at 5:29 AM Jason Wang <[email protected]> wrote:
>
> On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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

2023-03-21 09:08:40

by Michael S. Tsirkin

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21, 2023 at 12:00:42PM +0300, Viktor Prutyanov wrote:
> On Tue, Mar 21, 2023 at 5:29 AM Jason Wang <[email protected]> wrote:
> >
> > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <[email protected]> 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 <[email protected]>
> > > ---
> > >
> > > 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

2023-03-21 09:24:15

by Michael S. Tsirkin

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

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 <[email protected]>
> ---
>
> 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

2023-03-21 09:28:36

by Michael S. Tsirkin

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

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 <[email protected]>
> ---
>
> 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

2023-03-21 11:49:48

by Viktor Prutyanov

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21, 2023 at 12:23 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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
>

2023-03-21 12:22:38

by Michael S. Tsirkin

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

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 <[email protected]> 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 <[email protected]>
> > > ---
> > >
> > > 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
> >

2023-03-21 13:11:21

by Cornelia Huck

Subject: Re: [PATCH v2] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support

On Tue, Mar 21 2023, "Michael S. Tsirkin" <[email protected]> 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 <[email protected]> wrote:
>> >
>> > On Tue, Mar 21, 2023 at 7:21 AM Viktor Prutyanov <[email protected]> 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 <[email protected]>
>> > > ---
>> > >
>> > > 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().