| Message ID | 20220121202733.404989-12-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA shadow virtqueue | expand |
在 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@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. > + 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. 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(); >
On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang@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@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. 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(); > > >
On Mon, Jan 31, 2022 at 4:49 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang@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@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? 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(); > > > > >
在 2022/2/1 下午6:57, Eugenio Perez Martin 写道: > On Mon, Jan 31, 2022 at 4:49 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: >> On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasowang@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@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(); >>>>
Hi, Eugenio.
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;
#define VIRTIO_TRANSPORT_F_START 28
#define VIRTIO_TRANSPORT_F_END 38
#define VIRTIO_F_NOTIFY_ON_EMPTY 24
This case (VIRTIO_F_NOTIFY_ON_EMPTY) may be useless.
Thanks.
Xiangdong Liu
On Sat, Feb 26, 2022 at 10:31 AM Liuxiangdong <liuxiangdong5@huawei.com> wrote: > > Hi, Eugenio. > > 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; > > > > #define VIRTIO_TRANSPORT_F_START 28 > #define VIRTIO_TRANSPORT_F_END 38 > > #define VIRTIO_F_NOTIFY_ON_EMPTY 24 > > This case (VIRTIO_F_NOTIFY_ON_EMPTY) may be useless. > Hi Xiangdong Liu, You're right, it's out of the range so it does not make any sense to check for it. I will delete it in the next version, thank you very much! > > Thanks. > Xiangdong Liu >
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 */ + if (*dev_features & BIT_ULL(b)) { + clear_bit(b, dev_features); + r = false; + } + break; + + case VIRTIO_F_VERSION_1: + /* 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();
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@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(+)