Re: [PATCH v19 11/18] vfio/pci: Virtualize PCIe TPH capability registers

[Alex Williamson <alex@shazbot.org>]

On Thu, 2 Jul 2026 20:42:17 +0800
Chengwen Feng <fengchengwen@huawei.com> wrote:

> Implement virtualization and policy masking for PCIe TPH extended
> capability:
> - Split TPH config space permissions: keep header read-only, mark
>   TPH_CTRL and ST-table entries virtually writable.
> - Adjust virtual TPH capability bits according to hardware capability
>   and tph_policy, hiding unsupported modes.
> - Silently discard all write operations.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/vfio/pci/vfio_pci_config.c | 72 ++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 5c6ab172df6c..06d7b2fbf866 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1086,6 +1086,73 @@ static int __init init_pci_ext_cap_pwr_perm(struct perm_bits *perm)
>  	return 0;
>  }
>  
> +/* Permissions for TPH extended capability */
> +static int __init init_pci_ext_cap_tph_perm(struct perm_bits *perm)
> +{
> +	int i;
> +
> +	if (alloc_perm_bits(perm, pci_ext_cap_length[PCI_EXT_CAP_ID_TPH]))
> +		return -ENOMEM;
> +
> +	p_setd(perm, 0, ALL_VIRT, NO_WRITE);
> +	p_setd(perm, PCI_TPH_CAP, ALL_VIRT, NO_WRITE);
> +
> +	p_setd(perm, PCI_TPH_CTRL, ALL_VIRT,
> +	       PCI_TPH_CTRL_MODE_SEL_MASK | PCI_TPH_CTRL_REQ_EN_MASK);

This makes the control bits virtualized, writable with no backing
support yet.

> +
> +	/* Per PCI specification: There is an upper limit of 64 entries
> +	 * when the ST table is located in the TPH Requester Extended
> +	 * Capability structure.
> +	 * And the pci_ext_cap_length[PCI_EXT_CAP_ID_TPH] is 0xFF, so the
> +	 * following operation is fine.
> +	 */

Wrong multi-line comment style.  Also 0xFF for cap length doesn't
really explain why this is fine.  You are however highlighting that the
above perm bits alloc of size 0xFF is not fine.

> +	for (i = 0; i < 64; i++)
> +		p_setw(perm, PCI_TPH_BASE_SIZEOF + i * sizeof(u16),
> +		       (u16)ALL_VIRT, (u16)ALL_WRITE);
> +
> +	return 0;
> +}
> +
> +static void vfio_tph_capability_adjust(struct vfio_pci_core_device *vdev,
> +				       int pos)

Precedent is vfio_update_tph_vconfig_bytes(...)

> +{
> +	__le32 *vptr = (__le32 *)&vdev->vconfig[pos + PCI_TPH_CAP];
> +	struct pci_dev *pdev = vdev->pdev;
> +	u32 val = le32_to_cpu(*vptr);
> +	bool need_adjust = false;

Just write it back unconditionally, not worth tracking.

However, any modification to the capability needs to be gated by the
device feature opt-in, so this might be better handled in a readfn
rather than managed through permission bits.

> +
> +	if (!pcie_tph_supported(pdev, true)) {
> +		/* Remove extend TPH if root-port don't support */
> +		val &= ~PCI_TPH_CAP_EXT_TPH;
> +		need_adjust = true;
> +	}
> +
> +	if (vdev->tph_policy == VFIO_PCI_TPH_POLICY_NO_ST) {
> +		/* Report only No-ST mode supported */
> +		val &= ~(PCI_TPH_CAP_ST_IV | PCI_TPH_CAP_ST_DS |
> +			 PCI_TPH_CAP_LOC_MASK | PCI_TPH_CAP_ST_MASK);
> +		need_adjust = true;
> +	} else if (vdev->tph_policy == VFIO_PCI_TPH_POLICY_IV_ST) {
> +		/* Report only No-ST and IV modes supported */
> +		val &= ~PCI_TPH_CAP_ST_DS;
> +		/* Remove ST location and size if dev don't support IV mode */

s/don't/doesn't/

> +		if (!(val & PCI_TPH_CAP_ST_IV))
> +			val &= ~(PCI_TPH_CAP_LOC_MASK | PCI_TPH_CAP_ST_MASK);
> +		need_adjust = true;
> +	}
> +
> +	if (need_adjust)
> +		*vptr = cpu_to_le32(val);
> +}
> +
> +static int vfio_find_cap_start(struct vfio_pci_core_device *vdev, int pos);

??

> +static int vfio_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
> +				 int count, struct perm_bits *perm,
> +				 int offset, __le32 val)
> +{
> +	return count;
> +}
> +
>  /*
>   * Initialize the shared permission tables
>   */
> @@ -1101,6 +1168,7 @@ void vfio_pci_uninit_perm_bits(void)
>  
>  	free_perm_bits(&ecap_perms[PCI_EXT_CAP_ID_ERR]);
>  	free_perm_bits(&ecap_perms[PCI_EXT_CAP_ID_PWR]);
> +	free_perm_bits(&ecap_perms[PCI_EXT_CAP_ID_TPH]);
>  }
>  
>  int __init vfio_pci_init_perm_bits(void)
> @@ -1121,6 +1189,8 @@ int __init vfio_pci_init_perm_bits(void)
>  	/* Extended capabilities */
>  	ret |= init_pci_ext_cap_err_perm(&ecap_perms[PCI_EXT_CAP_ID_ERR]);
>  	ret |= init_pci_ext_cap_pwr_perm(&ecap_perms[PCI_EXT_CAP_ID_PWR]);
> +	ret |= init_pci_ext_cap_tph_perm(&ecap_perms[PCI_EXT_CAP_ID_TPH]);
> +	ecap_perms[PCI_EXT_CAP_ID_TPH].writefn = vfio_tph_config_write;
>  	ecap_perms[PCI_EXT_CAP_ID_VNDR].writefn = vfio_raw_config_write;
>  	ecap_perms[PCI_EXT_CAP_ID_DVSEC].writefn = vfio_raw_config_write;
>  
> @@ -1704,6 +1774,8 @@ static int vfio_ecap_init(struct vfio_pci_core_device *vdev)
>  		ret = vfio_fill_vconfig_bytes(vdev, epos, len);
>  		if (ret)
>  			return ret;
> +		if (ecap == PCI_EXT_CAP_ID_TPH && !hidden)

Never hidden.  Thanks,

Alex

> +			vfio_tph_capability_adjust(vdev, epos);
>  
>  		/*
>  		 * If we're just using this capability to anchor the list,