|
| From: | Jason Wang |
| Subject: | Re: [PATCH v6 05/10] vdpa: move SVQ vring features check to net/ |
| Date: | Mon, 14 Nov 2022 12:26:00 +0800 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 |
在 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. ThanksThanks!ThanksThanks!ThanksSince 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)
| [Prev in Thread] | Current Thread | [Next in Thread] |