| Message ID | 20230322141031.2211141-1-viktor@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support | expand |
On Wed, Mar 22, 2023 at 05:10:31PM +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, channel I/O and modern PCI > transports. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > v4: remove VP_NOTIFY macro and legacy PCI support, add > virtio_ccw_kvm_notify_with_data to virtio_ccw > 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 > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > to make sure nothing is broken. > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > and my hardware implementation of virtio-rng. what did you test? virtio pci? mmio? guessing not ccw... Cornelia could you hack up something to quickly test ccw? > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > include/linux/virtio_ring.h | 2 ++ > include/uapi/linux/virtio_config.h | 6 ++++++ > 6 files changed, 66 insertions(+), 5 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 954fc31b4bc7..3619676effb8 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > } > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > { > struct virtio_ccw_vq_info *info = vq->priv; > struct virtio_ccw_device *vcdev; > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > 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; > } > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > +{ > + return virtio_ccw_do_kvm_notify(vq, vq->index); > +} > + > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > +{ > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > +} > + > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > struct ccw1 *ccw, int index) > { > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > u64 queue; > unsigned long flags; > bool may_reduce; > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > + VIRTIO_F_NOTIFICATION_DATA) ? > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > /* Allocate queue. */ > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > may_reduce = vcdev->revision > 0; > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > vdev, true, may_reduce, ctx, > - virtio_ccw_kvm_notify, callback, name); > + notify, callback, name); > > if (!vq) { > /* For now, we fail if we can't get the requested size. */ > 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.cv > @@ -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_modern.c b/drivers/virtio/virtio_pci_modern.c > index 9e496e288cfa..9cc56f463dba 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > return vp_modern_config_vector(&vp_dev->mdev, vector); > } > > +static bool vp_notify_with_data(struct virtqueue *vq) > +{ > + u32 data = vring_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > + return true; > +} > + > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > struct virtio_pci_vq_info *info, > unsigned int index, > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > struct virtqueue *vq; > u16 num; > int err; > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > if (index >= vp_modern_get_num_queues(mdev)) > return ERR_PTR(-EINVAL); > @@ -321,7 +332,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); > + notify, 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 Wed, Mar 22, 2023, 19:33 Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Mar 22, 2023 at 05:10:31PM +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, channel I/O and modern PCI > > transports. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > 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 > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > > to make sure nothing is broken. > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > > and my hardware implementation of virtio-rng. > > what did you test? virtio pci? mmio? guessing not ccw... MMIO > > Cornelia could you hack up something to quickly test ccw? > > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 6 files changed, 66 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index 954fc31b4bc7..3619676effb8 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > > } > > > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > > { > > struct virtio_ccw_vq_info *info = vq->priv; > > struct virtio_ccw_device *vcdev; > > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > 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; > > } > > > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vq->index); > > +} > > + > > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > > +} > > + > > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > > struct ccw1 *ccw, int index) > > { > > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > u64 queue; > > unsigned long flags; > > bool may_reduce; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? > > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > > > /* Allocate queue. */ > > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > may_reduce = vcdev->revision > 0; > > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > > vdev, true, may_reduce, ctx, > > - virtio_ccw_kvm_notify, callback, name); > > + notify, callback, name); > > > > if (!vq) { > > /* For now, we fail if we can't get the requested size. */ > > 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.cv > > @@ -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_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 9e496e288cfa..9cc56f463dba 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > return vp_modern_config_vector(&vp_dev->mdev, vector); > > } > > > > +static bool vp_notify_with_data(struct virtqueue *vq) > > +{ > > + u32 data = vring_notification_data(vq); > > + > > + iowrite32(data, (void __iomem *)vq->priv); > > + > > + return true; > > +} > > + > > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtio_pci_vq_info *info, > > unsigned int index, > > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtqueue *vq; > > u16 num; > > int err; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > > > if (index >= vp_modern_get_num_queues(mdev)) > > return ERR_PTR(-EINVAL); > > @@ -321,7 +332,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); > > + notify, 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 Wed, Mar 22 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Mar 22, 2023 at 05:10:31PM +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, channel I/O and modern PCI >> transports. >> >> Signed-off-by: Viktor Prutyanov <viktor@daynix.com> >> --- >> v4: remove VP_NOTIFY macro and legacy PCI support, add >> virtio_ccw_kvm_notify_with_data to virtio_ccw >> 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 >> >> Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x >> (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, >> virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) >> to make sure nothing is broken. >> Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux >> and my hardware implementation of virtio-rng. > > what did you test? virtio pci? mmio? guessing not ccw... > > Cornelia could you hack up something to quickly test ccw? Hm, I'm not entirely sure how notification data is supposed to be used in real life -- Viktor, what is your virtio-rng implementation doing; can this be hacked into all transports? (Also, if the other ccw folks have something handy, please speak up :)
On Wed, Mar 22, 2023 at 7:42 PM Cornelia Huck <cohuck@redhat.com> wrote: > > On Wed, Mar 22 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Mar 22, 2023 at 05:10:31PM +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, channel I/O and modern PCI > >> transports. > >> > >> Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > >> --- > >> v4: remove VP_NOTIFY macro and legacy PCI support, add > >> virtio_ccw_kvm_notify_with_data to virtio_ccw > >> 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 > >> > >> Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > >> (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > >> virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > >> to make sure nothing is broken. > >> Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > >> and my hardware implementation of virtio-rng. > > > > what did you test? virtio pci? mmio? guessing not ccw... > > > > Cornelia could you hack up something to quickly test ccw? > > Hm, I'm not entirely sure how notification data is supposed to be used > in real life -- Viktor, what is your virtio-rng implementation doing; > can this be hacked into all transports? In hardware implementation of split VirtIO queue, the notification data feature saves time on a memory request to avail_idx from the device side. I can definitely say about MMIO, but most likely it is also useful for PCI. It is also written here that this feature was added for efficiency: https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg02728.html Also DPDK has support for this feature in VirtIO PCI: https://github.com/DPDK/dpdk/commit/7e72f3ec1a8abefd9321a61e484846e16177f5b1 > > (Also, if the other ccw folks have something handy, please speak up :) >
On Wed, 22 Mar 2023 17:10:31 +0300, 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, channel I/O and modern PCI > transports. > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > --- > v4: remove VP_NOTIFY macro and legacy PCI support, add > virtio_ccw_kvm_notify_with_data to virtio_ccw > 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 > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > to make sure nothing is broken. > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > and my hardware implementation of virtio-rng. > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > include/linux/virtio_ring.h | 2 ++ > include/uapi/linux/virtio_config.h | 6 ++++++ > 6 files changed, 66 insertions(+), 5 deletions(-) > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 954fc31b4bc7..3619676effb8 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > } > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > { > struct virtio_ccw_vq_info *info = vq->priv; > struct virtio_ccw_device *vcdev; > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > 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; > } > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > +{ > + return virtio_ccw_do_kvm_notify(vq, vq->index); > +} > + > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > +{ > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > +} > + > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > struct ccw1 *ccw, int index) > { > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > u64 queue; > unsigned long flags; > bool may_reduce; > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > + VIRTIO_F_NOTIFICATION_DATA) ? > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > /* Allocate queue. */ > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > may_reduce = vcdev->revision > 0; > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > vdev, true, may_reduce, ctx, > - virtio_ccw_kvm_notify, callback, name); > + notify, callback, name); > > if (!vq) { > /* For now, we fail if we can't get the requested size. */ > 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.cv > @@ -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; Can we optimize this line? Thanks. > > 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_modern.c b/drivers/virtio/virtio_pci_modern.c > index 9e496e288cfa..9cc56f463dba 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > return vp_modern_config_vector(&vp_dev->mdev, vector); > } > > +static bool vp_notify_with_data(struct virtqueue *vq) > +{ > + u32 data = vring_notification_data(vq); > + > + iowrite32(data, (void __iomem *)vq->priv); > + > + return true; > +} > + > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > struct virtio_pci_vq_info *info, > unsigned int index, > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > struct virtqueue *vq; > u16 num; > int err; > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > if (index >= vp_modern_get_num_queues(mdev)) > return ERR_PTR(-EINVAL); > @@ -321,7 +332,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); > + notify, 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 Thu, Mar 23, 2023 at 4:22 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Wed, 22 Mar 2023 17:10:31 +0300, 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, channel I/O and modern PCI > > transports. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > 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 > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > > to make sure nothing is broken. > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > > and my hardware implementation of virtio-rng. > > > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 6 files changed, 66 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index 954fc31b4bc7..3619676effb8 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > > } > > > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > > { > > struct virtio_ccw_vq_info *info = vq->priv; > > struct virtio_ccw_device *vcdev; > > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > 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; > > } > > > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vq->index); > > +} > > + > > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > > +} > > + > > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > > struct ccw1 *ccw, int index) > > { > > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > u64 queue; > > unsigned long flags; > > bool may_reduce; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? > > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > > > /* Allocate queue. */ > > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > may_reduce = vcdev->revision > 0; > > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > > vdev, true, may_reduce, ctx, > > - virtio_ccw_kvm_notify, callback, name); > > + notify, callback, name); > > > > if (!vq) { > > /* For now, we fail if we can't get the requested size. */ > > 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.cv > > @@ -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; > > > Can we optimize this line? What kind of optimization do you mean? > > Thanks. > > > > > 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_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 9e496e288cfa..9cc56f463dba 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > return vp_modern_config_vector(&vp_dev->mdev, vector); > > } > > > > +static bool vp_notify_with_data(struct virtqueue *vq) > > +{ > > + u32 data = vring_notification_data(vq); > > + > > + iowrite32(data, (void __iomem *)vq->priv); > > + > > + return true; > > +} > > + > > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtio_pci_vq_info *info, > > unsigned int index, > > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtqueue *vq; > > u16 num; > > int err; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > > > if (index >= vp_modern_get_num_queues(mdev)) > > return ERR_PTR(-EINVAL); > > @@ -321,7 +332,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); > > + notify, 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 Thu, Mar 23, 2023 at 09:21:05AM +0800, Xuan Zhuo wrote: > On Wed, 22 Mar 2023 17:10:31 +0300, 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, channel I/O and modern PCI > > transports. > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > --- > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > 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 > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > > to make sure nothing is broken. > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > > and my hardware implementation of virtio-rng. > > > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > include/linux/virtio_ring.h | 2 ++ > > include/uapi/linux/virtio_config.h | 6 ++++++ > > 6 files changed, 66 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index 954fc31b4bc7..3619676effb8 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > > } > > > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > > { > > struct virtio_ccw_vq_info *info = vq->priv; > > struct virtio_ccw_device *vcdev; > > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > 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; > > } > > > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vq->index); > > +} > > + > > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > > +{ > > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > > +} > > + > > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > > struct ccw1 *ccw, int index) > > { > > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > u64 queue; > > unsigned long flags; > > bool may_reduce; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? > > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > > > /* Allocate queue. */ > > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > may_reduce = vcdev->revision > 0; > > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > > vdev, true, may_reduce, ctx, > > - virtio_ccw_kvm_notify, callback, name); > > + notify, callback, name); > > > > if (!vq) { > > /* For now, we fail if we can't get the requested size. */ > > 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.cv > > @@ -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; > > > Can we optimize this line? > > Thanks. why bother? it's setup time .. > > > > 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_modern.c b/drivers/virtio/virtio_pci_modern.c > > index 9e496e288cfa..9cc56f463dba 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > return vp_modern_config_vector(&vp_dev->mdev, vector); > > } > > > > +static bool vp_notify_with_data(struct virtqueue *vq) > > +{ > > + u32 data = vring_notification_data(vq); > > + > > + iowrite32(data, (void __iomem *)vq->priv); > > + > > + return true; > > +} > > + > > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtio_pci_vq_info *info, > > unsigned int index, > > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > struct virtqueue *vq; > > u16 num; > > int err; > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > > > if (index >= vp_modern_get_num_queues(mdev)) > > return ERR_PTR(-EINVAL); > > @@ -321,7 +332,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); > > + notify, 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 Thu, Mar 23, 2023 at 10:18:56AM +0300, Viktor Prutyanov wrote: > On Thu, Mar 23, 2023 at 4:22 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > On Wed, 22 Mar 2023 17:10:31 +0300, 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, channel I/O and modern PCI > > > transports. > > > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > > --- > > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > > 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 > > > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > > > to make sure nothing is broken. > > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > > > and my hardware implementation of virtio-rng. > > > > > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > > > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > > > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > > include/linux/virtio_ring.h | 2 ++ > > > include/uapi/linux/virtio_config.h | 6 ++++++ > > > 6 files changed, 66 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > index 954fc31b4bc7..3619676effb8 100644 > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > > > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > > > } > > > > > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > > > { > > > struct virtio_ccw_vq_info *info = vq->priv; > > > struct virtio_ccw_device *vcdev; > > > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > 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; > > > } > > > > > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > +{ > > > + return virtio_ccw_do_kvm_notify(vq, vq->index); > > > +} > > > + > > > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > > > +{ > > > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > > > +} > > > + > > > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > > > struct ccw1 *ccw, int index) > > > { > > > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > > u64 queue; > > > unsigned long flags; > > > bool may_reduce; > > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > > > + VIRTIO_F_NOTIFICATION_DATA) ? > > > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > > > > > /* Allocate queue. */ > > > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > > > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > > may_reduce = vcdev->revision > 0; > > > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > > > vdev, true, may_reduce, ctx, > > > - virtio_ccw_kvm_notify, callback, name); > > > + notify, callback, name); > > > > > > if (!vq) { > > > /* For now, we fail if we can't get the requested size. */ > > > 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.cv > > > @@ -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; > > > > > > Can we optimize this line? > > What kind of optimization do you mean? In fact speed does not matter here but it is not very readable. Use of "?" was justified if you put this inside the call to vring_create_virtqueue (which I still feel would be best). But if you use a variable, just use plain if: bool (*notify)(struct virtqueue *vq); if (__virtio_test_bit(vdev,VIRTIO_F_NOTIFICATION_DATA)) vq = vm_notify_with_data; else vq = vm_notify; > > > > Thanks. > > > > > > > > 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_modern.c b/drivers/virtio/virtio_pci_modern.c > > > index 9e496e288cfa..9cc56f463dba 100644 > > > --- a/drivers/virtio/virtio_pci_modern.c > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > > return vp_modern_config_vector(&vp_dev->mdev, vector); > > > } > > > > > > +static bool vp_notify_with_data(struct virtqueue *vq) > > > +{ > > > + u32 data = vring_notification_data(vq); > > > + > > > + iowrite32(data, (void __iomem *)vq->priv); > > > + > > > + return true; > > > +} > > > + > > > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > struct virtio_pci_vq_info *info, > > > unsigned int index, > > > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > struct virtqueue *vq; > > > u16 num; > > > int err; > > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > > > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > > > > > if (index >= vp_modern_get_num_queues(mdev)) > > > return ERR_PTR(-EINVAL); > > > @@ -321,7 +332,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); > > > + notify, 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 Thu, 23 Mar 2023 03:25:25 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Mar 23, 2023 at 10:18:56AM +0300, Viktor Prutyanov wrote: > > On Thu, Mar 23, 2023 at 4:22 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > On Wed, 22 Mar 2023 17:10:31 +0300, 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, channel I/O and modern PCI > > > > transports. > > > > > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > > > --- > > > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > > > 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 > > > > > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x > > > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) > > > > to make sure nothing is broken. > > > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux > > > > and my hardware implementation of virtio-rng. > > > > > > > > drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- > > > > drivers/virtio/virtio_mmio.c | 14 +++++++++++++- > > > > drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- > > > > drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ > > > > include/linux/virtio_ring.h | 2 ++ > > > > include/uapi/linux/virtio_config.h | 6 ++++++ > > > > 6 files changed, 66 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > > > index 954fc31b4bc7..3619676effb8 100644 > > > > --- a/drivers/s390/virtio/virtio_ccw.c > > > > +++ b/drivers/s390/virtio/virtio_ccw.c > > > > @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, > > > > ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); > > > > } > > > > > > > > -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > > +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) > > > > { > > > > struct virtio_ccw_vq_info *info = vq->priv; > > > > struct virtio_ccw_device *vcdev; > > > > @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > > 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; > > > > } > > > > > > > > +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) > > > > +{ > > > > + return virtio_ccw_do_kvm_notify(vq, vq->index); > > > > +} > > > > + > > > > +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) > > > > +{ > > > > + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); > > > > +} > > > > + > > > > static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, > > > > struct ccw1 *ccw, int index) > > > > { > > > > @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > > > u64 queue; > > > > unsigned long flags; > > > > bool may_reduce; > > > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, > > > > + VIRTIO_F_NOTIFICATION_DATA) ? > > > > + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; > > > > > > > > /* Allocate queue. */ > > > > info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); > > > > @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, > > > > may_reduce = vcdev->revision > 0; > > > > vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, > > > > vdev, true, may_reduce, ctx, > > > > - virtio_ccw_kvm_notify, callback, name); > > > > + notify, callback, name); > > > > > > > > if (!vq) { > > > > /* For now, we fail if we can't get the requested size. */ > > > > 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.cv > > > > @@ -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; > > > > > > > > > Can we optimize this line? > > > > What kind of optimization do you mean? > > In fact speed does not matter here but it is not very readable. > Use of "?" was justified if you put this inside the call to vring_create_virtqueue > (which I still feel would be best). > But if you use a variable, just use plain if: > > bool (*notify)(struct virtqueue *vq); > > if (__virtio_test_bit(vdev,VIRTIO_F_NOTIFICATION_DATA)) > vq = vm_notify_with_data; > else > vq = vm_notify; Yes, I mean this, I don't mean performance. It refers to the form of code. It should be that my expression is inaccurate. Thanks. > > > > > > > > Thanks. > > > > > > > > > > > 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_modern.c b/drivers/virtio/virtio_pci_modern.c > > > > index 9e496e288cfa..9cc56f463dba 100644 > > > > --- a/drivers/virtio/virtio_pci_modern.c > > > > +++ b/drivers/virtio/virtio_pci_modern.c > > > > @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > > > return vp_modern_config_vector(&vp_dev->mdev, vector); > > > > } > > > > > > > > +static bool vp_notify_with_data(struct virtqueue *vq) > > > > +{ > > > > + u32 data = vring_notification_data(vq); > > > > + > > > > + iowrite32(data, (void __iomem *)vq->priv); > > > > + > > > > + return true; > > > > +} > > > > + > > > > static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > > struct virtio_pci_vq_info *info, > > > > unsigned int index, > > > > @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, > > > > struct virtqueue *vq; > > > > u16 num; > > > > int err; > > > > + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, > > > > + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; > > > > > > > > if (index >= vp_modern_get_num_queues(mdev)) > > > > return ERR_PTR(-EINVAL); > > > > @@ -321,7 +332,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); > > > > + notify, 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 Wed, 2023-03-22 at 17:42 +0100, Cornelia Huck wrote: > On Wed, Mar 22 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Mar 22, 2023 at 05:10:31PM +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, channel I/O and modern PCI > > > transports. > > > > > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com> > > > --- > > > v4: remove VP_NOTIFY macro and legacy PCI support, add > > > virtio_ccw_kvm_notify_with_data to virtio_ccw > > > 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 > > > > > > Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system- > > > s390x > > > (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, > > > virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio- > > > net-pci) > > > to make sure nothing is broken. > > > Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V > > > Linux > > > and my hardware implementation of virtio-rng. > > > > what did you test? virtio pci? mmio? guessing not ccw... > > > > Cornelia could you hack up something to quickly test ccw? > > Hm, I'm not entirely sure how notification data is supposed to be > used > in real life -- Viktor, what is your virtio-rng implementation doing; > can this be hacked into all transports? > > (Also, if the other ccw folks have something handy, please speak up > :) > (Sorry for delay, caught the illness going through our house before I had a chance to look at this.) I applied v6 of this patch and hacked to QEMU to enable VIRTIO_F_NOTIFICATION_DATA for virtio-blk, and have the -ccw code ignore the additional data that then comes along the way. Not surprisingly, things behave fine once I accommodate the new payload. To Cornelia's point of how it should be used in real life... I didn't go beyond using this info as a "debugging aid" here, based on time constraints. But at least it looks reasonable. Eric
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 954fc31b4bc7..3619676effb8 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -391,7 +391,7 @@ static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev, ccw_device_dma_free(vcdev->cdev, thinint_area, sizeof(*thinint_area)); } -static bool virtio_ccw_kvm_notify(struct virtqueue *vq) +static inline bool virtio_ccw_do_kvm_notify(struct virtqueue *vq, u32 data) { struct virtio_ccw_vq_info *info = vq->priv; struct virtio_ccw_device *vcdev; @@ -402,12 +402,22 @@ static bool virtio_ccw_kvm_notify(struct virtqueue *vq) 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; } +static bool virtio_ccw_kvm_notify(struct virtqueue *vq) +{ + return virtio_ccw_do_kvm_notify(vq, vq->index); +} + +static bool virtio_ccw_kvm_notify_with_data(struct virtqueue *vq) +{ + return virtio_ccw_do_kvm_notify(vq, vring_notification_data(vq)); +} + static int virtio_ccw_read_vq_conf(struct virtio_ccw_device *vcdev, struct ccw1 *ccw, int index) { @@ -501,6 +511,9 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, u64 queue; unsigned long flags; bool may_reduce; + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(vdev, + VIRTIO_F_NOTIFICATION_DATA) ? + virtio_ccw_kvm_notify_with_data : virtio_ccw_kvm_notify; /* Allocate queue. */ info = kzalloc(sizeof(struct virtio_ccw_vq_info), GFP_KERNEL); @@ -524,7 +537,7 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev, may_reduce = vcdev->revision > 0; vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, vdev, true, may_reduce, ctx, - virtio_ccw_kvm_notify, callback, name); + notify, callback, name); if (!vq) { /* For now, we fail if we can't get the requested size. */ 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.cv @@ -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_modern.c b/drivers/virtio/virtio_pci_modern.c index 9e496e288cfa..9cc56f463dba 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -288,6 +288,15 @@ static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) return vp_modern_config_vector(&vp_dev->mdev, vector); } +static bool vp_notify_with_data(struct virtqueue *vq) +{ + u32 data = vring_notification_data(vq); + + iowrite32(data, (void __iomem *)vq->priv); + + return true; +} + static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, struct virtio_pci_vq_info *info, unsigned int index, @@ -301,6 +310,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, struct virtqueue *vq; u16 num; int err; + bool (*notify)(struct virtqueue *vq) = __virtio_test_bit(&vp_dev->vdev, + VIRTIO_F_NOTIFICATION_DATA) ? vp_notify_with_data : vp_notify; if (index >= vp_modern_get_num_queues(mdev)) return ERR_PTR(-EINVAL); @@ -321,7 +332,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); + notify, 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. */
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, channel I/O and modern PCI transports. Signed-off-by: Viktor Prutyanov <viktor@daynix.com> --- v4: remove VP_NOTIFY macro and legacy PCI support, add virtio_ccw_kvm_notify_with_data to virtio_ccw 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 Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-s390x (virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device, virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-net-pci) to make sure nothing is broken. Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V Linux and my hardware implementation of virtio-rng. drivers/s390/virtio/virtio_ccw.c | 19 ++++++++++++++++--- drivers/virtio/virtio_mmio.c | 14 +++++++++++++- drivers/virtio/virtio_pci_modern.c | 13 ++++++++++++- drivers/virtio/virtio_ring.c | 17 +++++++++++++++++ include/linux/virtio_ring.h | 2 ++ include/uapi/linux/virtio_config.h | 6 ++++++ 6 files changed, 66 insertions(+), 5 deletions(-)