From:   Cornelia Huck <cohuck@redhat.com>
To:     Viktor Prutyanov <viktor@daynix.com>
Cc:     mst@redhat.com, jasowang@redhat.com, pasic@linux.ibm.com,
        farman@linux.ibm.com, linux-s390@vger.kernel.org,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, yan@daynix.com
Subject: Re: [PATCH v3] virtio: add VIRTIO_F_NOTIFICATION_DATA feature support
In-Reply-To: <CAPv0NP6Ep4-B7cMc285E3d3vYjgwO7O1pq5sG3OYYAoZd3EAYQ@mail.gmail.com>
Organization: Red Hat GmbH
References: <20230321134410.2097163-1-viktor@daynix.com>
 <87h6uem9qc.fsf@redhat.com>
 <CAPv0NP6Ep4-B7cMc285E3d3vYjgwO7O1pq5sG3OYYAoZd3EAYQ@mail.gmail.com>
User-Agent: Notmuch/0.37 (https://notmuchmail.org)
Date:   Tue, 21 Mar 2023 16:30:57 +0100
Message-ID: <87bkkmm89a.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 21 2023, Viktor Prutyanov <viktor@daynix.com> wrote:

> On Tue, Mar 21, 2023 at 5:59=E2=80=AFPM Cornelia Huck <cohuck@redhat.com>=
 wrote:
>>
>> On Tue, Mar 21 2023, 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, PCI and channel I/O transports.
>> >
>> > Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
>> > ---
>> >  v3: support feature in virtio_ccw, remove VM_NOTIFY, use avail_idx_sh=
adow,
>> >     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/vi=
rtio_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 virtque=
ue *vq)
>> >       struct virtio_ccw_vq_info *info =3D vq->priv;
>> >       struct virtio_ccw_device *vcdev;
>> >       struct subchannel_id schid;
>> > +     u32 data =3D __virtio_test_bit(vq->vdev, VIRTIO_F_NOTIFICATION_D=
ATA) ?
>> > +                     vring_notification_data(vq) : vq->index;
>> >
>> >       vcdev =3D to_vc_device(info->vq->vdev);
>> >       ccw_device_get_schid(vcdev->cdev, &schid);
>> >       BUILD_BUG_ON(sizeof(struct subchannel_id) !=3D sizeof(unsigned i=
nt));
>> >       info->cookie =3D 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 =3D to_virtio_mmio_device(vq->=
vdev);
>> > +     u32 data =3D 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.)