On Thu, Oct 27, 2022 at 6:18 PM Eugenio Perez Martin
<epere...@redhat.com> wrote:
>
> On Thu, Oct 27, 2022 at 8:54 AM Jason Wang <jasow...@redhat.com> wrote:
> >
> > On Thu, Oct 27, 2022 at 2:47 PM Eugenio Perez Martin
> > <epere...@redhat.com> wrote:
> > >
> > > On Thu, Oct 27, 2022 at 6:32 AM Jason Wang <jasow...@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 <epere...@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.
>
> 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
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);
> > > >
> > >
> >
>
