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

[Jason Wang <jasowang@redhat.com>]

在 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.


>   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.

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)