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

[Kangjie Xu]

在 2022/8/25 10:52, Jason Wang 写道:
> On Wed, Aug 24, 2022 at 7:27 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> > 在 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?
> That's a must for the device to work.
>
> E.g for PCI device, I can't image the device is ready to work before
> PCI is ready.
>
> > 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.
> Note that virtio_queue_enabled() is an exported helper, you can't
> assume how it will be used in the future.
>
> > 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.
> How can this happen, won't we call device specific method after we set
> queue_enabled as true? It's the charge of the device specific method
> to synchronize the necessary external I/O in this case.
>
> Thanks

I get your point.

You are right. I realized that I ignored the external I/O.

Will fix it.

Thanks

> > Thus, from my perspective, to prevent illegal access, we need to make transport 
> > ready after virtio_queue_enable().
> >
> > Thanks
> >
> >
> >
> > Thanks
> >
> >
> >            } else {