Re: [PATCH 28/31] vdpa: Expose VHOST_F_LOG_ALL on SVQ

[Eugenio Perez Martin]

On Tue, Feb 8, 2022 at 9:25 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/2/1 下午7:45, Eugenio Perez Martin 写道:
> > On Sun, Jan 30, 2022 at 7:50 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> >>> SVQ is able to log the dirty bits by itself, so let's use it to not
> >>> block migration.
> >>>
> >>> Also, ignore set and clear of VHOST_F_LOG_ALL on set_features if SVQ is
> >>> enabled. Even if the device supports it, the reports would be nonsense
> >>> because SVQ memory is in the qemu region.
> >>>
> >>> The log region is still allocated. Future changes might skip that, but
> >>> this series is already long enough.
> >>>
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>    hw/virtio/vhost-vdpa.c | 20 ++++++++++++++++++++
> >>>    1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>> index fb0a338baa..75090d65e8 100644
> >>> --- a/hw/virtio/vhost-vdpa.c
> >>> +++ b/hw/virtio/vhost-vdpa.c
> >>> @@ -1022,6 +1022,9 @@ static int vhost_vdpa_get_features(struct vhost_dev 
> >>> *dev, uint64_t *features)
> >>>        if (ret == 0 && v->shadow_vqs_enabled) {
> >>>            /* Filter only features that SVQ can offer to guest */
> >>>            vhost_svq_valid_guest_features(features);
> >>> +
> >>> +        /* Add SVQ logging capabilities */
> >>> +        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> >>>        }
> >>>
> >>>        return ret;
> >>> @@ -1039,8 +1042,25 @@ static int vhost_vdpa_set_features(struct 
> >>> vhost_dev *dev,
> >>>
> >>>        if (v->shadow_vqs_enabled) {
> >>>            uint64_t dev_features, svq_features, acked_features;
> >>> +        uint8_t status = 0;
> >>>            bool ok;
> >>>
> >>> +        ret = vhost_vdpa_call(dev, VHOST_VDPA_GET_STATUS, &status);
> >>> +        if (unlikely(ret)) {
> >>> +            return ret;
> >>> +        }
> >>> +
> >>> +        if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> >>> +            /*
> >>> +             * vhost is trying to enable or disable _F_LOG, and the 
> >>> device
> >>> +             * would report wrong dirty pages. SVQ handles it.
> >>> +             */
> >>
> >> I fail to understand this comment, I'd think there's no way to disable
> >> dirty page tracking for SVQ.
> >>
> > vhost_log_global_{start,stop} are called at the beginning and end of
> > migration. To inform the device that it should start logging, they set
> > or clean VHOST_F_LOG_ALL at vhost_dev_set_log.
>
>
> Yes, but for SVQ, we can't disable dirty page tracking, isn't it? The
> only thing is to ignore or filter out the F_LOG_ALL and pretend to be
> enabled and disabled.
>

Yes, that's what this patch does.

>
> >
> > While SVQ does not use VHOST_F_LOG_ALL, it exports the feature bit so
> > vhost does not block migration. Maybe we need to look for another way
> > to do this?
>
>
> I'm fine with filtering since it's much more simpler, but I fail to
> understand why we need to check DRIVER_OK.
>

Ok maybe I can make that part more clear,

Since both operations use vhost_vdpa_set_features we must just filter
the one that actually sets or removes VHOST_F_LOG_ALL, without
affecting other features.

In practice, that means to not forward the set features after
DRIVER_OK. The device is not expecting them anymore.

Does that make more sense?

Thanks!

> Thanks
>
>
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>
> >>> +            return 0;
> >>> +        }
> >>> +
> >>> +        /* We must not ack _F_LOG if SVQ is enabled */
> >>> +        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> >>> +
> >>>            ret = vhost_vdpa_get_dev_features(dev, &dev_features);
> >>>            if (ret != 0) {
> >>>                error_report("Can't get vdpa device features, got (%d)", 
> >>> ret);
>