Jason Wang
2022-Feb-08 03:37 UTC
[PATCH 11/31] vhost: Add vhost_svq_valid_device_features to shadow vq
? 2022/2/1 ??6:57, Eugenio Perez Martin ??:> On Mon, Jan 31, 2022 at 4:49 PM Eugenio Perez Martin > <eperezma at redhat.com> wrote: >> On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang at redhat.com> wrote: >>> >>> ? 2022/1/22 ??4:27, Eugenio P?rez ??: >>>> This allows SVQ to negotiate features with the device. For the device, >>>> SVQ is a driver. While this function needs to bypass all non-transport >>>> features, it needs to disable the features that SVQ does not support >>>> when forwarding buffers. This includes packed vq layout, indirect >>>> descriptors or event idx. >>>> >>>> Signed-off-by: Eugenio P?rez <eperezma at redhat.com> >>>> --- >>>> hw/virtio/vhost-shadow-virtqueue.h | 2 ++ >>>> hw/virtio/vhost-shadow-virtqueue.c | 44 ++++++++++++++++++++++++++++++ >>>> hw/virtio/vhost-vdpa.c | 21 ++++++++++++++ >>>> 3 files changed, 67 insertions(+) >>>> >>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h >>>> index c9ffa11fce..d963867a04 100644 >>>> --- a/hw/virtio/vhost-shadow-virtqueue.h >>>> +++ b/hw/virtio/vhost-shadow-virtqueue.h >>>> @@ -15,6 +15,8 @@ >>>> >>>> typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; >>>> >>>> +bool vhost_svq_valid_device_features(uint64_t *features); >>>> + >>>> void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd); >>>> void vhost_svq_set_guest_call_notifier(VhostShadowVirtqueue *svq, int call_fd); >>>> const EventNotifier *vhost_svq_get_dev_kick_notifier( >>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c >>>> index 9619c8082c..51442b3dbf 100644 >>>> --- a/hw/virtio/vhost-shadow-virtqueue.c >>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c >>>> @@ -45,6 +45,50 @@ const EventNotifier *vhost_svq_get_dev_kick_notifier( >>>> return &svq->hdev_kick; >>>> } >>>> >>>> +/** >>>> + * Validate the transport device features that SVQ can use with the device >>>> + * >>>> + * @dev_features The device features. If success, the acknowledged features. >>>> + * >>>> + * Returns true if SVQ can go with a subset of these, false otherwise. >>>> + */ >>>> +bool vhost_svq_valid_device_features(uint64_t *dev_features) >>>> +{ >>>> + bool r = true; >>>> + >>>> + for (uint64_t b = VIRTIO_TRANSPORT_F_START; b <= VIRTIO_TRANSPORT_F_END; >>>> + ++b) { >>>> + switch (b) { >>>> + case VIRTIO_F_NOTIFY_ON_EMPTY: >>>> + case VIRTIO_F_ANY_LAYOUT: >>>> + continue; >>>> + >>>> + case VIRTIO_F_ACCESS_PLATFORM: >>>> + /* SVQ does not know how to translate addresses */ >>> >>> I may miss something but any reason that we need to disable >>> ACCESS_PLATFORM? I'd expect the vring helper we used for shadow >>> virtqueue can deal with vIOMMU perfectly. >>> >> This function is validating SVQ <-> Device communications features, >> that may or may not be the same as guest <-> SVQ. These feature flags >> are valid for guest <-> SVQ communication, same as with indirect >> descriptors one. >> >> Having said that, there is a point in the series where >> VIRTIO_F_ACCESS_PLATFORM is actually mandatory, so I think we could >> use the latter addition of x-svq cmdline parameter and delay the >> feature validations where it makes more sense. >> >>>> + if (*dev_features & BIT_ULL(b)) { >>>> + clear_bit(b, dev_features); >>>> + r = false; >>>> + } >>>> + break; >>>> + >>>> + case VIRTIO_F_VERSION_1: >>> >>> I had the same question here. >>> >> For VERSION_1 it's easier to assume that guest is little endian at >> some points, but we could try harder to support both endianness if >> needed. >> > Re-thinking the SVQ feature isolation stuff for this first iteration > based on your comments. > > Maybe it's easier to simply fail if the device does not *match* the > expected feature set, and add all of the "feature isolation" later. > While a lot of guest <-> SVQ communication details are already solved > for free with qemu's VirtQueue (indirect, packed, ...), we may > simplify this series in particular and add the support for it later. > > For example, at this moment would be valid for the device to export > indirect descriptors feature flag, and SVQ simply forward that feature > flag offering to the guest. So the guest <-> SVQ communication could > have indirect descriptors (qemu's VirtQueue code handles it for free), > but SVQ would not acknowledge it for the device. As a side note, to > negotiate it would have been harmless actually, but it's not the case > of packed vq. > > So maybe for the v2 we can simply force the device to just export the > strictly needed features and nothing else with qemu cmdline, and then > enable the feature negotiation isolation for each side of SVQ?Yes, that's exactly my point. Thanks> > Thanks! > > >> Thanks! >> >>> Thanks >>> >>> >>>> + /* SVQ trust that guest vring is little endian */ >>>> + if (!(*dev_features & BIT_ULL(b))) { >>>> + set_bit(b, dev_features); >>>> + r = false; >>>> + } >>>> + continue; >>>> + >>>> + default: >>>> + if (*dev_features & BIT_ULL(b)) { >>>> + clear_bit(b, dev_features); >>>> + } >>>> + } >>>> + } >>>> + >>>> + return r; >>>> +} >>>> + >>>> /* Forward guest notifications */ >>>> static void vhost_handle_guest_kick(EventNotifier *n) >>>> { >>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>>> index bdb45c8808..9d801cf907 100644 >>>> --- a/hw/virtio/vhost-vdpa.c >>>> +++ b/hw/virtio/vhost-vdpa.c >>>> @@ -855,10 +855,31 @@ static int vhost_vdpa_init_svq(struct vhost_dev *hdev, struct vhost_vdpa *v, >>>> size_t n_svqs = v->shadow_vqs_enabled ? hdev->nvqs : 0; >>>> g_autoptr(GPtrArray) shadow_vqs = g_ptr_array_new_full(n_svqs, >>>> vhost_psvq_free); >>>> + uint64_t dev_features; >>>> + uint64_t svq_features; >>>> + int r; >>>> + bool ok; >>>> + >>>> if (!v->shadow_vqs_enabled) { >>>> goto out; >>>> } >>>> >>>> + r = vhost_vdpa_get_features(hdev, &dev_features); >>>> + if (r != 0) { >>>> + error_setg(errp, "Can't get vdpa device features, got (%d)", r); >>>> + return r; >>>> + } >>>> + >>>> + svq_features = dev_features; >>>> + ok = vhost_svq_valid_device_features(&svq_features); >>>> + if (unlikely(!ok)) { >>>> + error_setg(errp, >>>> + "SVQ Invalid device feature flags, offer: 0x%"PRIx64", ok: 0x%"PRIx64, >>>> + hdev->features, svq_features); >>>> + return -1; >>>> + } >>>> + >>>> + shadow_vqs = g_ptr_array_new_full(hdev->nvqs, vhost_psvq_free); >>>> for (unsigned n = 0; n < hdev->nvqs; ++n) { >>>> VhostShadowVirtqueue *svq = vhost_svq_new(); >>>>