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

[Eugenio Perez Martin]

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

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?

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