Messages in this thread | | | From | Jason Wang <> | Date | Wed, 14 Dec 2022 14:44:03 +0800 | Subject | Re: [PATCH net-next 05/11] sfc: implement vdpa device config operations |
| |
On Wed, Dec 7, 2022 at 10:56 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > vDPA config operations can be broadly categorized in to either > 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://gitlab.com/qemu-project/qemu/-/issues/331#), 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@amd.com> > --- > 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://gitlab.com/qemu-project/qemu/-/issues/331#), 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. > + */
I don't see why this comment is placed here?
> + features |= BIT_ULL(VIRTIO_NET_F_MAC); > + > + 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) {
Under which case could we reach this condition? The vdpa_device_register() should be called after switching the state to EF100_VDPA_STATE_INITIALIZED.
> + dev_err(&vdev->dev, "%s: Invalid state %u\n", > + __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);
It looks to me this will use MC_CMD_VIRTIO_TEST_FEATURES command, I 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()?
> + > + 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;
I wonder if we need similar checks in the vdpa core.
> + } > + 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;
Do we need to update hardware filters?
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 >
| |