Re: [PATCH 3/3] vdpa: Expose VIRTIO_NET_F_STATUS unconditionally

[Eugenio Perez Martin]

On Tue, Nov 1, 2022 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Oct 28, 2022 at 5:30 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Fri, Oct 28, 2022 at 3:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > > > > <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasowang@redhat.com> 
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > > 在 2022/10/26 17:53, Eugenio Pérez 写道:
> > > > > > > > Now that qemu can handle and emulate it if the vdpa backend 
> > > > > > > > does not
> > > > > > > > support it we can offer it always.
> > > > > > > >
> > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > >
> > > > > > >
> > > > > > > I may miss something but isn't more easier to simply remove the
> > > > > > > _F_STATUS from vdpa_feature_bits[]?
> > > > > > >
> > > > > >
> > > > > > How is that? if we remove it, the guest cannot ack it so it cannot
> > > > > > access the net status, isn't it?
> > > > >
> > > > > My understanding is that the bits stored in the vdpa_feature_bits[]
> > > > > are the features that must be explicitly supported by the vhost
> > > > > device.
> > > >
> > > > (Non English native here, so maybe I don't get what you mean :) ) The
> > > > device may not support them. net simulator lacks some of them
> > > > actually, and it works.
> > >
> > > Speaking too fast, I think I meant that, if the bit doesn't belong to
> > > vdpa_feature_bits[], it is assumed to be supported by the Qemu without
> > > the support of the vhost. So Qemu won't even try to validate if vhost
> > > has this support. E.g for vhost-net, we only have:
> > >
> > > static const int kernel_feature_bits[] = {
> > >     VIRTIO_F_NOTIFY_ON_EMPTY,
> > >     VIRTIO_RING_F_INDIRECT_DESC,
> > >     VIRTIO_RING_F_EVENT_IDX,
> > >     VIRTIO_NET_F_MRG_RXBUF,
> > >     VIRTIO_F_VERSION_1,
> > >     VIRTIO_NET_F_MTU,
> > >     VIRTIO_F_IOMMU_PLATFORM,
> > >     VIRTIO_F_RING_PACKED,
> > >     VIRTIO_NET_F_HASH_REPORT,
> > >     VHOST_INVALID_FEATURE_BIT
> > > };
> > >
> > > You can see there's no STATUS bit there since it is emulated by Qemu.
> > >
> >
> > Ok now I get what you mean, and yes we may modify the patches in that 
> > direction.
> >
> > But if we go then we need to modify how qemu ack the features, because
> > the features that are not in vdpa_feature_bits are not acked to the
> > device. More on this later.
> >
> > > >
> > > > From what I see these are the only features that will be forwarded to
> > > > the guest as device_features. If it is not in the list, the feature
> > > > will be masked out,
> > >
> > > Only when there's no support for this feature from the vhost.
> > >
> > > > as if the device does not support it.
> > > >
> > > > So now _F_STATUS it was forwarded only if the device supports it. If
> > > > we remove it from bit_mask, it will never be offered to the guest. But
> > > > we want to offer it always, since we will need it for
> > > > _F_GUEST_ANNOUNCE.
> > > >
> > > > Things get more complex because we actually need to ack it back if the
> > > > device offers it, so the vdpa device can report link_down. We will
> > > > only emulate LINK_UP always in the case the device does not support
> > > > _F_STATUS.
> > > >
> > > > > So if we remove _F_STATUS, Qemu vhost code won't validate if
> > > > > vhost-vdpa device has this support:
> > > > >
> > > > > uint64_t vhost_get_features(struct vhost_dev *hdev, const int 
> > > > > *feature_bits,
> > > > >                             uint64_t features)
> > > > > {
> > > > >     const int *bit = feature_bits;
> > > > >     while (*bit != VHOST_INVALID_FEATURE_BIT) {
> > > > >         uint64_t bit_mask = (1ULL << *bit);
> > > > >         if (!(hdev->features & bit_mask)) {
> > > > >             features &= ~bit_mask;
> > > > >         }
> > > > >         bit++;
> > > > >     }
> > > > >     return features;
> > > > > }
> > > > >
> > > >
> > > > Now maybe I'm the one missing something, but why is this not done as a
> > > > masking directly?
> > >
> > > Not sure, the code has been there since day 0.
> > >
> > > But you can see from the code:
> > >
> > > 1) if STATUS is in feature_bits, we need validate the hdev->features
> > > and mask it if the vhost doesn't have the support
> > > 2) if STATUS is not, we don't do the check and driver may still see STATUS
> > >
> >
> > That's useful for _F_GUEST_ANNOUNCE, but we need to ack _F_STATUS for
> > the device if it supports it.
>
> Rethink about this, I don't see why ANNOUNCE depends on STATUS (spec
> doesn't say so).
>

It is needed for the guest to read the status bit:
"""
status only exists if VIRTIO_NET_F_STATUS is set. Two read-only bits
(for the driver) are currently defined for the status field:
VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
"""

A change on the standard could be possible, like "status only exists
if VIRTIO_NET_F_STATUS or VIRTIO_NET_F_GUEST_ANNOUNCE is set".
However, Linux drivers already expect _F_STATUS to read _S_ANNOUNCE
and to emulate _F_STATUS in case the device doesn't support it should
not be a big deal in my opinion.

> > QEMU cannot detect by itself when the
> > link is not up. I think that setting unconditionally
> > VIRTIO_NET_S_LINK_UP is actually a regression, since the guest cannot
> > detect the link down that way.
>
> I think the idea is to still read status from config if the device
> supports this.
>

Yes, that's my point. If I delete it from vdpa_feature_bits, it will
not be acked to the device, so we cannot read status from the device.

> >
> > To enable _F_STATUS unconditionally is only done in the case the
> > device does not support it, because its emulation is very easy. That
> > way we support _F_GUEST_ANNOUNCE in all cases without device's
> > cooperation.
> >
> > Having said that, should we go the opposite route and ack _F_STATE as
> > long as the device supports it? As an advantage, all backends should
> > support that at this moment, isn't it?
>
> So I think the method used in this patch is fine, but I wonder if it's
> better to move it to the vhost layer instead of doing it in vhost_net
> since we do the features validation there. We probably need another
> table as input for get/set features there?
>

We can discuss how to do it for sure. But as you pointed out,
vhost_net and virtio_net already modify the features received from the
devices, so it makes sense to me to modify the features set by the
guest.

The problem is that we need to transmit to vhost when ack _F_STATUS
and when not. The first proposal was to add a new member of vhost_vdpa
but this is not optimal.

If we add a new table it should be a static const one, and vhost_vdpa
should have a pointer to it, isn't it? Something like features that
are emulated by qemu so they must be offered always to the guest?

Thanks!

> Thanks
>
> >
> > Thanks!
> >
> >
> >
> >
> > > Thanks
> > >
> > > >
> > > > Instead of making feature_bits an array of ints, to declare it as a
> > > > uint64_t with the valid feature bits and simply return features &
> > > > feature_bits.
> > > >
> > > > Thanks!
> > > >
> > > > > Thanks
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > The goal with this patch series is to let the guest access the 
> > > > > > status
> > > > > > always, even if the device doesn't support _F_STATUS.
> > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > >   include/net/vhost-vdpa.h |  1 +
> > > > > > > >   hw/net/vhost_net.c       | 16 ++++++++++++++--
> > > > > > > >   net/vhost-vdpa.c         |  3 +++
> > > > > > > >   3 files changed, 18 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
> > > > > > > > index b81f9a6f2a..cfbcce6427 100644
> > > > > > > > --- a/include/net/vhost-vdpa.h
> > > > > > > > +++ b/include/net/vhost-vdpa.h
> > > > > > > > @@ -17,5 +17,6 @@
> > > > > > > >   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState 
> > > > > > > > *nc);
> > > > > > > >
> > > > > > > >   extern const int vdpa_feature_bits[];
> > > > > > > > +extern const uint64_t vhost_vdpa_net_added_feature_bits;
> > > > > > > >
> > > > > > > >   #endif /* VHOST_VDPA_H */
> > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > > > > > index d28f8b974b..7c15cc6e8f 100644
> > > > > > > > --- a/hw/net/vhost_net.c
> > > > > > > > +++ b/hw/net/vhost_net.c
> > > > > > > > @@ -109,10 +109,22 @@ static const int 
> > > > > > > > *vhost_net_get_feature_bits(struct vhost_net *net)
> > > > > > > >       return feature_bits;
> > > > > > > >   }
> > > > > > > >
> > > > > > > > +static uint64_t vhost_net_add_feature_bits(struct vhost_net 
> > > > > > > > *net)
> > > > > > > > +{
> > > > > > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > > > > > > +        return vhost_vdpa_net_added_feature_bits;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >   uint64_t vhost_net_get_features(struct vhost_net *net, 
> > > > > > > > uint64_t features)
> > > > > > > >   {
> > > > > > > > -    return vhost_get_features(&net->dev, 
> > > > > > > > vhost_net_get_feature_bits(net),
> > > > > > > > -            features);
> > > > > > > > +    uint64_t ret = vhost_get_features(&net->dev,
> > > > > > > > +                                      
> > > > > > > > vhost_net_get_feature_bits(net),
> > > > > > > > +                                      features);
> > > > > > > > +
> > > > > > > > +    return ret | vhost_net_add_feature_bits(net);
> > > > > > > >   }
> > > > > > > >   int vhost_net_get_config(struct vhost_net *net,  uint8_t 
> > > > > > > > *config,
> > > > > > > >                            uint32_t config_len)
> > > > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > > > index 6d64000202..24d2857593 100644
> > > > > > > > --- a/net/vhost-vdpa.c
> > > > > > > > +++ b/net/vhost-vdpa.c
> > > > > > > > @@ -99,6 +99,9 @@ static const uint64_t 
> > > > > > > > vdpa_svq_device_features =
> > > > > > > >       BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > > > > > >       BIT_ULL(VIRTIO_NET_F_STANDBY);
> > > > > > > >
> > > > > > > > +const uint64_t vhost_vdpa_net_added_feature_bits =
> > > > > > > > +    BIT_ULL(VIRTIO_NET_F_STATUS);
> > > > > > > > +
> > > > > > > >   VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > > > > > >   {
> > > > > > > >       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>