Re: [PATCH v2 06/24] virtio-pci: support queue reset

[Kangjie Xu]

在 2022/8/23 15:40, Jason Wang 写道:
> 在 2022/8/16 09:06, Kangjie Xu 写道:
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >
> > PCI devices support vq reset.
> >
> > Based on this function, the driver can adjust the size of the ring, and
> > quickly recycle the buffer in the ring.
> >
> > The migration of the virtio devices will not happen during a reset
> > operation. This is becuase the global iothread lock is held. Migration
> > thread also needs the lock. As a result, we do not need to migrate the
> > reset state of VirtIOPCIQueue.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > ---
> >   hw/virtio/virtio-pci.c         | 19 +++++++++++++++++++
> >   include/hw/virtio/virtio-pci.h |  1 +
> >   2 files changed, 20 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 45327f0b31..ec8e92052f 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1246,6 +1246,9 @@ static uint64_t virtio_pci_common_read(void 
> > *opaque, hwaddr addr,
> >       case VIRTIO_PCI_COMMON_Q_USEDHI:
> >           val = proxy->vqs[vdev->queue_sel].used[1];
> >           break;
> > +    case VIRTIO_PCI_COMMON_Q_RESET:
> > +        val = proxy->vqs[vdev->queue_sel].reset;
> > +        break;
> >       default:
> >           val = 0;
> >       }
> > @@ -1333,6 +1336,7 @@ static void virtio_pci_common_write(void 
> > *opaque, hwaddr addr,
> > ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
> > proxy->vqs[vdev->queue_sel].used[0]);
> >               proxy->vqs[vdev->queue_sel].enabled = 1;
> > +            proxy->vqs[vdev->queue_sel].reset = 0;
> >           } else {
> >               virtio_error(vdev, "wrong value for queue_enable 
> > %"PRIx64, val);
> >           }
> > @@ -1355,6 +1359,20 @@ static void virtio_pci_common_write(void 
> > *opaque, hwaddr addr,
> >       case VIRTIO_PCI_COMMON_Q_USEDHI:
> >           proxy->vqs[vdev->queue_sel].used[1] = val;
> >           break;
> > +    case VIRTIO_PCI_COMMON_Q_RESET:
> > +        if (val == 1) {
> > +            /*
> > +             * With the global iothread lock taken, the migration 
> > will not
> > +             * happen until the virtqueue reset is done.
> > +             */
>
>
> This comment applies to all other common cfg operation as well, So it 
> looks not necessary?
Get it.

> > + proxy->vqs[vdev->queue_sel].reset = 1;
> > +
> > +            virtio_queue_reset(vdev, vdev->queue_sel);
> > +
> > +            proxy->vqs[vdev->queue_sel].reset = 0;
> > +            proxy->vqs[vdev->queue_sel].enabled = 0;
> > +        }
> > +        break;
> >       default:
> >           break;
> >       }
> > @@ -1950,6 +1968,7 @@ static void virtio_pci_reset(DeviceState *qdev)
> >         for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> >           proxy->vqs[i].enabled = 0;
> > +        proxy->vqs[i].reset = 0;
> >           proxy->vqs[i].num = 0;
> >           proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
> >           proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
> > diff --git a/include/hw/virtio/virtio-pci.h 
> > b/include/hw/virtio/virtio-pci.h
> > index 2446dcd9ae..e9290e2b94 100644
> > --- a/include/hw/virtio/virtio-pci.h
> > +++ b/include/hw/virtio/virtio-pci.h
> > @@ -117,6 +117,7 @@ typedef struct VirtIOPCIRegion {
> >   typedef struct VirtIOPCIQueue {
> >     uint16_t num;
> >     bool enabled;
> > +  bool reset;
>
>
> Do we need to migrate this?
>
> Thanks
I think we do not need to migrate this because we hold the global 
iothread lock when virtqueue reset is triggered. The migration of these 
device states also needs this lock.

On the other hand, the 'reset' state of virtqueue is same(is 0) before 
and after the process of resetting a virtqueue.

Thus, the migration will not happen when we are resetting a virtqueue 
and we do not to migrate it.

Thanks

> >     uint32_t desc[2];
> >     uint32_t avail[2];
> >     uint32_t used[2];