On Fri, Sep 11, 2026 at 04:11:33PM +0800, Peng Hao wrote:
> 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.
That's a device bug then? The spec says:
\begin{description}
\item[\field{device_feature_select}]
The driver uses this to select which feature bits
\field{device_feature} shows.
Value 0x0 selects Feature Bits 0 to 31, 0x1 selects Feature Bits 32 to
63, etc.
that "etc" means any value is valid)
There's no "extended features space". It's a quick hack we did
in the virtio code to avoid changing all drivers, but the spec
treats all features uniformly.
So you have a device that ignores a feature selector? Or some specific
bits from it?
> 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.
So using all bits is illegal, and so is anything that will by luck
mirror low bits?
This is really VIRTIO_F_BAD_FEATURE mess replaying itself.
Really quite a hack :(
> 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.
This part is ok.
> 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,
dump?
> and add a max_features_u64s
> module parameter to force the legacy 64 bits space on devices whose
> quirk the detection does not catch.
This is even worse.
> 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]>
Can you simply fix the device please? Why not?
And please tell us in what way exactly are your devices broken?
I am not merging multiple hacks "just in case" because there's no
way to test them for me.
> ---
> 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
