Re: [PATCH v2 07/24] virtio-pci: support queue enable

[Kangjie Xu]

在 2022/8/24 16:59, Jason Wang 写道:

>       
>       在 2022/8/23 16:20, Kangjie Xu 写道:
>       
> >         
> >         在 2022/8/23 15:44, Jason Wang 写道:
> >         
> > >           
> > >           在 2022/8/16 09:06, Kangjie Xu 写道:
> > >           
> > > > PCI devices support vq enable.
> > > >             
> > > >           
> > >           
> > >           
> > >           Nit: it might be "support device specific vq enable"
> > >           
> > >           
> > >           
> > >         
> >         Get it.
> >         
> > > >             
> > > >             Based on this function, the driver can re-enable the
> > > >             virtqueue after the
> > > >             
> > > >             virtqueue is reset.
> > > >             
> > > >             
> > > >             Signed-off-by: Kangjie Xu
> > > >             <kangjie.xu@linux.alibaba.com>
> > > >             
> > > >             Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > >             
> > > >             ---
> > > >             
> > > >               hw/virtio/virtio-pci.c | 1 +
> > > >             
> > > >               1 file changed, 1 insertion(+)
> > > >             
> > > >             
> > > >             diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > >             
> > > >             index ec8e92052f..3d560e45ad 100644
> > > >             
> > > >             --- a/hw/virtio/virtio-pci.c
> > > >             
> > > >             +++ b/hw/virtio/virtio-pci.c
> > > >             
> > > >             @@ -1335,6 +1335,7 @@ static void
> > > >             virtio_pci_common_write(void *opaque, hwaddr addr,
> > > >             
> > > >             proxy->vqs[vdev->queue_sel].avail[0],
> > > >             
> > > >             ((uint64_t)proxy->vqs[vdev->queue_sel].used[1])
> > > >             << 32 |
> > > >             
> > > >             proxy->vqs[vdev->queue_sel].used[0]);
> > > >             
> > > >             +            virtio_queue_enable(vdev, vdev->queue_sel);
> > > >             
> > > >                           proxy->vqs[vdev->queue_sel].enabled = 1;
> > > >             
> > > >                           proxy->vqs[vdev->queue_sel].reset = 0;
> > > >             
> > > >           
> > >           
> > >           
> > >           Any reason we do it before the assignment of 1? It probably
> > >           means the device specific method can't depend on
> > >           virtio_queue_enabled()?
> > >           
> > >           
> > >           Thanks
> > >           
> > >           
> > >         
> >         Sorry, I don't get why device specific method can't depend on
> >         virtio_queue_enabled().
> >         
> >       
>       
>       
>       I meant if the device specific method call virtio_queue_enabled()
>       it will return false in this case, is this intended?
>       
>       
>     
    Yes, I intend it to behave in this way.
>       
> >         
> >         Before virtio_queue_enable() is done, virtqueue should always be
> >         not ready and disabled.
> >         
> >         
> >         Otherwise, If we put it after the assignment of enabled to 1,
> >         the virtqueue may be accessed illegally and may cause panic,
> >         because the virtqueue is still being intialized and being
> >         configured.
> >         
> >       
>       
>       
>       How? Shouldn't we make transport ready before making device
>       virtqueue(device) ready?
>       
>       
>       Thanks
>       
>       
>

I am not experienced in this field, could you tell me why we should make the transport ready first?

I make the transport ready later than making device ready for two aspects:

1. In QEMU, the virtio_queue_enabled() is used only when we start the device/queue pair (vhost_dev_start_one), or reading VIRTIO_PCI_COMMON_Q_ENABLE. These two operations and resetting the queue will be synchronized using iothread lock, so we do not need to worry about the case currently.

2. Suppose we use virtio_queue_enabled() or access the enabled status asynchronously, and we make the transport ready first.

After enabled set to true, and before virtio_queue_enable() is finished, somewhere  that call virtio_queue_enabled()  or access the enabled status of VirtIOPCIQueue. Then the caller will consider the virtqueue as enabled(enabled = true in VirtIOPCIQueue). The caller might access the virtqueue(access avail ring / desc table). But I think the access here is illegal because the virtqueue might still be unintialized status.

Thus, from my perspective, to prevent illegal access, we need to make transport ready after virtio_queue_enable().

Thanks

>       
> >         
> >         Thanks
> >         
> >         
> > >           
> > > >           } else {
> > > >             
> > > >           
> > >         
> >         
> >       
>