| Message ID | 20220121202733.404989-29-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA shadow virtqueue | expand |
在 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. 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);
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. 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? 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); >
在 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. > > 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. 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);
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); >
On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > 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. I wonder what happens if we don't do this. So kernel had this check: /* * It's not allowed to change the features after they have * been negotiated. */ if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) return -EBUSY; So is it FEATURES_OK actually? For this patch, I wonder if the thing we need to do is to see whether it is a enable/disable F_LOG_ALL and simply return. Thanks > > 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); > > >
On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > 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. > > I wonder what happens if we don't do this. > If we simply delete the check vhost_dev_set_features will return an error, failing the start of the migration. More on this below. > So kernel had this check: > > /* > * It's not allowed to change the features after they have > * been negotiated. > */ > if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) > return -EBUSY; > > So is it FEATURES_OK actually? > Yes, FEATURES_OK seems more appropriate actually so I will switch to it for the next version. But it should be functionally equivalent, since vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot be concurrent with it. > For this patch, I wonder if the thing we need to do is to see whether > it is a enable/disable F_LOG_ALL and simply return. > Yes, that's the intention of the patch. We have 4 cases here: a) We're being called from vhost_dev_start, with enable_log = false b) We're being called from vhost_dev_start, with enable_log = true c) We're being called from vhost_dev_set_log, with enable_log = false d) We're being called from vhost_dev_set_log, with enable_log = true The way to tell the difference between a/b and c/d is to check if {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty memory through the memory unmapping, so we clear the bit unconditionally if we detect that VHOST_SET_FEATURES will be called (cases a and b). Another possibility is to track if features have been set with a bool in vhost_vdpa or something like that. But it seems cleaner to me to only store that in the actual device. > Thanks > > > > > 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); > > > > > >
在 2022/2/17 下午4:22, Eugenio Perez Martin 写道: > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin >> <eperezma@redhat.com> wrote: >>> 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. >> I wonder what happens if we don't do this. >> > If we simply delete the check vhost_dev_set_features will return an > error, failing the start of the migration. More on this below. Ok. > >> So kernel had this check: >> >> /* >> * It's not allowed to change the features after they have >> * been negotiated. >> */ >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) >> return -EBUSY; >> >> So is it FEATURES_OK actually? >> > Yes, FEATURES_OK seems more appropriate actually so I will switch to > it for the next version. > > But it should be functionally equivalent, since > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot > be concurrent with it. Right. > >> For this patch, I wonder if the thing we need to do is to see whether >> it is a enable/disable F_LOG_ALL and simply return. >> > Yes, that's the intention of the patch. > > We have 4 cases here: > a) We're being called from vhost_dev_start, with enable_log = false > b) We're being called from vhost_dev_start, with enable_log = true And this case makes us can't simply return without calling vhost-vdpa. > c) We're being called from vhost_dev_set_log, with enable_log = false > d) We're being called from vhost_dev_set_log, with enable_log = true > > The way to tell the difference between a/b and c/d is to check if > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty > memory through the memory unmapping, so we clear the bit > unconditionally if we detect that VHOST_SET_FEATURES will be called > (cases a and b). > > Another possibility is to track if features have been set with a bool > in vhost_vdpa or something like that. But it seems cleaner to me to > only store that in the actual device. So I suggest to make sure codes match the comment: 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. */ return 0; } It would be better to check whether the caller is toggling _F_LOG_ALL in this case. Thanks > >> Thanks >> >>> 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);
On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2022/2/17 下午4:22, Eugenio Perez Martin 写道: > > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: > >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin > >> <eperezma@redhat.com> wrote: > >>> 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. > >> I wonder what happens if we don't do this. > >> > > If we simply delete the check vhost_dev_set_features will return an > > error, failing the start of the migration. More on this below. > > > Ok. > > > > > >> So kernel had this check: > >> > >> /* > >> * It's not allowed to change the features after they have > >> * been negotiated. > >> */ > >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) > >> return -EBUSY; > >> > >> So is it FEATURES_OK actually? > >> > > Yes, FEATURES_OK seems more appropriate actually so I will switch to > > it for the next version. > > > > But it should be functionally equivalent, since > > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot > > be concurrent with it. > > > Right. > > > > > >> For this patch, I wonder if the thing we need to do is to see whether > >> it is a enable/disable F_LOG_ALL and simply return. > >> > > Yes, that's the intention of the patch. > > > > We have 4 cases here: > > a) We're being called from vhost_dev_start, with enable_log = false > > b) We're being called from vhost_dev_start, with enable_log = true > > > And this case makes us can't simply return without calling vhost-vdpa. > It calls because {FEATURES,DRIVER}_OK is still not set at that point. > > > c) We're being called from vhost_dev_set_log, with enable_log = false > > d) We're being called from vhost_dev_set_log, with enable_log = true > > > > The way to tell the difference between a/b and c/d is to check if > > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, > > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty > > memory through the memory unmapping, so we clear the bit > > unconditionally if we detect that VHOST_SET_FEATURES will be called > > (cases a and b). > > > > Another possibility is to track if features have been set with a bool > > in vhost_vdpa or something like that. But it seems cleaner to me to > > only store that in the actual device. > > > So I suggest to make sure codes match the comment: > > 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. > */ > return 0; > } > > It would be better to check whether the caller is toggling _F_LOG_ALL in > this case. > How to detect? We can save feature flags and compare, but ignoring all set_features after FEATURES_OK seems simpler to me. Would changing the comment work? Something like "set_features after _S_FEATURES_OK means vhost is trying to enable or disable _F_LOG, and the device would report wrong dirty pages. SVQ handles it." Thanks! > Thanks > > > > > >> Thanks > >> > >>> 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); >
On Tue, Feb 22, 2022 at 4:06 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > 在 2022/2/17 下午4:22, Eugenio Perez Martin 写道: > > > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: > > >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin > > >> <eperezma@redhat.com> wrote: > > >>> 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. > > >> I wonder what happens if we don't do this. > > >> > > > If we simply delete the check vhost_dev_set_features will return an > > > error, failing the start of the migration. More on this below. > > > > > > Ok. > > > > > > > > > >> So kernel had this check: > > >> > > >> /* > > >> * It's not allowed to change the features after they have > > >> * been negotiated. > > >> */ > > >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) > > >> return -EBUSY; > > >> > > >> So is it FEATURES_OK actually? > > >> > > > Yes, FEATURES_OK seems more appropriate actually so I will switch to > > > it for the next version. > > > > > > But it should be functionally equivalent, since > > > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot > > > be concurrent with it. > > > > > > Right. > > > > > > > > > >> For this patch, I wonder if the thing we need to do is to see whether > > >> it is a enable/disable F_LOG_ALL and simply return. > > >> > > > Yes, that's the intention of the patch. > > > > > > We have 4 cases here: > > > a) We're being called from vhost_dev_start, with enable_log = false > > > b) We're being called from vhost_dev_start, with enable_log = true > > > > > > And this case makes us can't simply return without calling vhost-vdpa. > > > > It calls because {FEATURES,DRIVER}_OK is still not set at that point. > > > > > > c) We're being called from vhost_dev_set_log, with enable_log = false > > > d) We're being called from vhost_dev_set_log, with enable_log = true > > > > > > The way to tell the difference between a/b and c/d is to check if > > > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, > > > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty > > > memory through the memory unmapping, so we clear the bit > > > unconditionally if we detect that VHOST_SET_FEATURES will be called > > > (cases a and b). > > > > > > Another possibility is to track if features have been set with a bool > > > in vhost_vdpa or something like that. But it seems cleaner to me to > > > only store that in the actual device. > > > > > > So I suggest to make sure codes match the comment: > > > > 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. > > */ > > return 0; > > } > > > > It would be better to check whether the caller is toggling _F_LOG_ALL in > > this case. > > > > How to detect? We can save feature flags and compare, but ignoring all > set_features after FEATURES_OK seems simpler to me. Something like: (status ^ status_old == _F_LOG_ALL) ? It helps us to return errors on wrong features set during DRIVER_OK. Thanks > > Would changing the comment work? Something like "set_features after > _S_FEATURES_OK means vhost is trying to enable or disable _F_LOG, and > the device would report wrong dirty pages. SVQ handles it." > > Thanks! > > > Thanks > > > > > > > > > >> Thanks > > >> > > >>> 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); > > >
On Wed, Feb 23, 2022 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Feb 22, 2022 at 4:06 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > 在 2022/2/17 下午4:22, Eugenio Perez Martin 写道: > > > > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: > > > >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin > > > >> <eperezma@redhat.com> wrote: > > > >>> 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. > > > >> I wonder what happens if we don't do this. > > > >> > > > > If we simply delete the check vhost_dev_set_features will return an > > > > error, failing the start of the migration. More on this below. > > > > > > > > > Ok. > > > > > > > > > > > > > >> So kernel had this check: > > > >> > > > >> /* > > > >> * It's not allowed to change the features after they have > > > >> * been negotiated. > > > >> */ > > > >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) > > > >> return -EBUSY; > > > >> > > > >> So is it FEATURES_OK actually? > > > >> > > > > Yes, FEATURES_OK seems more appropriate actually so I will switch to > > > > it for the next version. > > > > > > > > But it should be functionally equivalent, since > > > > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot > > > > be concurrent with it. > > > > > > > > > Right. > > > > > > > > > > > > > >> For this patch, I wonder if the thing we need to do is to see whether > > > >> it is a enable/disable F_LOG_ALL and simply return. > > > >> > > > > Yes, that's the intention of the patch. > > > > > > > > We have 4 cases here: > > > > a) We're being called from vhost_dev_start, with enable_log = false > > > > b) We're being called from vhost_dev_start, with enable_log = true > > > > > > > > > And this case makes us can't simply return without calling vhost-vdpa. > > > > > > > It calls because {FEATURES,DRIVER}_OK is still not set at that point. > > > > > > > > > c) We're being called from vhost_dev_set_log, with enable_log = false > > > > d) We're being called from vhost_dev_set_log, with enable_log = true > > > > > > > > The way to tell the difference between a/b and c/d is to check if > > > > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, > > > > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty > > > > memory through the memory unmapping, so we clear the bit > > > > unconditionally if we detect that VHOST_SET_FEATURES will be called > > > > (cases a and b). > > > > > > > > Another possibility is to track if features have been set with a bool > > > > in vhost_vdpa or something like that. But it seems cleaner to me to > > > > only store that in the actual device. > > > > > > > > > So I suggest to make sure codes match the comment: > > > > > > 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. > > > */ > > > return 0; > > > } > > > > > > It would be better to check whether the caller is toggling _F_LOG_ALL in > > > this case. > > > > > > > How to detect? We can save feature flags and compare, but ignoring all > > set_features after FEATURES_OK seems simpler to me. > > Something like: > > (status ^ status_old == _F_LOG_ALL) ? > s/status/features/ ? > It helps us to return errors on wrong features set during DRIVER_OK. > Do you mean to return errors in case of toggling other features than _F_LOG_ALL, isn't it? That's interesting actually, but it seems it forces vhost_vdpa to track acked_features too. Actually, it seems to me vhost_dev->acked_features will retain the bad features even on error. I'll investigate it. Thanks! > Thanks > > > > > Would changing the comment work? Something like "set_features after > > _S_FEATURES_OK means vhost is trying to enable or disable _F_LOG, and > > the device would report wrong dirty pages. SVQ handles it." > >
On Wed, Feb 23, 2022 at 4:06 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Wed, Feb 23, 2022 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Feb 22, 2022 at 4:06 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > 在 2022/2/17 下午4:22, Eugenio Perez Martin 写道: > > > > > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang@redhat.com> wrote: > > > > >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez Martin > > > > >> <eperezma@redhat.com> wrote: > > > > >>> 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. > > > > >> I wonder what happens if we don't do this. > > > > >> > > > > > If we simply delete the check vhost_dev_set_features will return an > > > > > error, failing the start of the migration. More on this below. > > > > > > > > > > > > Ok. > > > > > > > > > > > > > > > > > >> So kernel had this check: > > > > >> > > > > >> /* > > > > >> * It's not allowed to change the features after they have > > > > >> * been negotiated. > > > > >> */ > > > > >> if (ops->get_status(vdpa) & VIRTIO_CONFIG_S_FEATURES_OK) > > > > >> return -EBUSY; > > > > >> > > > > >> So is it FEATURES_OK actually? > > > > >> > > > > > Yes, FEATURES_OK seems more appropriate actually so I will switch to > > > > > it for the next version. > > > > > > > > > > But it should be functionally equivalent, since > > > > > vhost.c:vhost_dev_start sets both and the setting of _F_LOG_ALL cannot > > > > > be concurrent with it. > > > > > > > > > > > > Right. > > > > > > > > > > > > > > > > > >> For this patch, I wonder if the thing we need to do is to see whether > > > > >> it is a enable/disable F_LOG_ALL and simply return. > > > > >> > > > > > Yes, that's the intention of the patch. > > > > > > > > > > We have 4 cases here: > > > > > a) We're being called from vhost_dev_start, with enable_log = false > > > > > b) We're being called from vhost_dev_start, with enable_log = true > > > > > > > > > > > > And this case makes us can't simply return without calling vhost-vdpa. > > > > > > > > > > It calls because {FEATURES,DRIVER}_OK is still not set at that point. > > > > > > > > > > > > c) We're being called from vhost_dev_set_log, with enable_log = false > > > > > d) We're being called from vhost_dev_set_log, with enable_log = true > > > > > > > > > > The way to tell the difference between a/b and c/d is to check if > > > > > {FEATURES,DRIVER}_OK is set. And, as you point out in previous mails, > > > > > F_LOG_ALL must be filtered unconditionally since SVQ tracks dirty > > > > > memory through the memory unmapping, so we clear the bit > > > > > unconditionally if we detect that VHOST_SET_FEATURES will be called > > > > > (cases a and b). > > > > > > > > > > Another possibility is to track if features have been set with a bool > > > > > in vhost_vdpa or something like that. But it seems cleaner to me to > > > > > only store that in the actual device. > > > > > > > > > > > > So I suggest to make sure codes match the comment: > > > > > > > > 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. > > > > */ > > > > return 0; > > > > } > > > > > > > > It would be better to check whether the caller is toggling _F_LOG_ALL in > > > > this case. > > > > > > > > > > How to detect? We can save feature flags and compare, but ignoring all > > > set_features after FEATURES_OK seems simpler to me. > > > > Something like: > > > > (status ^ status_old == _F_LOG_ALL) ? > > > > s/status/features/ ? Right. > > > It helps us to return errors on wrong features set during DRIVER_OK. > > > > Do you mean to return errors in case of toggling other features than > _F_LOG_ALL, isn't it? That's interesting actually, but it seems it > forces vhost_vdpa to track acked_features too. I meant we can change the check a little bit like: if (featurs ^ features_old == _F_LOG_ALL && status & VIRTIO_CONFIG_S_DRIVER_OK) { return 0; } For other features changing we and let it go down the logic as you proposed in this patch. Thanks > > Actually, it seems to me vhost_dev->acked_features will retain the bad > features even on error. I'll investigate it. > > Thanks! > > > > Thanks > > > > > > > > Would changing the comment work? Something like "set_features after > > > _S_FEATURES_OK means vhost is trying to enable or disable _F_LOG, and > > > the device would report wrong dirty pages. SVQ handles it." > > > >
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. + */ + 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);
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(+)