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

[Jason Wang]

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

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

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

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