[PULL,v4,29/83] virtio: introduce virtio_queue_enable()

Commit Message

Michael S. Tsirkin Nov. 7, 2022, 10:49 p.m. UTC

From: Kangjie Xu <[e-mail]>

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 <[e-mail]>
Signed-off-by: Xuan Zhuo <[e-mail]>
Acked-by: Jason Wang <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Michael S. Tsirkin <[e-mail]>
Signed-off-by: Michael S. Tsirkin <[e-mail]>
---
 include/hw/virtio/virtio.h |  2 ++
 hw/virtio/virtio.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Stefan Hajnoczi Nov. 8, 2022, 7:43 p.m. UTC | #1

On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
>
> From: Kangjie Xu <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Xuan Zhuo <[e-mail]>
> Acked-by: Jason Wang <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> ---
>  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.

$ 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

Jason Wang Nov. 9, 2022, 3:31 a.m. UTC | #2

On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
>
> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> >
> > From: Kangjie Xu <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Xuan Zhuo <[e-mail]>
> > Acked-by: Jason Wang <[e-mail]>
> > Message-Id: <[e-mail]>
> > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > ---
> >  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

>
> $ 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
>

Michael S. Tsirkin Nov. 9, 2022, 6:39 a.m. UTC | #3

On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> >
> > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > >
> > > From: Kangjie Xu <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > Acked-by: Jason Wang <[e-mail]>
> > > Message-Id: <[e-mail]>
> > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > ---
> > >  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
> 

I suspect we should just drop this print. Kangjie?


> > $ 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
> >

Xuan Zhuo Nov. 9, 2022, 6:48 a.m. UTC | #4

On Wed, 9 Nov 2022 01:39:32 -0500, "Michael S. Tsirkin" <[e-mail]> wrote:
> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> > >
> > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > > >
> > > > From: Kangjie Xu <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > > Acked-by: Jason Wang <[e-mail]>
> > > > Message-Id: <[e-mail]>
> > > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > > ---
> > > >  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
> >
>
> I suspect we should just drop this print. Kangjie?


I think it is.

At that time, this inspection was only added at hand, and theoretically it
should not be performed.

I am responsible for this patch set now.

hi, Michael,

What should I do, do I send a new version again?

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
> > >
>

Michael S. Tsirkin Nov. 9, 2022, 6:51 a.m. UTC | #5

On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> >
> > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > >
> > > From: Kangjie Xu <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > Acked-by: Jason Wang <[e-mail]>
> > > Message-Id: <[e-mail]>
> > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > ---
> > >  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?

> >
> > $ 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
> >

Jason Wang Nov. 9, 2022, 6:55 a.m. UTC | #6

在 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 <[e-mail]> wrote:
>>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
>>>> From: Kangjie Xu <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Xuan Zhuo <[e-mail]>
>>>> Acked-by: Jason Wang <[e-mail]>
>>>> Message-Id: <[e-mail]>
>>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
>>>> ---
>>>>   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?

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
>>>

Xuan Zhuo Nov. 9, 2022, 6:56 a.m. UTC | #7

On Wed, 9 Nov 2022 14:55:03 +0800, Jason Wang <[e-mail]> 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 <[e-mail]> wrote:
> >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> >>>> From: Kangjie Xu <[e-mail]>
> >>>>
> >>>> 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 <[e-mail]>
> >>>> Signed-off-by: Xuan Zhuo <[e-mail]>
> >>>> Acked-by: Jason Wang <[e-mail]>
> >>>> Message-Id: <[e-mail]>
> >>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> >>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> >>>> ---
> >>>>   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.

>
> 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
> >>>
>

Michael S. Tsirkin Nov. 9, 2022, 6:59 a.m. UTC | #8

On Wed, Nov 09, 2022 at 01:52:01AM -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> > >
> > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > > >
> > > > From: Kangjie Xu <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > > Acked-by: Jason Wang <[e-mail]>
> > > > Message-Id: <[e-mail]>
> > > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > > ---
> > > >  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?

It's a bios bug:


            
    vp_init_simple(&vdrive->vp, pci);
    if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
        dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
        goto fail; 
    }           
    
    if (vdrive->vp.use_modern) {
        struct vp_device *vp = &vdrive->vp;
        u64 features = vp_get_features(vp);
        u64 version1 = 1ull << VIRTIO_F_VERSION_1;
        u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
        u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
        if (!(features & version1)) {
            dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
            goto fail;
        }
        
        features = features & (version1 | iommu_platform | blk_size);
        vp_set_features(vp, features);
        status |= VIRTIO_CONFIG_S_FEATURES_OK;
        vp_set_status(vp, status);



Not good - does not match the spec. Here's what the spec says:


The driver MUST follow this sequence to initialize a device:
1. Reset the device.
2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
3. Set the DRIVER status bit: the guest OS knows how to drive the device.
4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
fields to check that it can support the device before accepting it.
5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not
support our subset of features and the device is unusable.
7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
8. Set the DRIVER_OK status bit. At this point the device is “live”.


Right?



> > >
> > > $ 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
> > >

Michael S. Tsirkin Nov. 9, 2022, 7:01 a.m. UTC | #9

On Wed, Nov 09, 2022 at 02:48:29PM +0800, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 01:39:32 -0500, "Michael S. Tsirkin" <[e-mail]> wrote:
> > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> > > >
> > > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > > > >
> > > > > From: Kangjie Xu <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > > > Acked-by: Jason Wang <[e-mail]>
> > > > > Message-Id: <[e-mail]>
> > > > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > > > ---
> > > > >  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
> > >
> >
> > I suspect we should just drop this print. Kangjie?
> 
> 
> I think it is.
> 
> At that time, this inspection was only added at hand, and theoretically it
> should not be performed.
> 
> I am responsible for this patch set now.
> 
> hi, Michael,
> 
> What should I do, do I send a new version again?
> 
> Thanks.

I debugged it and replied separately. Can you check EFI drivers too?

> >
> >
> > > > $ 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
> > > >
> >

Michael S. Tsirkin Nov. 9, 2022, 7:04 a.m. UTC | #10

On Wed, Nov 09, 2022 at 02:56:01PM +0800, Xuan Zhuo wrote:
> On Wed, 9 Nov 2022 14:55:03 +0800, Jason Wang <[e-mail]> 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 <[e-mail]> wrote:
> > >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > >>>> From: Kangjie Xu <[e-mail]>
> > >>>>
> > >>>> 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 <[e-mail]>
> > >>>> Signed-off-by: Xuan Zhuo <[e-mail]>
> > >>>> Acked-by: Jason Wang <[e-mail]>
> > >>>> Message-Id: <[e-mail]>
> > >>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > >>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > >>>> ---
> > >>>>   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
	- add a patch to drop the warning - we don't really need it


> >
> > 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
> > >>>
> >

Xuan Zhuo Nov. 9, 2022, 7:11 a.m. UTC | #11

On Wed, 9 Nov 2022 02:01:38 -0500, "Michael S. Tsirkin" <[e-mail]> wrote:
> On Wed, Nov 09, 2022 at 02:48:29PM +0800, Xuan Zhuo wrote:
> > On Wed, 9 Nov 2022 01:39:32 -0500, "Michael S. Tsirkin" <[e-mail]> wrote:
> > > On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> > > > On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> > > > >
> > > > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > > > > >
> > > > > > From: Kangjie Xu <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > Signed-off-by: Xuan Zhuo <[e-mail]>
> > > > > > Acked-by: Jason Wang <[e-mail]>
> > > > > > Message-Id: <[e-mail]>
> > > > > > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > > > > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > > > > ---
> > > > > >  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
> > > >
> > >
> > > I suspect we should just drop this print. Kangjie?
> >
> >
> > I think it is.
> >
> > At that time, this inspection was only added at hand, and theoretically it
> > should not be performed.
> >
> > I am responsible for this patch set now.
> >
> > hi, Michael,
> >
> > What should I do, do I send a new version again?
> >
> > Thanks.
>
> I debugged it and replied separately. Can you check EFI drivers too?

OK.

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
> > > > >
> > >
>

Xuan Zhuo Nov. 9, 2022, 7:12 a.m. UTC | #12

On Wed, 9 Nov 2022 02:04:17 -0500, "Michael S. Tsirkin" <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > >>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> > > >>>> From: Kangjie Xu <[e-mail]>
> > > >>>>
> > > >>>> 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 <[e-mail]>
> > > >>>> Signed-off-by: Xuan Zhuo <[e-mail]>
> > > >>>> Acked-by: Jason Wang <[e-mail]>
> > > >>>> Message-Id: <[e-mail]>
> > > >>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > > >>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > > >>>> ---
> > > >>>>   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
> > > >>>
> > >
>

Laszlo Ersek Nov. 9, 2022, 8:56 a.m. UTC | #13

On 11/09/22 07:59, Michael S. Tsirkin wrote:
> On Wed, Nov 09, 2022 at 01:52:01AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
>>> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
>>>>
>>>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
>>>>>
>>>>> From: Kangjie Xu <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Xuan Zhuo <[e-mail]>
>>>>> Acked-by: Jason Wang <[e-mail]>
>>>>> Message-Id: <[e-mail]>
>>>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
>>>>> ---
>>>>>  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?
>
> It's a bios bug:
>
>
>
>     vp_init_simple(&vdrive->vp, pci);
>     if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
>         dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
>         goto fail;
>     }
>
>     if (vdrive->vp.use_modern) {
>         struct vp_device *vp = &vdrive->vp;
>         u64 features = vp_get_features(vp);
>         u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>         u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>         u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
>         if (!(features & version1)) {
>             dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
>             goto fail;
>         }
>
>         features = features & (version1 | iommu_platform | blk_size);
>         vp_set_features(vp, features);
>         status |= VIRTIO_CONFIG_S_FEATURES_OK;
>         vp_set_status(vp, status);
>
>
>
> Not good - does not match the spec. Here's what the spec says:
>
>
> The driver MUST follow this sequence to initialize a device:
> 1. Reset the device.
> 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
> 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
> device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
> fields to check that it can support the device before accepting it.
> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
> 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not
> support our subset of features and the device is unusable.
> 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
> reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
> 8. Set the DRIVER_OK status bit. At this point the device is “live”.

Thanks for the Cc.

This should work properly in the edk2 (OVMF) guest drivers. When I
started work on the virtio-1.0 implementation, I noticed that the device
initialization sequence that guest drivers needed to follow had
*changed* from spec version 0.9.5 to spec version 1.0.

For example, in the virtio-rng driver, I addressed that with commit
0a781bdc7f87 ("OvmfPkg: VirtioRngDxe: adapt feature negotiation to
virtio-1.0", 2016-04-06):

  https://github.com/tianocore/edk2/commit/0a781bdc7f87

Therefore we have a larger context like this in edk2 (again the excerpt
is from the virtio-rng driver, but all drivers follow this pattern whose
devices can be either legacy or modern):

>   //
>   // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
>   //
>   NextDevStat = 0;             // step 1 -- reset device
>   Status      = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }

matches v1.0 spec step 1.

>
>   NextDevStat |= VSTAT_ACK;    // step 2 -- acknowledge device presence
>   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }

matches v1.0 spec step 2.

>
>   NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
>   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }

matches v1.0 spec step 3.

>
>   //
>   // Set Page Size - MMIO VirtIo Specific
>   //
>   Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // step 4a -- retrieve and validate features
>   //
>   Status = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo, &Features);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }

matches v1.0 spec step 4, the "read" part

>
>   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
>
>   //
>   // In virtio-1.0, feature negotiation is expected to complete before queue
>   // discovery, and the device can also reject the selected set of features.
>   //
>   if (Dev->VirtIo->Revision >= VIRTIO_SPEC_REVISION (1, 0, 0)) {
>     Status = Virtio10WriteFeatures (Dev->VirtIo, Features, &NextDevStat);
>     if (EFI_ERROR (Status)) {
>       goto Failed;
>     }
>   }

This is skipped for virtio-0.9.5. For virtio-1.0,
Virtio10WriteFeatures() does the following:

>   Status = VirtIo->SetGuestFeatures (VirtIo, Features);
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }

Covers v1.0 spec step 4, the rest of it.

>
>   *DeviceStatus |= VSTAT_FEATURES_OK;
>   Status         = VirtIo->SetDeviceStatus (VirtIo, *DeviceStatus);
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }

Covers v1.0 spec step 5.

>
>   Status = VirtIo->GetDeviceStatus (VirtIo, DeviceStatus);
>   if (EFI_ERROR (Status)) {
>     return Status;
>   }
>
>   if ((*DeviceStatus & VSTAT_FEATURES_OK) == 0) {
>     Status = EFI_UNSUPPORTED;
>   }

Covers v1.0 spec step 6.

OK, return to the previous call frame:

>
>   //
>   // step 4b -- allocate request virtqueue, just use #0
>   //
>   Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, 0);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // VirtioRngGetRNG() uses one descriptor
>   //
>   if (QueueSize < 1) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // If anything fails from here on, we must release the ring resources.
>   //
>   Status = VirtioRingMap (
>              Dev->VirtIo,
>              &Dev->Ring,
>              &RingBaseShift,
>              &Dev->RingMap
>              );
>   if (EFI_ERROR (Status)) {
>     goto ReleaseQueue;
>   }
>
>   //
>   // Additional steps for MMIO: align the queue appropriately, and set the
>   // size. If anything fails from here on, we must unmap the ring resources.
>   //
>   Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   //
>   // step 4c -- Report GPFN (guest-physical frame number) of queue.
>   //
>   Status = Dev->VirtIo->SetQueueAddress (
>                           Dev->VirtIo,
>                           &Dev->Ring,
>                           RingBaseShift
>                           );
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }

These cover v1.0 spec step 7.

>
>   //
>   // step 5 -- Report understood features and guest-tuneables.
>   //
>   if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
>     Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
>     Status    = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
>     if (EFI_ERROR (Status)) {
>       goto UnmapQueue;
>     }
>   }

This is exclusive to virtio-0.9.5; the "step 5" reference in the comment
pertains to that spec version. In virtio-0.9.5, this is the location
(just before setting DRIVER_OK) where the guest driver has to report its
desired features (and the device has no means to reject any feature set
that is otherwise a subset of the host feature set).

>
>   //
>   // step 6 -- initialization complete
>   //
>   NextDevStat |= VSTAT_DRIVER_OK;
>   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }

This covers v1.0 spec step 8 (and v0.9.5 spec step 6, as the comment
shows).

Now, in drivers whose devices are virtio-1.0-only, such as
virtio-gpu-pci, vhost-user-fs-pci, this "selectivity" is not there. The
virtio-0.9.5 branch is simply eliminated, and the virtio-1.0 logic is
unconditional. Additionally, in those drivers, the "step" references in
the code comments match the v1.0 bullet list. For example, in the
virtio-fs driver:

> EFI_STATUS
> VirtioFsInit (
>   IN OUT VIRTIO_FS  *VirtioFs
>   )
> {
>   UINT8             NextDevStat;
>   EFI_STATUS        Status;
>   UINT64            Features;
>   VIRTIO_FS_CONFIG  Config;
>   UINTN             Idx;
>   UINT64            RingBaseShift;
>
>   //
>   // Execute virtio-v1.1-cs01-87fa6b5d8155, 3.1.1 Driver Requirements: Device
>   // Initialization.
>   //
>   // 1. Reset the device.
>   //
>   NextDevStat = 0;
>   Status      = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 2. Set the ACKNOWLEDGE status bit [...]
>   //
>   NextDevStat |= VSTAT_ACK;
>   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 3. Set the DRIVER status bit [...]
>   //
>   NextDevStat |= VSTAT_DRIVER;
>   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 4. Read device feature bits...
>   //
>   Status = VirtioFs->Virtio->GetDeviceFeatures (VirtioFs->Virtio, &Features);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   if ((Features & VIRTIO_F_VERSION_1) == 0) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   //
>   // No device-specific feature bits have been defined in file "virtio-fs.tex"
>   // of the virtio spec at <https://github.com/oasis-tcs/virtio-spec.git>, as
>   // of commit 87fa6b5d8155.
>   //
>   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
>
>   //
>   // ... and write the subset of feature bits understood by the [...] driver to
>   // the device. [...]
>   // 5. Set the FEATURES_OK status bit.
>   // 6. Re-read device status to ensure the FEATURES_OK bit is still set [...]
>   //
>   Status = Virtio10WriteFeatures (VirtioFs->Virtio, Features, &NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 7. Perform device-specific setup, including discovery of virtqueues for
>   // the device, [...] reading [...] the device's virtio configuration space
>   //
>   Status = VirtioFsReadConfig (VirtioFs->Virtio, &Config);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 7.a. Convert the filesystem label from UTF-8 to UCS-2. Only labels with
>   // printable ASCII code points (U+0020 through U+007E) are supported.
>   // NUL-terminate at either the terminator we find, or right after the
>   // original label.
>   //
>   for (Idx = 0; Idx < VIRTIO_FS_TAG_BYTES && Config.Tag[Idx] != '\0'; Idx++) {
>     if ((Config.Tag[Idx] < 0x20) || (Config.Tag[Idx] > 0x7E)) {
>       Status = EFI_UNSUPPORTED;
>       goto Failed;
>     }
>
>     VirtioFs->Label[Idx] = Config.Tag[Idx];
>   }
>
>   VirtioFs->Label[Idx] = L'\0';
>
>   //
>   // 7.b. We need one queue for sending normal priority requests.
>   //
>   if (Config.NumReqQueues < 1) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   //
>   // 7.c. Fetch and remember the number of descriptors we can place on the
>   // queue at once. We'll need two descriptors per request, as a minimum --
>   // request header, response header.
>   //
>   Status = VirtioFs->Virtio->SetQueueSel (
>                                VirtioFs->Virtio,
>                                VIRTIO_FS_REQUEST_QUEUE
>                                );
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   Status = VirtioFs->Virtio->GetQueueNumMax (
>                                VirtioFs->Virtio,
>                                &VirtioFs->QueueSize
>                                );
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   if (VirtioFs->QueueSize < 2) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   //
>   // 7.d. [...] population of virtqueues [...]
>   //
>   Status = VirtioRingInit (
>              VirtioFs->Virtio,
>              VirtioFs->QueueSize,
>              &VirtioFs->Ring
>              );
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   Status = VirtioRingMap (
>              VirtioFs->Virtio,
>              &VirtioFs->Ring,
>              &RingBaseShift,
>              &VirtioFs->RingMap
>              );
>   if (EFI_ERROR (Status)) {
>     goto ReleaseQueue;
>   }
>
>   Status = VirtioFs->Virtio->SetQueueAddress (
>                                VirtioFs->Virtio,
>                                &VirtioFs->Ring,
>                                RingBaseShift
>                                );
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   //
>   // 8. Set the DRIVER_OK status bit.
>   //
>   NextDevStat |= VSTAT_DRIVER_OK;
>   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   return EFI_SUCCESS;
>
> UnmapQueue:
>   VirtioFs->Virtio->UnmapSharedBuffer (VirtioFs->Virtio, VirtioFs->RingMap);
>
> ReleaseQueue:
>   VirtioRingUninit (VirtioFs->Virtio, &VirtioFs->Ring);
>
> Failed:
>   //
>   // If any of these steps go irrecoverably wrong, the driver SHOULD set the
>   // FAILED status bit to indicate that it has given up on the device (it can
>   // reset the device later to restart if desired). [...]
>   //
>   // Virtio access failure here should not mask the original error.
>   //
>   NextDevStat |= VSTAT_FAILED;
>   VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
>
>   return Status;
> }

In the virtio-gpu driver:

> EFI_STATUS
> VirtioGpuInit (
>   IN OUT VGPU_DEV  *VgpuDev
>   )
> {
>   UINT8       NextDevStat;
>   EFI_STATUS  Status;
>   UINT64      Features;
>   UINT16      QueueSize;
>   UINT64      RingBaseShift;
>
>   //
>   // Execute virtio-v1.0-cs04, 3.1.1 Driver Requirements: Device
>   // Initialization.
>   //
>   // 1. Reset the device.
>   //
>   NextDevStat = 0;
>   Status      = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 2. Set the ACKNOWLEDGE status bit [...]
>   //
>   NextDevStat |= VSTAT_ACK;
>   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 3. Set the DRIVER status bit [...]
>   //
>   NextDevStat |= VSTAT_DRIVER;
>   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 4. Read device feature bits...
>   //
>   Status = VgpuDev->VirtIo->GetDeviceFeatures (VgpuDev->VirtIo, &Features);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   if ((Features & VIRTIO_F_VERSION_1) == 0) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   //
>   // We only want the most basic 2D features.
>   //
>   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
>
>   //
>   // ... and write the subset of feature bits understood by the [...] driver to
>   // the device. [...]
>   // 5. Set the FEATURES_OK status bit.
>   // 6. Re-read device status to ensure the FEATURES_OK bit is still set [...]
>   //
>   Status = Virtio10WriteFeatures (VgpuDev->VirtIo, Features, &NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // 7. Perform device-specific setup, including discovery of virtqueues for
>   // the device [...]
>   //
>   Status = VgpuDev->VirtIo->SetQueueSel (
>                               VgpuDev->VirtIo,
>                               VIRTIO_GPU_CONTROL_QUEUE
>                               );
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   Status = VgpuDev->VirtIo->GetQueueNumMax (VgpuDev->VirtIo, &QueueSize);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // We implement each VirtIo GPU command that we use with two descriptors:
>   // request, response.
>   //
>   if (QueueSize < 2) {
>     Status = EFI_UNSUPPORTED;
>     goto Failed;
>   }
>
>   //
>   // [...] population of virtqueues [...]
>   //
>   Status = VirtioRingInit (VgpuDev->VirtIo, QueueSize, &VgpuDev->Ring);
>   if (EFI_ERROR (Status)) {
>     goto Failed;
>   }
>
>   //
>   // If anything fails from here on, we have to release the ring.
>   //
>   Status = VirtioRingMap (
>              VgpuDev->VirtIo,
>              &VgpuDev->Ring,
>              &RingBaseShift,
>              &VgpuDev->RingMap
>              );
>   if (EFI_ERROR (Status)) {
>     goto ReleaseQueue;
>   }
>
>   //
>   // If anything fails from here on, we have to unmap the ring.
>   //
>   Status = VgpuDev->VirtIo->SetQueueAddress (
>                               VgpuDev->VirtIo,
>                               &VgpuDev->Ring,
>                               RingBaseShift
>                               );
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   //
>   // 8. Set the DRIVER_OK status bit.
>   //
>   NextDevStat |= VSTAT_DRIVER_OK;
>   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
>   if (EFI_ERROR (Status)) {
>     goto UnmapQueue;
>   }
>
>   return EFI_SUCCESS;
>
> UnmapQueue:
>   VgpuDev->VirtIo->UnmapSharedBuffer (VgpuDev->VirtIo, VgpuDev->RingMap);
>
> ReleaseQueue:
>   VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
>
> Failed:
>   //
>   // If any of these steps go irrecoverably wrong, the driver SHOULD set the
>   // FAILED status bit to indicate that it has given up on the device (it can
>   // reset the device later to restart if desired). [...]
>   //
>   // VirtIo access failure here should not mask the original error.
>   //
>   NextDevStat |= VSTAT_FAILED;
>   VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
>
>   return Status;
> }

Laszlo

Michael S. Tsirkin Nov. 9, 2022, 11 a.m. UTC | #14

On Wed, Nov 09, 2022 at 09:56:40AM +0100, Laszlo Ersek wrote:
> On 11/09/22 07:59, Michael S. Tsirkin wrote:
> > On Wed, Nov 09, 2022 at 01:52:01AM -0500, Michael S. Tsirkin wrote:
> >> On Wed, Nov 09, 2022 at 11:31:23AM +0800, Jason Wang wrote:
> >>> On Wed, Nov 9, 2022 at 3:43 AM Stefan Hajnoczi <[e-mail]> wrote:
> >>>>
> >>>> On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <[e-mail]> wrote:
> >>>>>
> >>>>> From: Kangjie Xu <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>> Signed-off-by: Xuan Zhuo <[e-mail]>
> >>>>> Acked-by: Jason Wang <[e-mail]>
> >>>>> Message-Id: <[e-mail]>
> >>>>> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> >>>>> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> >>>>> ---
> >>>>>  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?
> >
> > It's a bios bug:
> >
> >
> >
> >     vp_init_simple(&vdrive->vp, pci);
> >     if (vp_find_vq(&vdrive->vp, 0, &vdrive->vq) < 0 ) {
> >         dprintf(1, "fail to find vq for virtio-blk %pP\n", pci);
> >         goto fail;
> >     }
> >
> >     if (vdrive->vp.use_modern) {
> >         struct vp_device *vp = &vdrive->vp;
> >         u64 features = vp_get_features(vp);
> >         u64 version1 = 1ull << VIRTIO_F_VERSION_1;
> >         u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
> >         u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> >         if (!(features & version1)) {
> >             dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
> >             goto fail;
> >         }
> >
> >         features = features & (version1 | iommu_platform | blk_size);
> >         vp_set_features(vp, features);
> >         status |= VIRTIO_CONFIG_S_FEATURES_OK;
> >         vp_set_status(vp, status);
> >
> >
> >
> > Not good - does not match the spec. Here's what the spec says:
> >
> >
> > The driver MUST follow this sequence to initialize a device:
> > 1. Reset the device.
> > 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device.
> > 3. Set the DRIVER status bit: the guest OS knows how to drive the device.
> > 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the
> > device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration
> > fields to check that it can support the device before accepting it.
> > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step.
> > 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not
> > support our subset of features and the device is unusable.
> > 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup,
> > reading and possibly writing the device’s virtio configuration space, and population of virtqueues.
> > 8. Set the DRIVER_OK status bit. At this point the device is “live”.
> 
> Thanks for the Cc.
> 
> This should work properly in the edk2 (OVMF) guest drivers. When I
> started work on the virtio-1.0 implementation, I noticed that the device
> initialization sequence that guest drivers needed to follow had
> *changed* from spec version 0.9.5 to spec version 1.0.
> 
> For example, in the virtio-rng driver, I addressed that with commit
> 0a781bdc7f87 ("OvmfPkg: VirtioRngDxe: adapt feature negotiation to
> virtio-1.0", 2016-04-06):
> 
>   https://github.com/tianocore/edk2/commit/0a781bdc7f87


Thanks, and great analysis below!

Yes, in 0.9 it was:

1. Reset the device. This is not required on initial start up.
2. The ACKNOWLEDGE status bit is set: we have noticed the device.
3. The DRIVER status bit is set: we know how to drive the device.
4. Device-specific setup, including reading the Device Feature Bits, discov-
ery of virtqueues for the device, optional MSI-X setup, and reading and
possibly writing the virtio configuration space.
5. The subset of Device Feature Bits understood by the driver is written to
the device.
6. The DRIVER_OK status bit is set.
7. The device can now be used (ie. buffers added to the virtqueues)

Interestingly, Linux stopped following this order even for 0.9.X:

commit ef688e151c00e5d529703be9a04fd506df8bc54e
Author: Rusty Russell <[e-mail]>
Date:   Fri Jun 12 22:16:35 2009 -0600

    virtio: meet virtio spec by finalizing features before using device
    
    Virtio devices are supposed to negotiate features before they start using
    the device, but the current code doesn't do this.  This is because the
    driver's probe() function invariably has to add buffers to a virtqueue,
    or probe the disk (virtio_blk).
    
    This currently doesn't matter since no existing backend is strict about
    the feature negotiation.  But it's possible to imagine a future feature
    which completely changes how a device operates: in this case, we'd need
    to acknowledge it before using the device.
    
    Signed-off-by: Rusty Russell <[e-mail]>





So it's especially unfortunate that bios is still following
the 0.9 sequence even for 1.0.





> Therefore we have a larger context like this in edk2 (again the excerpt
> is from the virtio-rng driver, but all drivers follow this pattern whose
> devices can be either legacy or modern):
> 
> >   //
> >   // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
> >   //
> >   NextDevStat = 0;             // step 1 -- reset device
> >   Status      = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> 
> matches v1.0 spec step 1.
> 
> >
> >   NextDevStat |= VSTAT_ACK;    // step 2 -- acknowledge device presence
> >   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> 
> matches v1.0 spec step 2.
> 
> >
> >   NextDevStat |= VSTAT_DRIVER; // step 3 -- we know how to drive it
> >   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> 
> matches v1.0 spec step 3.
> 
> >
> >   //
> >   // Set Page Size - MMIO VirtIo Specific
> >   //
> >   Status = Dev->VirtIo->SetPageSize (Dev->VirtIo, EFI_PAGE_SIZE);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // step 4a -- retrieve and validate features
> >   //
> >   Status = Dev->VirtIo->GetDeviceFeatures (Dev->VirtIo, &Features);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> 
> matches v1.0 spec step 4, the "read" part
> 
> >
> >   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
> >
> >   //
> >   // In virtio-1.0, feature negotiation is expected to complete before queue
> >   // discovery, and the device can also reject the selected set of features.
> >   //
> >   if (Dev->VirtIo->Revision >= VIRTIO_SPEC_REVISION (1, 0, 0)) {
> >     Status = Virtio10WriteFeatures (Dev->VirtIo, Features, &NextDevStat);
> >     if (EFI_ERROR (Status)) {
> >       goto Failed;
> >     }
> >   }
> 
> This is skipped for virtio-0.9.5. For virtio-1.0,
> Virtio10WriteFeatures() does the following:
> 
> >   Status = VirtIo->SetGuestFeatures (VirtIo, Features);
> >   if (EFI_ERROR (Status)) {
> >     return Status;
> >   }
> 
> Covers v1.0 spec step 4, the rest of it.
> 
> >
> >   *DeviceStatus |= VSTAT_FEATURES_OK;
> >   Status         = VirtIo->SetDeviceStatus (VirtIo, *DeviceStatus);
> >   if (EFI_ERROR (Status)) {
> >     return Status;
> >   }
> 
> Covers v1.0 spec step 5.
> 
> >
> >   Status = VirtIo->GetDeviceStatus (VirtIo, DeviceStatus);
> >   if (EFI_ERROR (Status)) {
> >     return Status;
> >   }
> >
> >   if ((*DeviceStatus & VSTAT_FEATURES_OK) == 0) {
> >     Status = EFI_UNSUPPORTED;
> >   }
> 
> Covers v1.0 spec step 6.
> 
> OK, return to the previous call frame:
> 
> >
> >   //
> >   // step 4b -- allocate request virtqueue, just use #0
> >   //
> >   Status = Dev->VirtIo->SetQueueSel (Dev->VirtIo, 0);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   Status = Dev->VirtIo->GetQueueNumMax (Dev->VirtIo, &QueueSize);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // VirtioRngGetRNG() uses one descriptor
> >   //
> >   if (QueueSize < 1) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   Status = VirtioRingInit (Dev->VirtIo, QueueSize, &Dev->Ring);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // If anything fails from here on, we must release the ring resources.
> >   //
> >   Status = VirtioRingMap (
> >              Dev->VirtIo,
> >              &Dev->Ring,
> >              &RingBaseShift,
> >              &Dev->RingMap
> >              );
> >   if (EFI_ERROR (Status)) {
> >     goto ReleaseQueue;
> >   }
> >
> >   //
> >   // Additional steps for MMIO: align the queue appropriately, and set the
> >   // size. If anything fails from here on, we must unmap the ring resources.
> >   //
> >   Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize);
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE);
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   //
> >   // step 4c -- Report GPFN (guest-physical frame number) of queue.
> >   //
> >   Status = Dev->VirtIo->SetQueueAddress (
> >                           Dev->VirtIo,
> >                           &Dev->Ring,
> >                           RingBaseShift
> >                           );
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> 
> These cover v1.0 spec step 7.
> 
> >
> >   //
> >   // step 5 -- Report understood features and guest-tuneables.
> >   //
> >   if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) {
> >     Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM);
> >     Status    = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features);
> >     if (EFI_ERROR (Status)) {
> >       goto UnmapQueue;
> >     }
> >   }
> 
> This is exclusive to virtio-0.9.5; the "step 5" reference in the comment
> pertains to that spec version. In virtio-0.9.5, this is the location
> (just before setting DRIVER_OK) where the guest driver has to report its
> desired features (and the device has no means to reject any feature set
> that is otherwise a subset of the host feature set).
> 
> >
> >   //
> >   // step 6 -- initialization complete
> >   //
> >   NextDevStat |= VSTAT_DRIVER_OK;
> >   Status       = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> 
> This covers v1.0 spec step 8 (and v0.9.5 spec step 6, as the comment
> shows).
> 
> Now, in drivers whose devices are virtio-1.0-only, such as
> virtio-gpu-pci, vhost-user-fs-pci, this "selectivity" is not there. The
> virtio-0.9.5 branch is simply eliminated, and the virtio-1.0 logic is
> unconditional. Additionally, in those drivers, the "step" references in
> the code comments match the v1.0 bullet list. For example, in the
> virtio-fs driver:
> 
> > EFI_STATUS
> > VirtioFsInit (
> >   IN OUT VIRTIO_FS  *VirtioFs
> >   )
> > {
> >   UINT8             NextDevStat;
> >   EFI_STATUS        Status;
> >   UINT64            Features;
> >   VIRTIO_FS_CONFIG  Config;
> >   UINTN             Idx;
> >   UINT64            RingBaseShift;
> >
> >   //
> >   // Execute virtio-v1.1-cs01-87fa6b5d8155, 3.1.1 Driver Requirements: Device
> >   // Initialization.
> >   //
> >   // 1. Reset the device.
> >   //
> >   NextDevStat = 0;
> >   Status      = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 2. Set the ACKNOWLEDGE status bit [...]
> >   //
> >   NextDevStat |= VSTAT_ACK;
> >   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 3. Set the DRIVER status bit [...]
> >   //
> >   NextDevStat |= VSTAT_DRIVER;
> >   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 4. Read device feature bits...
> >   //
> >   Status = VirtioFs->Virtio->GetDeviceFeatures (VirtioFs->Virtio, &Features);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   if ((Features & VIRTIO_F_VERSION_1) == 0) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   //
> >   // No device-specific feature bits have been defined in file "virtio-fs.tex"
> >   // of the virtio spec at <https://github.com/oasis-tcs/virtio-spec.git>, as
> >   // of commit 87fa6b5d8155.
> >   //
> >   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
> >
> >   //
> >   // ... and write the subset of feature bits understood by the [...] driver to
> >   // the device. [...]
> >   // 5. Set the FEATURES_OK status bit.
> >   // 6. Re-read device status to ensure the FEATURES_OK bit is still set [...]
> >   //
> >   Status = Virtio10WriteFeatures (VirtioFs->Virtio, Features, &NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 7. Perform device-specific setup, including discovery of virtqueues for
> >   // the device, [...] reading [...] the device's virtio configuration space
> >   //
> >   Status = VirtioFsReadConfig (VirtioFs->Virtio, &Config);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 7.a. Convert the filesystem label from UTF-8 to UCS-2. Only labels with
> >   // printable ASCII code points (U+0020 through U+007E) are supported.
> >   // NUL-terminate at either the terminator we find, or right after the
> >   // original label.
> >   //
> >   for (Idx = 0; Idx < VIRTIO_FS_TAG_BYTES && Config.Tag[Idx] != '\0'; Idx++) {
> >     if ((Config.Tag[Idx] < 0x20) || (Config.Tag[Idx] > 0x7E)) {
> >       Status = EFI_UNSUPPORTED;
> >       goto Failed;
> >     }
> >
> >     VirtioFs->Label[Idx] = Config.Tag[Idx];
> >   }
> >
> >   VirtioFs->Label[Idx] = L'\0';
> >
> >   //
> >   // 7.b. We need one queue for sending normal priority requests.
> >   //
> >   if (Config.NumReqQueues < 1) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   //
> >   // 7.c. Fetch and remember the number of descriptors we can place on the
> >   // queue at once. We'll need two descriptors per request, as a minimum --
> >   // request header, response header.
> >   //
> >   Status = VirtioFs->Virtio->SetQueueSel (
> >                                VirtioFs->Virtio,
> >                                VIRTIO_FS_REQUEST_QUEUE
> >                                );
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   Status = VirtioFs->Virtio->GetQueueNumMax (
> >                                VirtioFs->Virtio,
> >                                &VirtioFs->QueueSize
> >                                );
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   if (VirtioFs->QueueSize < 2) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   //
> >   // 7.d. [...] population of virtqueues [...]
> >   //
> >   Status = VirtioRingInit (
> >              VirtioFs->Virtio,
> >              VirtioFs->QueueSize,
> >              &VirtioFs->Ring
> >              );
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   Status = VirtioRingMap (
> >              VirtioFs->Virtio,
> >              &VirtioFs->Ring,
> >              &RingBaseShift,
> >              &VirtioFs->RingMap
> >              );
> >   if (EFI_ERROR (Status)) {
> >     goto ReleaseQueue;
> >   }
> >
> >   Status = VirtioFs->Virtio->SetQueueAddress (
> >                                VirtioFs->Virtio,
> >                                &VirtioFs->Ring,
> >                                RingBaseShift
> >                                );
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   //
> >   // 8. Set the DRIVER_OK status bit.
> >   //
> >   NextDevStat |= VSTAT_DRIVER_OK;
> >   Status       = VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   return EFI_SUCCESS;
> >
> > UnmapQueue:
> >   VirtioFs->Virtio->UnmapSharedBuffer (VirtioFs->Virtio, VirtioFs->RingMap);
> >
> > ReleaseQueue:
> >   VirtioRingUninit (VirtioFs->Virtio, &VirtioFs->Ring);
> >
> > Failed:
> >   //
> >   // If any of these steps go irrecoverably wrong, the driver SHOULD set the
> >   // FAILED status bit to indicate that it has given up on the device (it can
> >   // reset the device later to restart if desired). [...]
> >   //
> >   // Virtio access failure here should not mask the original error.
> >   //
> >   NextDevStat |= VSTAT_FAILED;
> >   VirtioFs->Virtio->SetDeviceStatus (VirtioFs->Virtio, NextDevStat);
> >
> >   return Status;
> > }
> 
> In the virtio-gpu driver:
> 
> > EFI_STATUS
> > VirtioGpuInit (
> >   IN OUT VGPU_DEV  *VgpuDev
> >   )
> > {
> >   UINT8       NextDevStat;
> >   EFI_STATUS  Status;
> >   UINT64      Features;
> >   UINT16      QueueSize;
> >   UINT64      RingBaseShift;
> >
> >   //
> >   // Execute virtio-v1.0-cs04, 3.1.1 Driver Requirements: Device
> >   // Initialization.
> >   //
> >   // 1. Reset the device.
> >   //
> >   NextDevStat = 0;
> >   Status      = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 2. Set the ACKNOWLEDGE status bit [...]
> >   //
> >   NextDevStat |= VSTAT_ACK;
> >   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 3. Set the DRIVER status bit [...]
> >   //
> >   NextDevStat |= VSTAT_DRIVER;
> >   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 4. Read device feature bits...
> >   //
> >   Status = VgpuDev->VirtIo->GetDeviceFeatures (VgpuDev->VirtIo, &Features);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   if ((Features & VIRTIO_F_VERSION_1) == 0) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   //
> >   // We only want the most basic 2D features.
> >   //
> >   Features &= VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM;
> >
> >   //
> >   // ... and write the subset of feature bits understood by the [...] driver to
> >   // the device. [...]
> >   // 5. Set the FEATURES_OK status bit.
> >   // 6. Re-read device status to ensure the FEATURES_OK bit is still set [...]
> >   //
> >   Status = Virtio10WriteFeatures (VgpuDev->VirtIo, Features, &NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // 7. Perform device-specific setup, including discovery of virtqueues for
> >   // the device [...]
> >   //
> >   Status = VgpuDev->VirtIo->SetQueueSel (
> >                               VgpuDev->VirtIo,
> >                               VIRTIO_GPU_CONTROL_QUEUE
> >                               );
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   Status = VgpuDev->VirtIo->GetQueueNumMax (VgpuDev->VirtIo, &QueueSize);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // We implement each VirtIo GPU command that we use with two descriptors:
> >   // request, response.
> >   //
> >   if (QueueSize < 2) {
> >     Status = EFI_UNSUPPORTED;
> >     goto Failed;
> >   }
> >
> >   //
> >   // [...] population of virtqueues [...]
> >   //
> >   Status = VirtioRingInit (VgpuDev->VirtIo, QueueSize, &VgpuDev->Ring);
> >   if (EFI_ERROR (Status)) {
> >     goto Failed;
> >   }
> >
> >   //
> >   // If anything fails from here on, we have to release the ring.
> >   //
> >   Status = VirtioRingMap (
> >              VgpuDev->VirtIo,
> >              &VgpuDev->Ring,
> >              &RingBaseShift,
> >              &VgpuDev->RingMap
> >              );
> >   if (EFI_ERROR (Status)) {
> >     goto ReleaseQueue;
> >   }
> >
> >   //
> >   // If anything fails from here on, we have to unmap the ring.
> >   //
> >   Status = VgpuDev->VirtIo->SetQueueAddress (
> >                               VgpuDev->VirtIo,
> >                               &VgpuDev->Ring,
> >                               RingBaseShift
> >                               );
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   //
> >   // 8. Set the DRIVER_OK status bit.
> >   //
> >   NextDevStat |= VSTAT_DRIVER_OK;
> >   Status       = VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
> >   if (EFI_ERROR (Status)) {
> >     goto UnmapQueue;
> >   }
> >
> >   return EFI_SUCCESS;
> >
> > UnmapQueue:
> >   VgpuDev->VirtIo->UnmapSharedBuffer (VgpuDev->VirtIo, VgpuDev->RingMap);
> >
> > ReleaseQueue:
> >   VirtioRingUninit (VgpuDev->VirtIo, &VgpuDev->Ring);
> >
> > Failed:
> >   //
> >   // If any of these steps go irrecoverably wrong, the driver SHOULD set the
> >   // FAILED status bit to indicate that it has given up on the device (it can
> >   // reset the device later to restart if desired). [...]
> >   //
> >   // VirtIo access failure here should not mask the original error.
> >   //
> >   NextDevStat |= VSTAT_FAILED;
> >   VgpuDev->VirtIo->SetDeviceStatus (VgpuDev->VirtIo, NextDevStat);
> >
> >   return Status;
> > }
> 
> Laszlo

Patch

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.");
+    }
+
+    if (k->queue_enable) {
+        k->queue_enable(vdev, queue_index);
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;