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, PCI and channel I/O transports.
Signed-off-by: Viktor Prutyanov <[email protected]>
---
v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
remove byte swap, rename to vring_notification_data
v2: reject the feature in virtio_ccw, replace __le32 with u32
drivers/s390/virtio/virtio_ccw.c | 4 +++-
drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
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, 57 insertions(+), 4 deletions(-)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 954fc31b4bc7..c33172c5b8d5 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
struct virtio_ccw_vq_info *info = vq->priv;
struct virtio_ccw_device *vcdev;
struct subchannel_id schid;
+ u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
+ vring_notification_data(vq) : vq->index;
vcdev = to_vc_device(info->vq->vdev);
ccw_device_get_schid(vcdev->cdev, &schid);
BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
*((unsigned int *)&schid),
- vq->index, info->cookie);
+ data, info->cookie);
if (info->cookie < 0)
return false;
return true;
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 3ff746e3f24a..7c16e622c33d 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -285,6 +285,16 @@ 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_notification_data(vq);
+
+ writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
+
+ return true;
+}
+
/* Notify all virtqueues on an interrupt. */
static irqreturn_t vm_interrupt(int irq, void *opaque)
{
@@ -368,6 +378,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
unsigned long flags;
unsigned int num;
int err;
+ bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev,
+ VIRTIO_F_NOTIFICATION_DATA) ? vm_notify_with_data : vm_notify;
if (!name)
return NULL;
@@ -397,7 +409,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, notify, 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..e915c22f2384 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_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 4c3bb0ddeb9b..837875cc3190 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2752,6 +2752,21 @@ void vring_del_virtqueue(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(vring_del_virtqueue);
+u32 vring_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)) |
+ vq->packed.avail_wrap_counter << 15;
+ else
+ next = vq->split.avail_idx_shadow;
+
+ return next << 16 | _vq->index;
+}
+EXPORT_SYMBOL_GPL(vring_notification_data);
+
/* Manipulates transport-specific feature bits. */
void vring_transport_features(struct virtio_device *vdev)
{
@@ -2771,6 +2786,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 8b95b69ef694..2550c9170f4f 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -117,4 +117,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_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
On Tue, Mar 21, 2023 at 04:44:10PM +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, PCI and channel I/O transports.
>
> Signed-off-by: Viktor Prutyanov <[email protected]>
Can you pls report what was tested and how? Thanks!
> ---
> v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
> remove byte swap, rename to vring_notification_data
> v2: reject the feature in virtio_ccw, replace __le32 with u32
>
> drivers/s390/virtio/virtio_ccw.c | 4 +++-
> drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
> 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, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 954fc31b4bc7..c33172c5b8d5 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
> struct virtio_ccw_vq_info *info = vq->priv;
> struct virtio_ccw_device *vcdev;
> struct subchannel_id schid;
> + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
> + vring_notification_data(vq) : vq->index;
>
> vcdev = to_vc_device(info->vq->vdev);
> ccw_device_get_schid(vcdev->cdev, &schid);
> BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
> info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
> *((unsigned int *)&schid),
> - vq->index, info->cookie);
> + data, info->cookie);
> if (info->cookie < 0)
> return false;
> return true;
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 3ff746e3f24a..7c16e622c33d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -285,6 +285,16 @@ 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_notification_data(vq);
> +
> + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> +
> + return true;
> +}
> +
> /* Notify all virtqueues on an interrupt. */
> static irqreturn_t vm_interrupt(int irq, void *opaque)
> {
> @@ -368,6 +378,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
> unsigned long flags;
> unsigned int num;
> int err;
> + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev,
> + VIRTIO_F_NOTIFICATION_DATA) ? vm_notify_with_data : vm_notify;
>
> if (!name)
> return NULL;
> @@ -397,7 +409,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, notify, 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..e915c22f2384 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_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)
> +
I still think this macro is pointless.
> /* 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);
And this is completely poinless because legacy can never
enable VIRTIO_F_NOTIFICATION_DATA.
> 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 4c3bb0ddeb9b..837875cc3190 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2752,6 +2752,21 @@ void vring_del_virtqueue(struct virtqueue *_vq)
> }
> EXPORT_SYMBOL_GPL(vring_del_virtqueue);
>
> +u32 vring_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)) |
> + vq->packed.avail_wrap_counter << 15;
> + else
> + next = vq->split.avail_idx_shadow;
> +
> + return next << 16 | _vq->index;
> +}
> +EXPORT_SYMBOL_GPL(vring_notification_data);
> +
> /* Manipulates transport-specific feature bits. */
> void vring_transport_features(struct virtio_device *vdev)
> {
> @@ -2771,6 +2786,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 8b95b69ef694..2550c9170f4f 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -117,4 +117,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_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
On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
>
> Signed-off-by: Viktor Prutyanov <[email protected]>
> ---
> v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
> remove byte swap, rename to vring_notification_data
> v2: reject the feature in virtio_ccw, replace __le32 with u32
>
> drivers/s390/virtio/virtio_ccw.c | 4 +++-
> drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
> 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, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 954fc31b4bc7..c33172c5b8d5 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
> struct virtio_ccw_vq_info *info = vq->priv;
> struct virtio_ccw_device *vcdev;
> struct subchannel_id schid;
> + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
> + vring_notification_data(vq) : vq->index;
>
> vcdev = to_vc_device(info->vq->vdev);
> ccw_device_get_schid(vcdev->cdev, &schid);
> BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
> info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
> *((unsigned int *)&schid),
> - vq->index, info->cookie);
> + data, info->cookie);
> if (info->cookie < 0)
> return false;
> return true;
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 3ff746e3f24a..7c16e622c33d 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -285,6 +285,16 @@ 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_notification_data(vq);
> +
> + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
Can't you simply use the same method as for ccw, i.e. use one callback
function that simply writes one value or the other?
> +
> + return true;
> +}
> +
> /* Notify all virtqueues on an interrupt. */
> static irqreturn_t vm_interrupt(int irq, void *opaque)
> {
> @@ -368,6 +378,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
> unsigned long flags;
> unsigned int num;
> int err;
> + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev,
> + VIRTIO_F_NOTIFICATION_DATA) ? vm_notify_with_data : vm_notify;
>
> if (!name)
> return NULL;
> @@ -397,7 +409,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, notify, 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..e915c22f2384 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_notification_data(vq);
> +
> + iowrite32(data, (void __iomem *)vq->priv);
Same for pci.
> +
> return true;
> }
>
On Tue, Mar 21, 2023 at 5:59 PM Cornelia Huck <[email protected]> wrote:
>
> On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
> >
> > Signed-off-by: Viktor Prutyanov <[email protected]>
> > ---
> > v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
> > remove byte swap, rename to vring_notification_data
> > v2: reject the feature in virtio_ccw, replace __le32 with u32
> >
> > drivers/s390/virtio/virtio_ccw.c | 4 +++-
> > drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
> > 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, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index 954fc31b4bc7..c33172c5b8d5 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
> > struct virtio_ccw_vq_info *info = vq->priv;
> > struct virtio_ccw_device *vcdev;
> > struct subchannel_id schid;
> > + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
> > + vring_notification_data(vq) : vq->index;
> >
> > vcdev = to_vc_device(info->vq->vdev);
> > ccw_device_get_schid(vcdev->cdev, &schid);
> > BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
> > info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
> > *((unsigned int *)&schid),
> > - vq->index, info->cookie);
> > + data, info->cookie);
> > if (info->cookie < 0)
> > return false;
> > return true;
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index 3ff746e3f24a..7c16e622c33d 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -285,6 +285,16 @@ 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_notification_data(vq);
> > +
> > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
>
> Can't you simply use the same method as for ccw, i.e. use one callback
> function that simply writes one value or the other?
The idea is to eliminate the conditional branch induced by feature bit
testing from the notification function. Probably, this can be done in
the same way in ccw.
>
> > +
> > + return true;
> > +}
> > +
> > /* Notify all virtqueues on an interrupt. */
> > static irqreturn_t vm_interrupt(int irq, void *opaque)
> > {
> > @@ -368,6 +378,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned int in
> > unsigned long flags;
> > unsigned int num;
> > int err;
> > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev,
> > + VIRTIO_F_NOTIFICATION_DATA) ? vm_notify_with_data : vm_notify;
> >
> > if (!name)
> > return NULL;
> > @@ -397,7 +409,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, notify, 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..e915c22f2384 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_notification_data(vq);
> > +
> > + iowrite32(data, (void __iomem *)vq->priv);
>
> Same for pci.
>
> > +
> > return true;
> > }
> >
>
On Tue, Mar 21 2023, Viktor Prutyanov <[email protected]> wrote:
> On Tue, Mar 21, 2023 at 5:59 PM Cornelia Huck <[email protected]> wrote:
>>
>> On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
>> >
>> > Signed-off-by: Viktor Prutyanov <[email protected]>
>> > ---
>> > v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
>> > remove byte swap, rename to vring_notification_data
>> > v2: reject the feature in virtio_ccw, replace __le32 with u32
>> >
>> > drivers/s390/virtio/virtio_ccw.c | 4 +++-
>> > drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
>> > 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, 57 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
>> > index 954fc31b4bc7..c33172c5b8d5 100644
>> > --- a/drivers/s390/virtio/virtio_ccw.c
>> > +++ b/drivers/s390/virtio/virtio_ccw.c
>> > @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
>> > struct virtio_ccw_vq_info *info = vq->priv;
>> > struct virtio_ccw_device *vcdev;
>> > struct subchannel_id schid;
>> > + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
>> > + vring_notification_data(vq) : vq->index;
>> >
>> > vcdev = to_vc_device(info->vq->vdev);
>> > ccw_device_get_schid(vcdev->cdev, &schid);
>> > BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
>> > info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
>> > *((unsigned int *)&schid),
>> > - vq->index, info->cookie);
>> > + data, info->cookie);
>> > if (info->cookie < 0)
>> > return false;
>> > return true;
>> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> > index 3ff746e3f24a..7c16e622c33d 100644
>> > --- a/drivers/virtio/virtio_mmio.c
>> > +++ b/drivers/virtio/virtio_mmio.c
>> > @@ -285,6 +285,16 @@ 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_notification_data(vq);
>> > +
>> > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
>>
>> Can't you simply use the same method as for ccw, i.e. use one callback
>> function that simply writes one value or the other?
>
> The idea is to eliminate the conditional branch induced by feature bit
> testing from the notification function. Probably, this can be done in
> the same way in ccw.
Hm, how noticable is that branch? IOW, is it worth making the code less
readable for this?
(In any case, all transports probably should use the same method.)
On Tue, Mar 21, 2023 at 04:30:57PM +0100, Cornelia Huck wrote:
> On Tue, Mar 21 2023, Viktor Prutyanov <[email protected]> wrote:
>
> > On Tue, Mar 21, 2023 at 5:59 PM Cornelia Huck <[email protected]> wrote:
> >>
> >> On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
> >> >
> >> > Signed-off-by: Viktor Prutyanov <[email protected]>
> >> > ---
> >> > v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
> >> > remove byte swap, rename to vring_notification_data
> >> > v2: reject the feature in virtio_ccw, replace __le32 with u32
> >> >
> >> > drivers/s390/virtio/virtio_ccw.c | 4 +++-
> >> > drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
> >> > 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, 57 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> >> > index 954fc31b4bc7..c33172c5b8d5 100644
> >> > --- a/drivers/s390/virtio/virtio_ccw.c
> >> > +++ b/drivers/s390/virtio/virtio_ccw.c
> >> > @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
> >> > struct virtio_ccw_vq_info *info = vq->priv;
> >> > struct virtio_ccw_device *vcdev;
> >> > struct subchannel_id schid;
> >> > + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
> >> > + vring_notification_data(vq) : vq->index;
> >> >
> >> > vcdev = to_vc_device(info->vq->vdev);
> >> > ccw_device_get_schid(vcdev->cdev, &schid);
> >> > BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
> >> > info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
> >> > *((unsigned int *)&schid),
> >> > - vq->index, info->cookie);
> >> > + data, info->cookie);
> >> > if (info->cookie < 0)
> >> > return false;
> >> > return true;
> >> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> >> > index 3ff746e3f24a..7c16e622c33d 100644
> >> > --- a/drivers/virtio/virtio_mmio.c
> >> > +++ b/drivers/virtio/virtio_mmio.c
> >> > @@ -285,6 +285,16 @@ 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_notification_data(vq);
> >> > +
> >> > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> >>
> >> Can't you simply use the same method as for ccw, i.e. use one callback
> >> function that simply writes one value or the other?
> >
> > The idea is to eliminate the conditional branch induced by feature bit
> > testing from the notification function. Probably, this can be done in
> > the same way in ccw.
>
> Hm, how noticable is that branch? IOW, is it worth making the code less
> readable for this?
I'm not sure but these things add up. I'm with Viktor here let's just
avoid the branch and not worry about whether it's important or not.
So let's use the same thing here then? And we can use a subfunction
to avoid code duplication.
> (In any case, all transports probably should use the same method.)
On Tue, Mar 21 2023, "Michael S. Tsirkin" <[email protected]> wrote:
> On Tue, Mar 21, 2023 at 04:30:57PM +0100, Cornelia Huck wrote:
>> On Tue, Mar 21 2023, Viktor Prutyanov <[email protected]> wrote:
>>
>> > On Tue, Mar 21, 2023 at 5:59 PM Cornelia Huck <[email protected]> wrote:
>> >>
>> >> On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
>> >> >
>> >> > Signed-off-by: Viktor Prutyanov <[email protected]>
>> >> > ---
>> >> > v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_shadow,
>> >> > remove byte swap, rename to vring_notification_data
>> >> > v2: reject the feature in virtio_ccw, replace __le32 with u32
>> >> >
>> >> > drivers/s390/virtio/virtio_ccw.c | 4 +++-
>> >> > drivers/virtio/virtio_mmio.c | 14 +++++++++++++-
>> >> > 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, 57 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
>> >> > index 954fc31b4bc7..c33172c5b8d5 100644
>> >> > --- a/drivers/s390/virtio/virtio_ccw.c
>> >> > +++ b/drivers/s390/virtio/virtio_ccw.c
>> >> > @@ -396,13 +396,15 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq)
>> >> > struct virtio_ccw_vq_info *info = vq->priv;
>> >> > struct virtio_ccw_device *vcdev;
>> >> > struct subchannel_id schid;
>> >> > + u32 data = __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_DATA) ?
>> >> > + vring_notification_data(vq) : vq->index;
>> >> >
>> >> > vcdev = to_vc_device(info->vq->vdev);
>> >> > ccw_device_get_schid(vcdev->cdev, &schid);
>> >> > BUILD_BUG_ON(sizeof(struct subchannel_id) != sizeof(unsigned int));
>> >> > info->cookie = kvm_hypercall3(KVM_S390_VIRTIO_CCW_NOTIFY,
>> >> > *((unsigned int *)&schid),
>> >> > - vq->index, info->cookie);
>> >> > + data, info->cookie);
>> >> > if (info->cookie < 0)
>> >> > return false;
>> >> > return true;
>> >> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> >> > index 3ff746e3f24a..7c16e622c33d 100644
>> >> > --- a/drivers/virtio/virtio_mmio.c
>> >> > +++ b/drivers/virtio/virtio_mmio.c
>> >> > @@ -285,6 +285,16 @@ 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_notification_data(vq);
>> >> > +
>> >> > + writel(data, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
>> >>
>> >> Can't you simply use the same method as for ccw, i.e. use one callback
>> >> function that simply writes one value or the other?
>> >
>> > The idea is to eliminate the conditional branch induced by feature bit
>> > testing from the notification function. Probably, this can be done in
>> > the same way in ccw.
>>
>> Hm, how noticable is that branch? IOW, is it worth making the code less
>> readable for this?
>
> I'm not sure but these things add up. I'm with Viktor here let's just
> avoid the branch and not worry about whether it's important or not.
> So let's use the same thing here then? And we can use a subfunction
> to avoid code duplication.
Ok, let's do it that way.
>
>> (In any case, all transports probably should use the same method.)