MIME-Version: 1.0
From:   Eugenio Perez Martin <eperezma@redhat.com>
Date:   Tue, 7 Mar 2023 15:41:15 +0100
Message-ID: <CAJaqyWdSRksBh0PX-6Hz6Qy4EZ48zLYh79+_hROQn-7oo_fsww@mail.gmail.com>
Subject: Emulated CVQ in vdpa-net framework
To:     Stefano Garzarella <sgarzare@redhat.com>,
        Shannon Nelson <snelson@pensando.io>,
        Gautam Dawar <gdawar@xilinx.com>,
        Laurent Vivier <lvivier@redhat.com>,
        Alvaro Karsz <alvaro.karsz@solid-run.com>,
        Longpeng <longpeng2@huawei.com>,
        virtualization <virtualization@lists.linux-foundation.org>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Cindy Lu <lulu@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Si-Wei Liu <si-wei.liu@oracle.com>,
        Liuxiangdong <liuxiangdong5@huawei.com>,
        Parav Pandit <parav@mellanox.com>,
        Eli Cohen <eli@mellanox.com>,
        Zhu Lingshan <lingshan.zhu@intel.com>,
        Harpreet Singh Anand <hanand@xilinx.com>,
        "Gonglei (Arei)" <arei.gonglei@huawei.com>,
        Jason Wang <jasowang@redhat.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        kvm list <kvm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi!

tl;dr the emulated CVQ is duplicated across some vdpa-net devices (For
example, betwen vdpa-sim-net and mlx5). This emulated CVQ is useful to
the rest of the parent drivers because it helps them to offload the
process of the virtio CVQ commands and convert them to device-specific
messages in the driver.

To merge these implementations in a separated object will reduce the
duplication of the code and the maintenance costs of the parent
drivers.
---

I'd distribute the work in two phases. The first one consists of
adding a specialized vringh module to make CVQ tasks. Let's call it
net_ctrl_vringh provisionally. I'll use the mlx5 vdpa driver as an
example, although there is no need to actually modify mlx5_vdpa in
this first stage.

This first step would simply merge vdpasim_handle_cvq and
mlx5_cvq_kick_handler to abstract the CVQ message handling. The parent
driver is still in charge of receiving the kick and forwarding it to
net_ctrl_vringh. We should be able to replace all vringh referenced
code for this new net_ctrl_vringh in the mlx5 driver

Since control messages actually need to handle each message
differently, the vendor driver must supply a struct of pointers in the
line of:
net_ctrl_vringh_ops = {
  .mac_table_set = ...
  .mac_addr_set = ...
  .vlan_add = ...
  .vlan_del = ...
  ...
}
---

So cmd handling and validation is shared between all parent drivers.
Let's say the net_ctrl_vringh callback to fetch a control message is
net_ctrl_vringh_handle_cvq(net_ctrl_vringh *v, ...). We can choose
between passing this struct as a parameter or to embed it in
net_ctrl_vringh. I prefer to use parameters.

This is already a big win as we offload vendors drivers to have to
worry about vringh nuances.

Second phase is to actually move it to the vdpa frontend. This step is
less defined at this moment, but it should be also doable with little
effort.

My main goal with this is to offload vendors from having to worry of
DMA and ASID nuances if they don't need to. These simple parent
drivers do not need to contain a single line about ASID, DMA, etc: All
of that is handled by vdpa frontend.

There are many possibilities to do it. One of them is to include some
property on vdpa_device like "emulated_virtqueues" or similar. Another
could be to create vdpa_net_device, which uses vdpa_device internally.
Although I think the second is simpler, it would be great if we can
abstract emulated virtqueues somehow.

For simplicity, I'm going to assume we create a new vdpa_net_device.
It will be in the line of:
struct vdpa_net_device {
  struct vdpa_device v;
  struct vringh ctrl_vq;
  const struct vdpa_config_ops *parent_ops;
  const struct
  ...
}
---

And it creates / register itself like a regular vdpa device, but
wrapping vendor's properties and callbacks to accommodate this new
CVQ. So we can do what Jason proposes in [1] vdpa-wise, not
vendor-wise. If the vdpa_device has an emulated CVQ, we can do:

int vhost_vdpa_net_set_vq_address(vdev, idx, areas...) {
  if (!is_ctrl_vq_idx(vdev, idx)) {
    return parent_ops->set_vq_address(vdev, idx, areas);
  }

  vdev->ctrl_vq.desc_addr = ...
  ...
}

struct vdpa_net_ops = {
  .set_vq_address = vhost_vdpa_net_set_vq_address
  ...
}

struct vdpa_net_device *vdpa_net_alloc_device(struct device *parent,
ngroups, nas, ...) {
  nas++; // emulated CVQ can always be in another AS
  ngroups++ // emulated CVQ will always be in an independent group
  ...
  vdpa_alloc_device(vdev, parent, vdpa_net_ops, ngroups, nas, ...
}
---

So the simple HW that does not care about DMA, ASID, etc does not need
to include a single line about these aspects. I think this can be done
incrementally too.

Thoughts?

Thanks!

[1] https://lore.kernel.org/virtualization/20230119061525.75068-6-jasowang@redhat.com/