? 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.
> 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);