On Sat, Jan 29, 2022 at 9:11 AM Jason Wang <jasow...@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 <epere...@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();
> >
>
