Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.As the comment was related to VIRTIO_F_IN_ORDER, I thought of adding it in config operation related to virtio features handling. But I think since it is already added in the commit description, this code comment can be removed.
On Wed, Dec 7, 2022 at 10:56 PM Gautam Dawar <gautam.dawar@xxxxxxx> wrote:
vDPA config operations can be broadly categorized in to eitherI don't see why this comment is placed here?
virtqueue operations, device operations or DMA operations.
This patch implements most of the device level config operations.
SN1022 supports VIRTIO_F_IN_ORDER which is supported by the DPDK
virtio driver but not the kernel virtio driver. Due to a bug in
QEMU (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fqemu-project%2Fqemu%2F-%2Fissues%2F331%23&data=05%7C01%7Cgautam.dawar%40amd.com%7C58787eb502484eeaa6f508dadd9ea016%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638065970623127805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cVomPr2aFJBHW7kwzeZJrNHuU6oTYOTV14eOS%2BpeNVc%3D&reserved=0), with
vhost-vdpa, this feature bit is returned with guest kernel virtio
driver in set_features config operation. The fix for this bug
(qemu_commit c33f23a419f95da16ab4faaf08be635c89b96ff0) is available
in QEMU versions 6.1.0 and later. Hence, that's the oldest QEMU
version required for testing with the vhost-vdpa driver.
With older QEMU releases, VIRTIO_F_IN_ORDER is negotiated but
not honored causing Firmware exception.
Signed-off-by: Gautam Dawar <gautam.dawar@xxxxxxx>
---
drivers/net/ethernet/sfc/ef100_vdpa.h | 14 ++
drivers/net/ethernet/sfc/ef100_vdpa_ops.c | 148 ++++++++++++++++++++++
2 files changed, 162 insertions(+)
diff --git a/drivers/net/ethernet/sfc/ef100_vdpa.h b/drivers/net/ethernet/sfc/ef100_vdpa.h
index 83f6d819f6a5..be7650c3166a 100644
--- a/drivers/net/ethernet/sfc/ef100_vdpa.h
+++ b/drivers/net/ethernet/sfc/ef100_vdpa.h
@@ -21,6 +21,18 @@
/* Max queue pairs currently supported */
#define EF100_VDPA_MAX_QUEUES_PAIRS 1
+/* Device ID of a virtio net device */
+#define EF100_VDPA_VIRTIO_NET_DEVICE_ID VIRTIO_ID_NET
+
+/* Vendor ID of Xilinx vDPA NIC */
+#define EF100_VDPA_VENDOR_ID PCI_VENDOR_ID_XILINX
+
+/* Max number of Buffers supported in the virtqueue */
+#define EF100_VDPA_VQ_NUM_MAX_SIZE 512
+
+/* Alignment requirement of the Virtqueue */
+#define EF100_VDPA_VQ_ALIGN 4096
+
/**
* enum ef100_vdpa_nic_state - possible states for a vDPA NIC
*
@@ -61,6 +73,7 @@ enum ef100_vdpa_vq_type {
* @net_config: virtio_net_config data
* @mac_address: mac address of interface associated with this vdpa device
* @mac_configured: true after MAC address is configured
+ * @cfg_cb: callback for config change
*/
struct ef100_vdpa_nic {
struct vdpa_device vdpa_dev;
@@ -76,6 +89,7 @@ struct ef100_vdpa_nic {
struct virtio_net_config net_config;
u8 *mac_address;
bool mac_configured;
+ struct vdpa_callback cfg_cb;
};
int ef100_vdpa_init(struct efx_probe_data *probe_data);
diff --git a/drivers/net/ethernet/sfc/ef100_vdpa_ops.c b/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
index 31952931c198..87899baa1c52 100644
--- a/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
+++ b/drivers/net/ethernet/sfc/ef100_vdpa_ops.c
@@ -10,12 +10,148 @@
#include <linux/vdpa.h>
#include "ef100_vdpa.h"
+#include "mcdi_vdpa.h"
static struct ef100_vdpa_nic *get_vdpa_nic(struct vdpa_device *vdev)
{
return container_of(vdev, struct ef100_vdpa_nic, vdpa_dev);
}
+static u32 ef100_vdpa_get_vq_align(struct vdpa_device *vdev)
+{
+ return EF100_VDPA_VQ_ALIGN;
+}
+
+static u64 ef100_vdpa_get_device_features(struct vdpa_device *vdev)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+ u64 features;
+ int rc;
+
+ rc = efx_vdpa_get_features(vdpa_nic->efx,
+ EF100_VDPA_DEVICE_TYPE_NET, &features);
+ if (rc) {
+ dev_err(&vdev->dev, "%s: MCDI get features error:%d\n",
+ __func__, rc);
+ /* Returning 0 as value of features will lead to failure
+ * of feature negotiation.
+ */
+ return 0;
+ }
+
+ /* SN1022 supports VIRTIO_F_IN_ORDER which is supported by the DPDK
+ * virtio driver but not the kernel virtio driver. Due to a bug in
+ * QEMU (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.com%2Fqemu-project%2Fqemu%2F-%2Fissues%2F331%23&data=05%7C01%7Cgautam.dawar%40amd.com%7C58787eb502484eeaa6f508dadd9ea016%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638065970623127805%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=cVomPr2aFJBHW7kwzeZJrNHuU6oTYOTV14eOS%2BpeNVc%3D&reserved=0), with
+ * vhost-vdpa, this feature bit is returned with guest kernel virtio
+ * driver in set_features config operation. The fix for this bug
+ * (commit c33f23a419f95da16ab4faaf08be635c89b96ff0) is available
+ * in QEMU versions 6.1.0 and later. Hence, that's the oldest QEMU
+ * version required for testing with the vhost-vdpa driver.
+ */
You're right. I'll remove this check as state is set to EF100_VDPA_STATE_INITIALIZED soon after vdpa_alloc_device but before _vdpa_register_device()
+ features |= BIT_ULL(VIRTIO_NET_F_MAC);Under which case could we reach this condition? The
+
+ return features;
+}
+
+static int ef100_vdpa_set_driver_features(struct vdpa_device *vdev,
+ u64 features)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+ u64 verify_features;
+ int rc;
+
+ mutex_lock(&vdpa_nic->lock);
+ if (vdpa_nic->vdpa_state != EF100_VDPA_STATE_INITIALIZED) {
vdpa_device_register() should be called after switching the state to
EF100_VDPA_STATE_INITIALIZED.
+ dev_err(&vdev->dev, "%s: Invalid state %u\n",It looks to me this will use MC_CMD_VIRTIO_TEST_FEATURES command, I
+ __func__, vdpa_nic->vdpa_state);
+ rc = -EINVAL;
+ goto err;
+ }
+ verify_features = features & ~BIT_ULL(VIRTIO_NET_F_MAC);
+ rc = efx_vdpa_verify_features(vdpa_nic->efx,
+ EF100_VDPA_DEVICE_TYPE_NET,
+ verify_features);
wonder if it's better to use
MC_CMD_VIRTIO_SET_FEATURES to align with the virtio spec and maybe
change efx_vdpa_verify_features to efx_vdpa_set_features()?
Yes, it would be better to have this validation at one place (vdpa framework) instead of individual vendor drivers. Although I am not sure if the framework can choose the correct config size based on the device class.+I wonder if we need similar checks in the vdpa core.
+ if (rc) {
+ dev_err(&vdev->dev, "%s: MCDI verify features error:%d\n",
+ __func__, rc);
+ goto err;
+ }
+
+ vdpa_nic->features = features;
+err:
+ mutex_unlock(&vdpa_nic->lock);
+ return rc;
+}
+
+static u64 ef100_vdpa_get_driver_features(struct vdpa_device *vdev)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+
+ return vdpa_nic->features;
+}
+
+static void ef100_vdpa_set_config_cb(struct vdpa_device *vdev,
+ struct vdpa_callback *cb)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+
+ if (cb)
+ vdpa_nic->cfg_cb = *cb;
+}
+
+static u16 ef100_vdpa_get_vq_num_max(struct vdpa_device *vdev)
+{
+ return EF100_VDPA_VQ_NUM_MAX_SIZE;
+}
+
+static u32 ef100_vdpa_get_device_id(struct vdpa_device *vdev)
+{
+ return EF100_VDPA_VIRTIO_NET_DEVICE_ID;
+}
+
+static u32 ef100_vdpa_get_vendor_id(struct vdpa_device *vdev)
+{
+ return EF100_VDPA_VENDOR_ID;
+}
+
+static size_t ef100_vdpa_get_config_size(struct vdpa_device *vdev)
+{
+ return sizeof(struct virtio_net_config);
+}
+
+static void ef100_vdpa_get_config(struct vdpa_device *vdev,
+ unsigned int offset,
+ void *buf, unsigned int len)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+
+ /* Avoid the possibility of wrap-up after the sum exceeds U32_MAX */
+ if (WARN_ON(((u64)offset + len) > sizeof(struct virtio_net_config))) {
+ dev_err(&vdev->dev,
+ "%s: Offset + len exceeds config size\n", __func__);
+ return;
Yes, it's done in a later patch where filters support is added (sfc: implement filters for receiving traffic).
+ }Do we need to update hardware filters?
+ memcpy(buf, (u8 *)&vdpa_nic->net_config + offset, len);
+}
+
+static void ef100_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
+ const void *buf, unsigned int len)
+{
+ struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
+
+ /* Avoid the possibility of wrap-up after the sum exceeds U32_MAX */
+ if (WARN_ON(((u64)offset + len) > sizeof(vdpa_nic->net_config))) {
+ dev_err(&vdev->dev,
+ "%s: Offset + len exceeds config size\n", __func__);
+ return;
+ }
+
+ memcpy((u8 *)&vdpa_nic->net_config + offset, buf, len);
+ if (is_valid_ether_addr(vdpa_nic->mac_address))
+ vdpa_nic->mac_configured = true;
Thanks
+}
+
static void ef100_vdpa_free(struct vdpa_device *vdev)
{
struct ef100_vdpa_nic *vdpa_nic = get_vdpa_nic(vdev);
@@ -24,5 +160,17 @@ static void ef100_vdpa_free(struct vdpa_device *vdev)
}
const struct vdpa_config_ops ef100_vdpa_config_ops = {
+ .get_vq_align = ef100_vdpa_get_vq_align,
+ .get_device_features = ef100_vdpa_get_device_features,
+ .set_driver_features = ef100_vdpa_set_driver_features,
+ .get_driver_features = ef100_vdpa_get_driver_features,
+ .set_config_cb = ef100_vdpa_set_config_cb,
+ .get_vq_num_max = ef100_vdpa_get_vq_num_max,
+ .get_device_id = ef100_vdpa_get_device_id,
+ .get_vendor_id = ef100_vdpa_get_vendor_id,
+ .get_config_size = ef100_vdpa_get_config_size,
+ .get_config = ef100_vdpa_get_config,
+ .set_config = ef100_vdpa_set_config,
+ .get_generation = NULL,
.free = ef100_vdpa_free,
};
--
2.30.1