From: Peng Hao <[email protected]> Since commit 69b9461512246 ("virtio_pci_modern: allow configuring extended features") the modern virtio-pci driver unconditionally accesses the whole 128 bits features space, i.e. it drives device_feature_select / guest_feature_select with the values 0..3.
Devices predating the extended features space only implement the legacy 64 bits one, and what they report for the selectors above it is not a valid features space. Negotiating it makes the driver and the device end up with different features sets: on a smart NIC exposing a virtio_net device the link comes up but carries no traffic, while the same device works with a kernel that only accesses the low 64 bits. Reading the features space has no side effect, so keep reading all of it and use the extended part to tell whether the device implements it: report the legacy 64 bits only when the extended words read back as all-ones or as an alias of the low words, and latch the device down for good. As the features negotiation ANDs the device and driver features, no feature above bit 63 can be negotiated afterwards. Writing a selector the device does not implement cannot be relied upon the same way, so never drive one above the highest word that actually carries a bit. The reset preceding the features negotiation zeroes the device side features, hence the words left unwritten stay cleared. Also dump the raw device_feature dwords, and add a max_features_u64s module parameter to force the legacy 64 bits space on devices whose quirk the detection does not catch. Conforming devices are unaffected: their extended words are neither all-ones nor an alias of the low ones, so the detection does not trigger. Signed-off-by: Peng Hao <[email protected]> --- diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c index 413a8c353463..cfb10c9f32dd 100644 --- a/drivers/virtio/virtio_pci_modern_dev.c +++ b/drivers/virtio/virtio_pci_modern_dev.c @@ -5,6 +5,21 @@ #include <linux/pci.h> #include <linux/delay.h> +static int max_features_u64s = -1; +module_param(max_features_u64s, int, 0444); +MODULE_PARM_DESC(max_features_u64s, + "Max number of 64 bit words of the virtio features space to access (1 = legacy 64 bits only, -1 = auto-detect)"); + +static u8 vp_modern_features_u64s(const struct virtio_pci_modern_device *mdev) +{ + u8 u64s = mdev->features_u64s ?: VIRTIO_FEATURES_U64S; + + if (max_features_u64s > 0 && u64s > max_features_u64s) + u64s = max_features_u64s; + + return u64s; +} + /* * vp_modern_map_capability - map a part of virtio pci capability * @mdev: the modern virtio-pci device @@ -230,6 +245,8 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev) check_offsets(); + mdev->features_u64s = VIRTIO_FEATURES_U64S; + if (mdev->device_id_check) { devid = mdev->device_id_check(pci_dev); if (devid < 0) @@ -398,15 +415,36 @@ void vp_modern_get_extended_features(struct virtio_pci_modern_device *mdev, u64 *features) { struct virtio_pci_common_cfg __iomem *cfg = mdev->common; + u32 raw[VIRTIO_FEATURES_BITS / 32]; + u8 u64s = vp_modern_features_u64s(mdev); int i; - virtio_features_zero(features); for (i = 0; i < VIRTIO_FEATURES_BITS / 32; i++) { - u64 cur; - vp_iowrite32(i, &cfg->device_feature_select); - cur = vp_ioread32(&cfg->device_feature); - features[i >> 1] |= cur << (32 * (i & 1)); + raw[i] = vp_ioread32(&cfg->device_feature); + } + + dev_info(&mdev->pci_dev->dev, + "virtio_pci: device_feature[0..3] = 0x%08x 0x%08x 0x%08x 0x%08x\n", + raw[0], raw[1], raw[2], raw[3]); + + virtio_features_zero(features); + for (i = 0; i < u64s * 2; i++) + features[i >> 1] |= (u64)raw[i] << (32 * (i & 1)); + + for (i = 1; i < u64s; i++) { + int j; + + if (features[i] != U64_MAX && + !(features[0] && features[i] == features[0])) + continue; + + dev_info(&mdev->pci_dev->dev, + "virtio_pci: no extended features space, using 64 bits features only\n"); + mdev->features_u64s = 1; + for (j = 1; j < VIRTIO_FEATURES_U64S; j++) + features[j] = 0; + break; } } EXPORT_SYMBOL_GPL(vp_modern_get_extended_features); @@ -424,10 +462,11 @@ vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev, u64 *features) { struct virtio_pci_common_cfg __iomem *cfg = mdev->common; + u8 u64s = vp_modern_features_u64s(mdev); int i; virtio_features_zero(features); - for (i = 0; i < VIRTIO_FEATURES_BITS / 32; i++) { + for (i = 0; i < u64s * 2; i++) { u64 cur; vp_iowrite32(i, &cfg->guest_feature_select); @@ -446,9 +485,19 @@ void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev, const u64 *features) { struct virtio_pci_common_cfg __iomem *cfg = mdev->common; + u8 u64s = vp_modern_features_u64s(mdev); int i; - for (i = 0; i < VIRTIO_FEATURES_BITS / 32; i++) { + /* + * Never drive a selector the device may not implement: stop at the + * highest word that carries a bit. The device side features are + * zeroed by the reset that precedes the features negotiation, so the + * words left unwritten keep the value the driver wants for them. + */ + while (u64s > 1 && !features[u64s - 1]) + u64s--; + + for (i = 0; i < u64s * 2; i++) { u32 cur = features[i >> 1] >> (32 * (i & 1)); vp_iowrite32(i, &cfg->guest_feature_select); diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h index 9a3f2fc53bd6..7dc76afa671e 100644 --- a/include/linux/virtio_pci_modern.h +++ b/include/linux/virtio_pci_modern.h @@ -27,6 +27,9 @@ * Returns the found device id or ERRNO * @dma_mask: Optional mask instead of the traditional DMA_BIT_MASK(64), * for vendor devices with DMA space address limitations + * @features_u64s: Number of 64 bit words of the features space that can be + * accessed on this device; 1 for devices not implementing + * the extended (128 bits) features space */ struct virtio_pci_modern_device { struct pci_dev *pci_dev; @@ -49,6 +52,7 @@ struct virtio_pci_modern_device { int (*device_id_check)(struct pci_dev *pdev); u64 dma_mask; + u8 features_u64s; }; /* -- 2.43.0
