On Wed, Aug 24, 2022 at 7:27 PM Kangjie Xu <kangjie...@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...@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanz...@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 {
>
>
