lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Dec 2022 15:23:25 +0530 From: Gautam Dawar <gdawar@....com> To: Jason Wang <jasowang@...hat.com>, Gautam Dawar <gautam.dawar@....com> Cc: linux-net-drivers@....com, netdev@...r.kernel.org, eperezma@...hat.com, tanuj.kamde@....com, Koushik.Dutta@....com, harpreet.anand@....com, Edward Cree <ecree.xilinx@...il.com>, Martin Habets <habetsm.xilinx@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH net-next 05/11] sfc: implement vdpa device config operations On 12/14/22 12:14, Jason Wang wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Wed, Dec 7, 2022 at 10:56 PM Gautam Dawar <gautam.dawar@....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://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@....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://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. >> + */ > I don't see why this comment is placed here? 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. > >> + 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. 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() > >> + 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()? Yes, it makes more sense to match the function names with the operations in virtio spec. However, MC_CMD_VIRTIO_TEST_FEATURES queries if the passed set of features is supported and it fails in case either driver requests an unsupported feature or misses a feature that device requires. Hence, it's more of a verification of feature set than applying/setting them. >> + >> + 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. 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. > >> + } >> + 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? Yes, it's done in a later patch where filters support is added (sfc: implement filters for receiving traffic). > > 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 >>
Powered by blists - more mailing lists