On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <[email protected]> wrote:
>
> On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> <[email protected]> wrote:
> >
> > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <[email protected]> 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 <[email protected]>
> > >
> > >
> > > 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.
>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, 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?
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);
> > >
> >
>
