On Wed, 9 Nov 2022 02:04:17 -0500, "Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Wed, Nov 09, 2022 at 02:56:01PM +0800, Xuan Zhuo wrote:
> > On Wed, 9 Nov 2022 14:55:03 +0800, Jason Wang <jasow...@redhat.com> wrote:
> > >
> > > 在 2022/11/9 14:51, Michael S. Tsirkin 写道:
> > > > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > > >> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <stefa...@gmail.com>
> > > >> wrote:
> > > >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <m...@redhat.com>
> > > >>> wrote:
> > > >>>> From: Kangjie Xu <kangjie...@linux.alibaba.com>
> > > >>>>
> > > >>>> Introduce the interface queue_enable() in VirtioDeviceClass and the
> > > >>>> fucntion virtio_queue_enable() in virtio, it can be called when
> > > >>>> VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > > >>>> started. It only supports the devices of virtio 1 or later. The
> > > >>>> not-supported devices can only start the virtqueue when DRIVER_OK.
> > > >>>>
> > > >>>> Signed-off-by: Kangjie Xu <kangjie...@linux.alibaba.com>
> > > >>>> Signed-off-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
> > > >>>> Acked-by: Jason Wang <jasow...@redhat.com>
> > > >>>> Message-Id: <20221017092558.111082-4-xuanz...@linux.alibaba.com>
> > > >>>> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> > > >>>> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> > > >>>> ---
> > > >>>> include/hw/virtio/virtio.h | 2 ++
> > > >>>> hw/virtio/virtio.c | 14 ++++++++++++++
> > > >>>> 2 files changed, 16 insertions(+)
> > > >>>>
> > > >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > >>>> index 74d76c1dbc..b00b3fcf31 100644
> > > >>>> --- a/include/hw/virtio/virtio.h
> > > >>>> +++ b/include/hw/virtio/virtio.h
> > > >>>> @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> > > >>>> void (*reset)(VirtIODevice *vdev);
> > > >>>> void (*set_status)(VirtIODevice *vdev, uint8_t val);
> > > >>>> void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> + void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> /* For transitional devices, this is a bitmap of features
> > > >>>> * that are only exposed on the legacy interface but not
> > > >>>> * the modern one.
> > > >>>> @@ -288,6 +289,7 @@ int
> > > >>>> virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> > > >>>> int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > > >>>> void virtio_reset(void *opaque);
> > > >>>> void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > >>>> void virtio_update_irq(VirtIODevice *vdev);
> > > >>>> int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > > >>>>
> > > >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > >>>> index cf5f9ca387..9683b2e158 100644
> > > >>>> --- a/hw/virtio/virtio.c
> > > >>>> +++ b/hw/virtio/virtio.c
> > > >>>> @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev,
> > > >>>> uint32_t queue_index)
> > > >>>> __virtio_queue_reset(vdev, queue_index);
> > > >>>> }
> > > >>>>
> > > >>>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > > >>>> +{
> > > >>>> + VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > >>>> +
> > > >>>> + if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > >>>> + error_report("queue_enable is only suppported in devices of
> > > >>>> virtio "
> > > >>>> + "1.0 or later.");
> > > >>> Why is this triggering here? Maybe virtio_queue_enable() is called too
> > > >>> early. I have verified that the Linux guest driver sets VERSION_1. I
> > > >>> didn't check what SeaBIOS does.
> > > >> Looks like a bug, we should check device features here at least and it
> > > >> should be guest errors instead of error_report() here.
> > > >>
> > > >> Thanks
> > > > But it's weird, queue enable is written before guest features?
> > > > How come?
> > >
> > >
> > > Or queue_enable is written when the driver doesn't negotiate VERSION_1?
> >
> > Is this a bug?
> >
> > Or is it allowed in some cases?
> >
> > I feel weird too.
> >
> > Thanks.
>
> Weren't you able to reproduce?
> I suggest
> - write a bios patch to make it spec compliant
> - check UEFI to make sure it's spec compliant
> - ask bios/uefi maintainers whether they can include the patch for this
> release
It looks very interesting, I am happy to study it.
> - add a patch to drop the warning - we don't really need it
I sent this patch first.
Thanks.
>
>
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >>> $ build/qemu-system-x86_64 -M accel=kvm -m 1G -cpu host -blockdev
> > > >>> file,node-name=drive0,filename=test.img -device
> > > >>> virtio-blk-pci,drive=drive0
> > > >>> qemu: queue_enable is only suppported in devices of virtio 1.0 or
> > > >>> later.
> > > >>>
> > > >>> Stefan
> > > >>>
> > >
>
