On Tue, Oct 25, 2022 at 4:45 AM Jason Wang <jasow...@redhat.com> wrote:
>
> On Mon, Oct 24, 2022 at 5:26 PM Eugenio Perez Martin
> <epere...@redhat.com> wrote:
> >
> > 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.
>
> 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.
>
That is valid for _F_GUEST_ANNOUNCE, because all of it is emulated and
we must forbid the ack of it for simplicity.
But _F_STATUS has two parts, one bit that must be exposed to the guest
from the physical NIC, since qemu cannot know when to change it
(VIRTIO_NET_S_LINK_UP) and another one that must be 100% emulated
(VIRTIO_NET_S_ANNOUNCE). VIRTIO_NET_F_STATUS is 100% emulated only if
it is not offered by the device.
I'm moving to vhost_net for the next series, and I'll try to make all
of this more explicit.
Thanks!
