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

[Michael S. Tsirkin]

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 <stefanha@gmail.com> wrote:
> > >
> > > On Mon, 7 Nov 2022 at 18:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > >
> > > > Introduce the interface queue_enable() in VirtioDeviceClass and the
> > > > fucntion virtio_queue_enable() in virtio, it can be called when
> > > > VIRTIO_PCI_COMMON_Q_ENABLE is written and related virtqueue can be
> > > > started. It only supports the devices of virtio 1 or later. The
> > > > not-supported devices can only start the virtqueue when DRIVER_OK.
> > > >
> > > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > Message-Id: <20221017092558.111082-4-xuanzhuo@linux.alibaba.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/hw/virtio/virtio.h |  2 ++
> > > >  hw/virtio/virtio.c         | 14 ++++++++++++++
> > > >  2 files changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > > index 74d76c1dbc..b00b3fcf31 100644
> > > > --- a/include/hw/virtio/virtio.h
> > > > +++ b/include/hw/virtio/virtio.h
> > > > @@ -149,6 +149,7 @@ struct VirtioDeviceClass {
> > > >      void (*reset)(VirtIODevice *vdev);
> > > >      void (*set_status)(VirtIODevice *vdev, uint8_t val);
> > > >      void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> > > > +    void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
> > > >      /* For transitional devices, this is a bitmap of features
> > > >       * that are only exposed on the legacy interface but not
> > > >       * the modern one.
> > > > @@ -288,6 +289,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice 
> > > > *vdev, int n,
> > > >  int virtio_set_status(VirtIODevice *vdev, uint8_t val);
> > > >  void virtio_reset(void *opaque);
> > > >  void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> > > >  void virtio_update_irq(VirtIODevice *vdev);
> > > >  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index cf5f9ca387..9683b2e158 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -2495,6 +2495,20 @@ void virtio_queue_reset(VirtIODevice *vdev, 
> > > > uint32_t queue_index)
> > > >      __virtio_queue_reset(vdev, queue_index);
> > > >  }
> > > >
> > > > +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > > > +{
> > > > +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > > > +
> > > > +    if (!virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> > > > +        error_report("queue_enable is only suppported in devices of 
> > > > virtio "
> > > > +                     "1.0 or later.");
> > >
> > > Why is this triggering here? Maybe virtio_queue_enable() is called too
> > > early. I have verified that the Linux guest driver sets VERSION_1. I
> > > didn't check what SeaBIOS does.
> > 
> > Looks like a bug, we should check device features here at least and it
> > should be guest errors instead of error_report() here.
> > 
> > Thanks
> 
> But it's weird, queue enable is written before guest features?
> How come?

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