Re: [PATCH] virtio_pci_modern: fall back to 64 bits features for devices without an extended features space

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 11, 2026 at 04:11:33PM +0800, Peng Hao wrote:
> From: Peng Hao <flyingingpeng@tencent.com>
> 
> 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 <flyingingpeng@tencent.com>


Also can you dump the pci header of the device please?

> ---
> 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