Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <9dea1ac2f7624b74050b125bcab01272ea7bbd4e.camel@linux.ibm.com>
Subject: Re: [PATCH v4] virtio: add VIRTIO_F_NOTIFICATION_DATA feature
 support
From:   Eric Farman <farman@linux.ibm.com>
To:     Cornelia Huck <cohuck@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Viktor Prutyanov <viktor@daynix.com>
Cc:     jasowang@redhat.com, pasic@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
Date:   Thu, 30 Mar 2023 13:48:04 -0400
In-Reply-To: <87mt44hh5f.fsf@redhat.com>
References: <20230322141031.2211141-1-viktor@daynix.com>
         <20230322123121-mutt-send-email-mst@kernel.org> <87mt44hh5f.fsf@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-03-22 at 17:42 +0100, Cornelia Huck wrote:
> On Wed, Mar 22 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>=20
> > 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.
> > >=20
> > > 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.
> > >=20
> > > Add support for this feature for MMIO, channel I/O and modern PCI
> > > transports.
> > >=20
> > > Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> > > ---
> > > =C2=A0v4: remove VP_NOTIFY macro and legacy PCI support, add
> > > =C2=A0=C2=A0=C2=A0 virtio_ccw_kvm_notify_with_data to virtio_ccw
> > > =C2=A0v3: support feature in virtio_ccw, remove VM_NOTIFY, use
> > > avail_idx_shadow,
> > > =C2=A0=C2=A0=C2=A0 remove byte swap, rename to vring_notification_dat=
a
> > > =C2=A0v2: reject the feature in virtio_ccw, replace __le32 with u32
> > >=20
> > > =C2=A0Tested with disabled VIRTIO_F_NOTIFICATION_DATA on qemu-system-
> > > s390x
> > > =C2=A0(virtio-blk-ccw), qemu-system-riscv64 (virtio-blk-device,
> > > =C2=A0virtio-rng-device), qemu-system-x86_64 (virtio-blk-pci, virtio-
> > > net-pci)
> > > =C2=A0to make sure nothing is broken.
> > > =C2=A0Tested with enabled VIRTIO_F_NOTIFICATION_DATA on 64-bit RISC-V
> > > Linux
> > > =C2=A0and my hardware implementation of virtio-rng.
> >=20
> > what did you test? virtio pci? mmio? guessing not ccw...
> >=20
> > Cornelia could you hack up something to quickly test ccw?
>=20
> 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?
>=20
> (Also, if the other ccw folks have something handy, please speak up
> :)
>=20

(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