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

[fengchengwen <fengchengwen@huawei.com>]

Hi Alex,

On 7/9/2026 7:31 AM, Alex Williamson wrote:
> 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.

I will gate all TPH config space virtualization and masking logic behind
the VFIO_DEVICE_FEATURE_TPH SET opt-in check. Without explicit opt-in,
TPH_CTRL and ST table entries will remain read-only to avoid exposing
writable virtual bits with no functional backing.

> 
>> +
>> +	/* 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.

I will try to fix the length

> 
>> +	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(...)

OK

> 
>> +{
>> +	__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.

OK

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

OK, I will try that in v20

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

ok

> 
>> +		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);
> 
> ??

I will move vfio_find_cap_start in front of this new function.

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

OK

Thanks

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