Re: [PATCH v12 6/6] vfio/pci: Add PCIe TPH control register virtualization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The TPH configuration write handler silently drops valid byte-level writes, blocks ST Table config-space writes, and bypasses virtual configuration space updates.
- [High] TPH state is lost and becomes unrecoverable after a device reset due to missing state synchronization.
- [High] `vfio_pci_tph_config_write` performs MMIO accesses without holding `vdev->memory_lock`, racing with concurrent device resets.
--

commit e0337d4fe2416ad1997b3bae043a087facf49ef3
Author: Chengwen Feng <fengchengwen@huawei.com>

vfio/pci: Add PCIe TPH control register virtualization

Implement virtualized handling for PCIe TPH capability control register
writes. Validate and mediate user write requests to accept only valid TPH
mode configurations. Synchronize shadow steering tag table to hardware when
TPH gets enabled successfully.

> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a10ed733f0e3a..188845f81626d 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c

[ ... ]

> @@ -313,6 +317,39 @@ static int vfio_virt_config_read(struct vfio_pci_core_device *vdev, int pos,
>  	return count;
>  }
>  
> +static int vfio_pci_tph_config_write(struct vfio_pci_core_device *vdev, int pos,
> +				     int count, struct perm_bits *perm,
> +				     int offset, __le32 val)
> +{
> +	struct pci_dev *pdev = vdev->pdev;
> +	u32 data = le32_to_cpu(val);
> +	u8 mode, req_en;
> +	int i, ret;
> +
> +	if (!enable_unsafe_tph)
> +		return count;
> +
> +	if (offset != PCI_TPH_CTRL || count < 2)
> +		return count;

[Severity: High]
Does this silently drop valid 1-byte writes to the control register, or
writes to the upper bytes of the register (e.g. offset 0x09)? This might
break guests that perform byte-wise accesses.

Also, could this drop guest writes to the ST Table if it resides in the
capability space (offset >= 0x0C)?

Additionally, since this function returns without ever updating
vdev->vconfig via vfio_default_config_write() or similar, does this bypass
virtual configuration space updates entirely?

> +
> +	guard(mutex)(&vdev->tph_lock);
> +
> +	mode = FIELD_GET(PCI_TPH_CTRL_MODE_SEL_MASK, data);
> +	req_en = FIELD_GET(PCI_TPH_CTRL_REQ_EN_MASK, data);
> +	if (req_en) {
> +		ret = pcie_enable_tph(pdev, mode);

[Severity: High]
If pcie_enable_tph() succeeds, it updates the hardware register and sets
pdev->tph_enabled = 1. However, since pci_save_state() is not called here,
if a device reset occurs (e.g. via VFIO_DEVICE_RESET), the PCI core
restores the stale saved state where TPH is disabled, clearing the hardware
register.

Since pdev->tph_enabled remains 1 in software, wouldn't subsequent attempts
by the guest to re-enable TPH cause pcie_enable_tph() to immediately return
-EBUSY, permanently blocking the guest from using TPH after a reset?

> +		if (ret == 0 && vdev->tph_st_shadow) {
> +			for (i = 0; i < vdev->tph_st_entries; i++)
> +				pcie_tph_set_st_entry(pdev, i,
> +						      vdev->tph_st_shadow[i]);

[Severity: High]
If the hardware ST table resides in the MSI-X table, pcie_tph_set_st_entry()
may perform MMIO writes. Since this is executed holding only
vdev->tph_lock, could this race with a concurrent device reset (which
acquires down_write(&vdev->memory_lock) to block MMIO accesses and disables
memory decode)?

Without holding down_read(&vdev->memory_lock) here, could these MMIO writes
interleave with the physical reset or occur while memory decode is disabled,
causing Master Aborts?

> +		}
> +	} else {
> +		pcie_disable_tph(vdev->pdev);
> +	}
> +
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526040830.52854-1-fengchengwen@huawei.com?part=6