On Thu, Nov 17, 2022 at 8:43 AM Eugenio Perez Martin
<eperezma@xxxxxxxxxx> wrote:
>
> On Thu, Nov 17, 2022 at 7:52 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> >
> >
> > 在 2022/11/16 23:05, Eugenio Pérez 写道:
> > > Isolate control virtqueue in its own group, allowing to intercept control
> > > commands but letting dataplane run totally passthrough to the guest.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@xxxxxxxxxx>
> > > ---
> > > v7:
> > > * Never ask for number of address spaces, just react if isolation is not
> > > possible.
> > > * Return ASID ioctl errors instead of masking them as if the device has
> > > no asid.
> > > * Simplify net_init_vhost_vdpa logic
> > > * Add "if possible" suffix
> > >
> > > v6:
> > > * Disable control SVQ if the device does not support it because of
> > > features.
> > >
> > > v5:
> > > * Fixing the not adding cvq buffers when x-svq=on is specified.
> > > * Move vring state in vhost_vdpa_get_vring_group instead of using a
> > > parameter.
> > > * Rename VHOST_VDPA_NET_CVQ_PASSTHROUGH to VHOST_VDPA_NET_DATA_ASID
> > >
> > > v4:
> > > * Squash vhost_vdpa_cvq_group_is_independent.
> > > * Rebased on last CVQ start series, that allocated CVQ cmd bufs at load
> > > * Do not check for cvq index on vhost_vdpa_net_prepare, we only have one
> > > that callback registered in that NetClientInfo.
> > >
> > > v3:
> > > * Make asid related queries print a warning instead of returning an
> > > error and stop the start of qemu.
> > > ---
> > > hw/virtio/vhost-vdpa.c | 3 +-
> > > net/vhost-vdpa.c | 117 +++++++++++++++++++++++++++++++++++++++--
> > > 2 files changed, 114 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 852baf8b2c..a29a18a6a9 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -653,7 +653,8 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
> > > {
> > > uint64_t features;
> > > uint64_t f = 0x1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2 |
> > > - 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH;
> > > + 0x1ULL << VHOST_BACKEND_F_IOTLB_BATCH |
> > > + 0x1ULL << VHOST_BACKEND_F_IOTLB_ASID;
> > > int r;
> > >
> > > if (vhost_vdpa_call(dev, VHOST_GET_BACKEND_FEATURES, &features)) {
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index a9c864741a..dc13a49311 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -101,6 +101,8 @@ static const uint64_t vdpa_svq_device_features =
> > > BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
> > > BIT_ULL(VIRTIO_NET_F_STANDBY);
> > >
> > > +#define VHOST_VDPA_NET_CVQ_ASID 1
> > > +
> > > VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
> > > {
> > > VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > @@ -242,6 +244,40 @@ static NetClientInfo net_vhost_vdpa_info = {
> > > .check_peer_type = vhost_vdpa_check_peer_type,
> > > };
> > >
> > > +static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index)
> > > +{
> > > + struct vhost_vring_state state = {
> > > + .index = vq_index,
> > > + };
> > > + int r = ioctl(device_fd, VHOST_VDPA_GET_VRING_GROUP, &state);
> > > +
> > > + if (unlikely(r < 0)) {
> > > + error_report("Cannot get VQ %u group: %s", vq_index,
> > > + g_strerror(errno));
> > > + return r;
> > > + }
> > > +
> > > + return state.num;
> > > +}
> > > +
> > > +static int vhost_vdpa_set_address_space_id(struct vhost_vdpa *v,
> > > + unsigned vq_group,
> > > + unsigned asid_num)
> > > +{
> > > + struct vhost_vring_state asid = {
> > > + .index = vq_group,
> > > + .num = asid_num,
> > > + };
> > > + int r;
> > > +
> > > + r = ioctl(v->device_fd, VHOST_VDPA_SET_GROUP_ASID, &asid);
> > > + if (unlikely(r < 0)) {
> > > + error_report("Can't set vq group %u asid %u, errno=%d (%s)",
> > > + asid.index, asid.num, errno, g_strerror(errno));
> > > + }
> > > + return r;
> > > +}
> > > +
> > > static void vhost_vdpa_cvq_unmap_buf(struct vhost_vdpa *v, void *addr)
> > > {
> > > VhostIOVATree *tree = v->iova_tree;
> > > @@ -316,11 +352,69 @@ dma_map_err:
> > > static int vhost_vdpa_net_cvq_start(NetClientState *nc)
> > > {
> > > VhostVDPAState *s;
> > > - int r;
> > > + struct vhost_vdpa *v;
> > > + uint64_t backend_features;
> > > + int64_t cvq_group;
> > > + int cvq_index, r;
> > >
> > > assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > >
> > > s = DO_UPCAST(VhostVDPAState, nc, nc);
> > > + v = &s->vhost_vdpa;
> > > +
> > > + v->shadow_data = s->always_svq;
> > > + v->shadow_vqs_enabled = s->always_svq;
> > > + s->vhost_vdpa.address_space_id = VHOST_VDPA_GUEST_PA_ASID;
> > > +
> > > + if (s->always_svq) {
> > > + goto out;
> > > + }
> > > +
> > > + /* Backend features are not available in v->dev yet. */
> > > + r = ioctl(v->device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
> > > + if (unlikely(r < 0)) {
> > > + error_report("Cannot get vdpa backend_features: %s(%d)",
> > > + g_strerror(errno), errno);
> > > + return -1;
> > > + }
> > > + if (!(backend_features & VHOST_BACKEND_F_IOTLB_ASID) ||
> > > + !vhost_vdpa_net_valid_svq_features(v->dev->features, NULL)) {
> >
> >
> > I think there should be some logic to block migration in this case?
> >
>
> Since vhost_vdpa are not shadowed they don't expose _F_LOG, so
> migration is blocked that way.
>
> We can override it to make its message a little bit clearer.
>
> >
> > > + return 0;
> > > + }
> > > +
> > > + /**
> > > + * Check if all the virtqueues of the virtio device are in a different vq
> > > + * than the last vq. VQ group of last group passed in cvq_group.
> > > + */
> > > + cvq_index = v->dev->vq_index_end - 1;
> > > + cvq_group = vhost_vdpa_get_vring_group(v->device_fd, cvq_index);
> > > + if (unlikely(cvq_group < 0)) {
> > > + return cvq_group;x
> > > + }
> > > + for (int i = 0; i < cvq_index; ++i) {
> > > + int64_t group = vhost_vdpa_get_vring_group(v->device_fd, i);
> > > +
> > > + if (unlikely(group < 0)) {
> > > + return group;
> > > + }
> > > +
> > > + if (unlikely(group == cvq_group)) {
> > > + warn_report(
> > > + "CVQ %"PRId64" group is the same as VQ %d one (%"PRId64")",
> > > + cvq_group, i, group);
> > > + return 0;
> >
> >
> > Ditto.
> >
> >
> > > + }
> > > + }
> > > +
> > > + r = vhost_vdpa_set_address_space_id(v, cvq_group, VHOST_VDPA_NET_CVQ_ASID);
> > > + if (unlikely(r < 0)) {
> > > + return r;
> > > + } else {
> > > + v->shadow_vqs_enabled = true;
> > > + s->vhost_vdpa.address_space_id = VHOST_VDPA_NET_CVQ_ASID;
> > > + }
> > > +
> > > +out:
> > > if (!s->vhost_vdpa.shadow_vqs_enabled) {
> > > return 0;
> > > }
> > > @@ -652,6 +746,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > > g_autoptr(VhostIOVATree) iova_tree = NULL;
> > > NetClientState *nc;
> > > int queue_pairs, r, i = 0, has_cvq = 0;
> > > + bool svq_cvq;
> > >
> > > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> > > opts = &netdev->u.vhost_vdpa;
> > > @@ -693,12 +788,24 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
> > > return queue_pairs;
> > > }
> > >
> > > - if (opts->x_svq) {
> > > - struct vhost_vdpa_iova_range iova_range;
> > > + svq_cvq = opts->x_svq || has_cvq;
> > > + if (svq_cvq) {
> > > + Error *warn = NULL;
> > >
> > > - if (!vhost_vdpa_net_valid_svq_features(features, errp)) {
> > > - goto err_svq;
> > > + svq_cvq = vhost_vdpa_net_valid_svq_features(features,
> > > + opts->x_svq ? errp : &warn);
> > > + if (!svq_cvq) {
> > > + if (opts->x_svq) {
> > > + goto err_svq;
> > > + } else {
> > > + warn_reportf_err(warn, "Cannot shadow CVQ: ");
> >
> >
> > This seems suspicious, we reach here we we can't just use svq for cvq.
> >
>
> Right, but what is the suspicious part?
>
> >
> >
> > > + }
> > > }
> >
> >
> > The above logic is not easy to follow. I guess the root cause is the
> > variable name. It looks to me svq_cvq is better to be renamed as "svq"?
> >
>
> Yes, we can rename it. I'll send a newer version with the rename.
>
Another possibility is to get rid of the variable and allocate the
iova_tree unconditionally. We could send the log in
vhost_vdpa_net_cvq_start, or make that message the vhost migration
blocker.
Thanks!
> Thanks!
>
> > Thanks
> >
> >
> > > + }
> > > +
> > > + if (svq_cvq) {
> > > + /* Allocate a common iova tree if there is a possibility of SVQ */
> > > + struct vhost_vdpa_iova_range iova_range;
> > >
> > > vhost_vdpa_get_iova_range(vdpa_device_fd, &iova_range);
> > > iova_tree = vhost_iova_tree_new(iova_range.first, iova_range.last);
> >
