Re: [PATCH v6 05/10] vdpa: move SVQ vring features check to net/

[Jason Wang <jasowang@redhat.com>]

在 2022/11/11 20:58, Eugenio Perez Martin 写道:
> On Fri, Nov 11, 2022 at 9:07 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Fri, Nov 11, 2022 at 3:56 PM Eugenio Perez Martin
>> <eperezma@redhat.com> wrote:
>>> On Fri, Nov 11, 2022 at 8:34 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> 在 2022/11/10 21:09, Eugenio Perez Martin 写道:
>>>>> On Thu, Nov 10, 2022 at 6:40 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> 在 2022/11/9 01:07, Eugenio Pérez 写道:
>>>>>>> The next patches will start control SVQ if possible. However, we don't
>>>>>>> know if that will be possible at qemu boot anymore.
>>>>>> If I was not wrong, there's no device specific feature that is checked
>>>>>> in the function. So it should be general enough to be used by devices
>>>>>> other than net. Then I don't see any advantage of doing this.
>>>>>>
>>>>> Because vhost_vdpa_init_svq is called at qemu boot, failing if it is
>>>>> not possible to shadow the Virtqueue.
>>>>>
>>>>> Now the CVQ will be shadowed if possible, so we need to check this at
>>>>> device start, not at initialization.
>>>>
>>>> Any reason we can't check this at device start? We don't need
>>>> driver_features and we can do any probing to make sure cvq has an unique
>>>> group during initialization time.
>>>>
>>> We need the CVQ index to check if it has an independent group. CVQ
>>> index depends on the features the guest's ack:
>>> * If it acks _F_MQ, it is the last one.
>>> * If it doesn't, CVQ idx is 2.
>>>
>>> We cannot have acked features at initialization, and they could
>>> change: It is valid for a guest to ack _F_MQ, then reset the device,
>>> then not ack it.
>> Can we do some probing by negotiating _F_MQ if the device offers it,
>> then we can know if cvq has a unique group?
>>
> What if the guest does not ack _F_MQ?
>
> To be completed it would go like:
>
> * Probe negotiate _F_MQ, check unique group,
> * Probe negotiate !_F_MQ, check unique group,


I think it should be a bug if device present a unique virtqueue group 
that depends on a specific feature. That is to say, we can do a single 
round of probing instead of try it twice here.


> * Actually negotiate with the guest's feature set.
> * React to failures. Probably the same way as if the CVQ is not
> isolated, disabling SVQ?
>
> To me it seems simpler to specify somehow that the vq must be independent.


It's just a suggestion, if you think doing it at the start, I'm fine. 
But we need document the reason with a comment maybe.

Thanks


>
> Thanks!
>
>>>>>    To store this information at boot
>>>>> time is not valid anymore, because v->shadow_vqs_enabled is not valid
>>>>> at this time anymore.
>>>>
>>>> Ok, but this doesn't explain why it is net specific but vhost-vdpa specific.
>>>>
>>> We can try to move it to a vhost op, but we have the same problem as
>>> the svq array allocation: We don't have the right place in vhost ops
>>> to check this. Maybe vhost_set_features is the right one here?
>> If we can do all the probing at the initialization phase, we can do
>> everything there.
>>
>> Thanks
>>
>>> Thanks!
>>>
>>>> Thanks
>>>>
>>>>
>>>>> Thanks!
>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>> Since the moved checks will be already evaluated at net/ to know if it
>>>>>>> is ok to shadow CVQ, move them.
>>>>>>>
>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>> ---
>>>>>>>     hw/virtio/vhost-vdpa.c | 33 ++-------------------------------
>>>>>>>     net/vhost-vdpa.c       |  3 ++-
>>>>>>>     2 files changed, 4 insertions(+), 32 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>>>>> index 3df2775760..146f0dcb40 100644
>>>>>>> --- a/hw/virtio/vhost-vdpa.c
>>>>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>>>>> @@ -402,29 +402,9 @@ static int vhost_vdpa_get_dev_features(struct vhost_dev *dev,
>>>>>>>         return ret;
>>>>>>>     }
>>>>>>>
>>>>>>> -static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
>>>>>>> -                               Error **errp)
>>>>>>> +static void vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v)
>>>>>>>     {
>>>>>>>         g_autoptr(GPtrArray) shadow_vqs = NULL;
>>>>>>> -    uint64_t dev_features, svq_features;
>>>>>>> -    int r;
>>>>>>> -    bool ok;
>>>>>>> -
>>>>>>> -    if (!v->shadow_vqs_enabled) {
>>>>>>> -        return 0;
>>>>>>> -    }
>>>>>>> -
>>>>>>> -    r = vhost_vdpa_get_dev_features(hdev, &dev_features);
>>>>>>> -    if (r != 0) {
>>>>>>> -        error_setg_errno(errp, -r, "Can't get vdpa device features");
>>>>>>> -        return r;
>>>>>>> -    }
>>>>>>> -
>>>>>>> -    svq_features = dev_features;
>>>>>>> -    ok = vhost_svq_valid_features(svq_features, errp);
>>>>>>> -    if (unlikely(!ok)) {
>>>>>>> -        return -1;
>>>>>>> -    }
>>>>>>>
>>>>>>>         shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_svq_free);
>>>>>>>         for (unsigned n = 0; n < hdev->nvqs; ++n) {
>>>>>>> @@ -436,7 +416,6 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v,
>>>>>>>         }
>>>>>>>
>>>>>>>         v->shadow_vqs = g_steal_pointer(&shadow_vqs);
>>>>>>> -    return 0;
>>>>>>>     }
>>>>>>>
>>>>>>>     static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>>>>>>> @@ -461,11 +440,7 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>>>>>>>         dev->opaque =  opaque ;
>>>>>>>         v->listener = vhost_vdpa_memory_listener;
>>>>>>>         v->msg_type = VHOST_IOTLB_MSG_V2;
>>>>>>> -    ret = vhost_vdpa_init_svq(dev, v, errp);
>>>>>>> -    if (ret) {
>>>>>>> -        goto err;
>>>>>>> -    }
>>>>>>> -
>>>>>>> +    vhost_vdpa_init_svq(dev, v);
>>>>>>>         vhost_vdpa_get_iova_range(v);
>>>>>>>
>>>>>>>         if (!vhost_vdpa_first_dev(dev)) {
>>>>>>> @@ -476,10 +451,6 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>>>>>>>                                    VIRTIO_CONFIG_S_DRIVER);
>>>>>>>
>>>>>>>         return 0;
>>>>>>> -
>>>>>>> -err:
>>>>>>> -    ram_block_discard_disable(false);
>>>>>>> -    return ret;
>>>>>>>     }
>>>>>>>
>>>>>>>     static void vhost_vdpa_host_notifier_uninit(struct vhost_dev *dev,
>>>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>>>> index d3b1de481b..fb35b17ab4 100644
>>>>>>> --- a/net/vhost-vdpa.c
>>>>>>> +++ b/net/vhost-vdpa.c
>>>>>>> @@ -117,9 +117,10 @@ static bool vhost_vdpa_net_valid_svq_features(uint64_t features, Error **errp)
>>>>>>>         if (invalid_dev_features) {
>>>>>>>             error_setg(errp, "vdpa svq does not work with features 0x%" PRIx64,
>>>>>>>                        invalid_dev_features);
>>>>>>> +        return false;
>>>>>>>         }
>>>>>>>
>>>>>>> -    return !invalid_dev_features;
>>>>>>> +    return vhost_svq_valid_features(features, errp);
>>>>>>>     }
>>>>>>>
>>>>>>>     static int vhost_vdpa_net_check_device_id(struct vhost_net *net)