On Mon, Oct 24, 2022 at 4:14 AM Jason Wang <jasow...@redhat.com> wrote: > > On Fri, Oct 21, 2022 at 4:56 PM Eugenio Perez Martin > <epere...@redhat.com> wrote: > > > > On Fri, Oct 21, 2022 at 4:57 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > On Thu, Oct 20, 2022 at 2:34 PM Eugenio Perez Martin > > > <epere...@redhat.com> wrote: > > > > > > > > On Thu, Oct 20, 2022 at 6:23 AM Jason Wang <jasow...@redhat.com> wrote: > > > > > > > > > > On Wed, Oct 19, 2022 at 8:52 PM Eugenio Pérez <epere...@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 <epere...@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 > > > > > > > > > > > > > > > > > > > > >