On Tue, Feb 8, 2022 at 9:25 AM Jason Wang <jasow...@redhat.com> wrote: > > > 在 2022/2/1 下午7:45, Eugenio Perez Martin 写道: > > On Sun, Jan 30, 2022 at 7:50 AM Jason Wang <jasow...@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 <epere...@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); >