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

[Jason Wang]

On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> 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.

This reminds me of something. For vhost, if Qemu can handle some
feature bits, we don't need to validate if vhost has such support.

E.g we don't have _F_SATAUS and _F_GUEST_ANNOUNCE in kernel_feature_bits.

I wonder if we can do something similar for vhost-vDPA? Then we don't
need to bother new fields.

Thanks

>
> 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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>