On Tue, Mar 21 2023, "Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> On Tue, Mar 21, 2023 at 04:30:57PM +0100, Cornelia Huck wrote:
>> On Tue, Mar 21 2023, Viktor Prutyanov <viktor@xxxxxxxxxx> wrote:
>>
>> > On Tue, Mar 21, 2023 at 5:59 PM Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
>> >>
>> >> On Tue, Mar 21 2023, Viktor Prutyanov <viktor@xxxxxxxxxx> 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 <viktor@xxxxxxxxxx>
>> >> > ---
>> >> > 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.)
