On Wed, Feb 23, 2022 at 4:06 PM Eugenio Perez Martin
<eperezma at redhat.com> wrote:>
> On Wed, Feb 23, 2022 at 4:47 AM Jason Wang <jasowang at redhat.com>
wrote:
> >
> > On Tue, Feb 22, 2022 at 4:06 PM Eugenio Perez Martin
> > <eperezma at redhat.com> wrote:
> > >
> > > On Tue, Feb 22, 2022 at 8:41 AM Jason Wang <jasowang at
redhat.com> wrote:
> > > >
> > > >
> > > > ? 2022/2/17 ??4:22, Eugenio Perez Martin ??:
> > > > > On Thu, Feb 17, 2022 at 7:02 AM Jason Wang <jasowang
at redhat.com> wrote:
> > > > >> On Wed, Feb 16, 2022 at 11:54 PM Eugenio Perez
Martin
> > > > >> <eperezma at redhat.com> wrote:
> > > > >>> On Tue, Feb 8, 2022 at 9:25 AM Jason Wang
<jasowang at redhat.com> wrote:
> > > > >>>>
> > > > >>>> ? 2022/2/1 ??7:45, Eugenio Perez Martin ??:
> > > > >>>>> On Sun, Jan 30, 2022 at 7:50 AM Jason
Wang <jasowang at 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 at 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."
> > >
>