Re: [RFC PATCH v2 2/8] vdpa: Save emulated features list in vhost_vdpa

[Eugenio Perez Martin <eperezma@redhat.com>]

On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > At this moment only _F_LOG is added there.
> > > > > >
> > > > > > However future patches add features that depend on the kind of device.
> > > > > > In particular, only net devices can add VIRTIO_F_GUEST_ANNOUNCE. So
> > > > > > let's allow vhost_vdpa creator to set custom emulated device features.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >  include/hw/virtio/vhost-vdpa.h | 2 ++
> > > > > >  hw/virtio/vhost-vdpa.c         | 8 ++++----
> > > > > >  net/vhost-vdpa.c               | 4 ++++
> > > > > >  3 files changed, 10 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h
> > > > > > index 1111d85643..50083e1e3b 100644
> > > > > > --- a/include/hw/virtio/vhost-vdpa.h
> > > > > > +++ b/include/hw/virtio/vhost-vdpa.h
> > > > > > @@ -31,6 +31,8 @@ typedef struct vhost_vdpa {
> > > > > >      bool iotlb_batch_begin_sent;
> > > > > >      MemoryListener listener;
> > > > > >      struct vhost_vdpa_iova_range iova_range;
> > > > > > +    /* VirtIO device features that can be emulated by qemu */
> > > > > > +    uint64_t added_features;
> > > > >
> > > > > Any reason we need a per vhost_vdpa storage for this? Or is there a
> > > > > chance that this field could be different among the devices?
> > > > >
> > > >
> > > > Yes, one device could support SVQ and the other one could not support
> > > > it because of different feature sets for example.
> > >
> > > Right, but for those devices that don't support SVQ, we don't even
> > > need mediation for feature like F_LOG and _F_STATUS?
> > >
> >
> > No, and we cannot offer it to the guest either.
>
> Just to make sure we are on the same page, what I meant is, consider
> in the future SVQ get the support of all features, so we can remove
> this field? This is because _F_STATUS can be mediated unconditionally
> anyhow.
>

For _F_STATUS that is right. But we cannot handle full
_F_GUEST_ANNOUNCE since control SVQ (will) needs features from the
device that cannot be emulated, like ASID.

I think your point is "Since qemu cannot migrate these devices it will
never set VIRTIO_NET_S_ANNOUNCE, so the guest will never send
VIRTIO_NET_CTRL_ANNOUNCE messages". And I think that is totally right,
but I still feel it is weird to expose it if we cannot handle it.

Maybe a good first step is to move added_features to vhost_net, or
maybe to convert it to "bool guest_announce_emulated" or something
similar?  This way hw/virtio/vhost-vdpa is totally unaware of this and
changes are more self contained.

Thanks!



> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks
> > > > >
> > > > > >      uint64_t acked_features;
> > > > > >      bool shadow_vqs_enabled;
> > > > > >      /* IOVA mapping used by the Shadow Virtqueue */
> > > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > > index 7468e44b87..ddb5e29288 100644
> > > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > > @@ -660,8 +660,8 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
> > > > > >
> > > > > >          v->acked_features = features;
> > > > > >
> > > > > > -        /* We must not ack _F_LOG if SVQ is enabled */
> > > > > > -        features &= ~BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +        /* Do not ack features emulated by qemu */
> > > > > > +        features &= ~v->added_features;
> > > > > >      }
> > > > > >
> > > > > >      trace_vhost_vdpa_set_features(dev, features);
> > > > > > @@ -1244,8 +1244,8 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
> > > > > >      int ret = vhost_vdpa_get_dev_features(dev, features);
> > > > > >
> > > > > >      if (ret == 0 && v->shadow_vqs_enabled) {
> > > > > > -        /* Add SVQ logging capabilities */
> > > > > > -        *features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +        /* Add emulated capabilities */
> > > > > > +        *features |= v->added_features;
> > > > > >      }
> > > > > >
> > > > > >      return ret;
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index eebf29f5c1..3803452800 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -599,6 +599,10 @@ static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
> > > > > >      s->vhost_vdpa.index = queue_pair_index;
> > > > > >      s->vhost_vdpa.shadow_vqs_enabled = svq;
> > > > > >      s->vhost_vdpa.iova_tree = iova_tree;
> > > > > > +    if (svq) {
> > > > > > +        /* Add SVQ logging capabilities */
> > > > > > +        s->vhost_vdpa.added_features |= BIT_ULL(VHOST_F_LOG_ALL);
> > > > > > +    }
> > > > > >      if (!is_datapath) {
> > > > > >          s->cvq_cmd_out_buffer = qemu_memalign(qemu_real_host_page_size(),
> > > > > >                                              vhost_vdpa_net_cvq_cmd_page_len());
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > > >
> > >
> >
>