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

[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

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