RE: [PATCH v5 2/4] bus/pci: introduce the PCIe TLP Processing Hints API

[Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com>]

> > rte_pci_tph_st_{get, set} functions will return an error if processing
> > any of the rte_tph_info objects fails. The API does not indicate which
> > entry in the rte_tph_info array was executed successfully and which
> > caused an error. Therefore, in case of an error, the caller should
> > discard the output. If rte_pci_tph_set returns an error, it should be
> > treated as a partial error. Hence, the steering-tag update on the
> > device should be considered partial and inconsistent with the expected
> outcome.
> > This should be resolved by resetting the endpoint device before
> > further attempts to set steering tags.
> 
> This seems very clunky for the user. Is there a fundamental reason why we cannot
> report out what ones passed or failed?
> 
> If it's a limitation of the kernel IOCTL, how about just making one ioctl for each
> individual op requested, one at a time. That way we will know what failed to
> report it?
> 

The V1 of the kernel patch had that feature, but it was frowned upon, and I was
asked to implement the IOCTL this way. Please find it here (V1)
https://lore.kernel.org/kvm/20250221224638.1836909-1-wathsala.vithanage@arm.com/T/#me73cf9b9c87da97d7d9461dfb97863b78ca1755b

> Other comments inline below.
> 

I will address them in the next version.

Thanks.

--wathsala

> /Bruce
> 
> >
> > Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com>
> > ---
> >  drivers/bus/pci/bsd/pci.c        |  43 ++++++++
> >  drivers/bus/pci/bus_pci_driver.h |  52 ++++++++++
> >  drivers/bus/pci/linux/pci.c      | 100 ++++++++++++++++++
> >  drivers/bus/pci/linux/pci_init.h |  13 +++
> > drivers/bus/pci/linux/pci_vfio.c | 170 +++++++++++++++++++++++++++++++
> >  drivers/bus/pci/private.h        |   8 ++
> >  drivers/bus/pci/rte_bus_pci.h    |  67 ++++++++++++
> >  drivers/bus/pci/windows/pci.c    |  43 ++++++++
> >  lib/pci/rte_pci.h                |  15 +++
> >  9 files changed, 511 insertions(+)
> >
> > diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
> > index 5e2e09d5a4..dff750c4d6 100644
> > --- a/drivers/bus/pci/bsd/pci.c
> > +++ b/drivers/bus/pci/bsd/pci.c
> 
> <snip>
> 
> > diff --git a/drivers/bus/pci/bus_pci_driver.h
> > b/drivers/bus/pci/bus_pci_driver.h
> > index 2cc1119072..b1c2829fc1 100644
> > --- a/drivers/bus/pci/bus_pci_driver.h
> > +++ b/drivers/bus/pci/bus_pci_driver.h
> > @@ -46,6 +46,7 @@ struct rte_pci_device {
> >  	char *bus_info;                     /**< PCI bus specific info */
> >  	struct rte_intr_handle *vfio_req_intr_handle;
> >  				/**< Handler of VFIO request interrupt */
> > +	uint8_t tph_enabled;                /**< TPH enabled on this device */
> 
> question: what would happen if we always enabled tph for each device. Does
> doing so disable the default handling for the device?
> 
> >  };
> >
> >  /**
> > @@ -194,6 +195,57 @@ struct rte_pci_ioport {
> >  	uint64_t len; /* only filled for memory mapped ports */  };
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change, or be removed, without
> > +prior
> > + * notice
> > + *
> > + * This structure is passed into the TPH Steering-Tag set or get
> > +function as an
> > + * argument by the caller. Return values are set in the same
> > +structure in st and
> > + * ph_ignore fields by the calee.
> > + *
> > + * Refer to PCI-SIG ECN "Revised _DSM for Cache Locality TPH
> > +Features" for
> > + * details.
> > + */
> > +struct rte_tph_info {
> > +	/* Input */
> > +	uint32_t cpu_id;	/*Logical CPU id*/
> > +	uint32_t cache_level;	/*Cache level relative to CPU. l1d=0,l2d=1,...*/
> > +	uint8_t flags;		/*Memory type, procesisng hint etc.*/
> > +	uint16_t index;		/*Index in vector table to store the ST*/
> > +
> 
> These fields should be reordered in order of size to avoid unnecessary gaps.
> 
> For the flags field, I dislike having different sets of flags all multiplexed into a
> single field. Can we instead of the flags field, and the set of #defines below split
> these out into separate enums, and then have separate fields for each one.
> 
> For example:
> 	struct rte_tph_info {
> 		uint32_t cpu_id;
> 		uint32_t cache_level;
> 		enum rte_tph_mem_type mem_type;
> 		enum rte_tph_hint hints;
> 		enum rte_tph_mode mode;
> 		...
> 	}
> 
> While the structure takes more space this way, this is not a datapath structure
> that we should be seeing large arrays of it, or that needs to be processed quickly,
> so usability should be prioritized over size/compactness.
> 
> 

+1

> > +	/* Output */
> > +	uint16_t st;		/*Steering tag returned by the platform*/
> > +	uint8_t ph_ignore;	/*Platform ignores PH for the returned ST*/
> > +};
> > +
> > +#define RTE_PCI_TPH_MEM_TYPE_MASK		0x1
> > +#define RTE_PCI_TPH_MEM_TYPE_SHIFT		0
> > +/** Request volatile memory ST */
> > +#define RTE_PCI_TPH_MEM_TYPE_VMEM		0
> > +/** Request persistent memory ST */
> > +#define RTE_PCI_TPH_MEM_TYPE_PMEM		1
> > +
> > +/** TLP Processing Hints - PCIe 6.0 specification section 2.2.7.1.1 */
> > +#define RTE_PCI_TPH_HINT_MASK		0x3
> 
> Looking at the mask usage below, does this mask not need to also be shifted by
> the TPH_HINT_SHIFT? Otherwise it overlaps with the type mask.
> 
> > +#define RTE_PCI_TPH_HINT_SHIFT		1
> > +/** Host and device access data equally */
> > +#define RTE_PCI_TPH_HINT_BIDIR		0
> > +/** Device accesses data more frequently */
> > +#define RTE_PCI_TPH_HINT_REQSTR		(1 <<
> RTE_PCI_TPH_HINT_SHIFT)
> > +/** Host access data more frequently */
> > +#define RTE_PCI_TPH_HINT_TARGET		(2 <<
> RTE_PCI_TPH_HINT_SHIFT)
> > +/** Host access data more frequently with a high temporal locality */
> > +#define RTE_PCI_TPH_HINT_TARGET_PRIO	(3 <<
> RTE_PCI_TPH_HINT_SHIFT)
> > +
> > +#define RTE_PCI_TPH_ST_MODE_MASK   0x3
> > +/** TPH no ST mode */
> > +#define RTE_PCI_TPH_ST_NS_MODE	   0
> > +/** TPH interrupt vector mode */
> > +#define RTE_PCI_TPH_ST_IV_MODE	   1
> > +/** TPH device specific mode */
> > +#define RTE_PCI_TPH_ST_DS_MODE	   2
> > +
> 
> As above, I think these would be nicer defined in different enums, going to
> separate fields in the struct. That would also remove any ambiguity as to whether
> the masks include the shift or not.
> 

+1

> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c
> > index c20d159218..b5a8ba0a86 100644
> > --- a/drivers/bus/pci/linux/pci.c
> > +++ b/drivers/bus/pci/linux/pci.c
> > @@ -814,3 +814,103 @@ rte_pci_ioport_unmap(struct rte_pci_ioport *p)
> >
> >  	return ret;
> >  }
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_enable, 25.07) int
> > +rte_pci_tph_enable(struct rte_pci_device *dev, int mode) {
> > +	int ret = 0;
> > +
> 
> Should check here if dev->tph_enabled is already true.
> 

+1

> > +	switch (dev->kdrv) {
> > +#ifdef VFIO_PRESENT
> > +	case RTE_PCI_KDRV_VFIO:
> > +		if (pci_vfio_is_enabled())
> > +			ret = pci_vfio_tph_enable(dev, mode);
> > +		break;
> > +#endif
> > +	case RTE_PCI_KDRV_IGB_UIO:
> > +	case RTE_PCI_KDRV_UIO_GENERIC:
> > +	default:
> > +		ret = -ENOTSUP;
> > +		break;
> > +	}
> > +
> > +	if (!ret)
> 
> Prefer "ret == 0" for this comparison.
> 
> > +		dev->tph_enabled = 1;
> > +
> > +	return ret;
> > +}
> > +
> 
> Function could probably be shortened to something like (including a check for
> already enabled, 2 lines shorter if we rely on checks in the
> vfio_tph_enable() call):
> 
> int
> rte_pci_tph_enable(...)
> {
> #ifdef VFIO_PRESENT
> 	if (dev->kdrv == RTE_PCI_KDRV_VFIO && pci_vfio_is_enabled()) {
> 		if (dev->tph_enabled == 0) {
> 			int ret = pci_vfio_tph_enable(...);
> 			if (ret != 0)
> 				return ret;
> 			dev->tph_enabled = 1;
> 		}
> 		return 0;
> 	}
> #endif
> 	return -ENOTSUP
> }
> 
> 
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_disable, 25.07) int
> > +rte_pci_tph_disable(struct rte_pci_device *dev) {
> > +	int ret = 0;
> > +
> > +	switch (dev->kdrv) {
> > +#ifdef VFIO_PRESENT
> > +	case RTE_PCI_KDRV_VFIO:
> > +		if (pci_vfio_is_enabled())
> > +			ret = pci_vfio_tph_disable(dev);
> > +		break;
> > +#endif
> > +	case RTE_PCI_KDRV_IGB_UIO:
> > +	case RTE_PCI_KDRV_UIO_GENERIC:
> > +	default:
> > +		ret = -ENOTSUP;
> > +		break;
> > +	}
> > +
> > +	if (!ret)
> > +		dev->tph_enabled = 0;
> > +
> > +	return ret;
> > +}
> 
> As above, we can shorten this function by replacing the switch with a straight
> check for kdrv == RTE_PCI_KDRV_VFIO. Same with functions below too.
> 
 
+1

> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_st_get, 25.07) int
> > +rte_pci_tph_st_get(const struct rte_pci_device *dev,
> > +		   struct rte_tph_info *info, size_t count) {
> > +	int ret = 0;
> > +
> > +	switch (dev->kdrv) {
> > +#ifdef VFIO_PRESENT
> > +	case RTE_PCI_KDRV_VFIO:
> > +		if (pci_vfio_is_enabled())
> > +			ret = pci_vfio_tph_st_get(dev, info, count);
> > +		break;
> > +#endif
> > +	case RTE_PCI_KDRV_IGB_UIO:
> > +	case RTE_PCI_KDRV_UIO_GENERIC:
> > +	default:
> > +		ret = -ENOTSUP;
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_st_set, 25.07) int
> > +rte_pci_tph_st_set(const struct rte_pci_device *dev,
> > +		   struct rte_tph_info *info, size_t count) {
> > +	int ret = 0;
> > +
> > +	switch (dev->kdrv) {
> > +#ifdef VFIO_PRESENT
> > +	case RTE_PCI_KDRV_VFIO:
> > +		if (pci_vfio_is_enabled())
> > +			ret = pci_vfio_tph_st_set(dev, info, count);
> > +		break;
> > +#endif
> > +	case RTE_PCI_KDRV_IGB_UIO:
> > +	case RTE_PCI_KDRV_UIO_GENERIC:
> > +	default:
> > +		ret = -ENOTSUP;
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > diff --git a/drivers/bus/pci/linux/pci_init.h
> > b/drivers/bus/pci/linux/pci_init.h
> > index 25b901f460..e71bfd2dce 100644
> > --- a/drivers/bus/pci/linux/pci_init.h
> > +++ b/drivers/bus/pci/linux/pci_init.h
> > @@ -5,6 +5,7 @@
> >  #ifndef EAL_PCI_INIT_H_
> >  #define EAL_PCI_INIT_H_
> >
> > +#include <rte_compat.h>
> >  #include <rte_vfio.h>
> >  #include <uapi/linux/vfio_tph.h>
> >
> > @@ -76,6 +77,18 @@ int pci_vfio_ioport_unmap(struct rte_pci_ioport
> > *p);  int pci_vfio_map_resource(struct rte_pci_device *dev);  int
> > pci_vfio_unmap_resource(struct rte_pci_device *dev);
> >
> > +/* TLP Processing Hints control functions */ __rte_experimental int
> > +pci_vfio_tph_enable(const struct rte_pci_device *dev, int mode);
> > +__rte_experimental int pci_vfio_tph_disable(const struct
> > +rte_pci_device *dev); __rte_experimental int
> > +pci_vfio_tph_st_get(const struct rte_pci_device *dev,
> > +			struct rte_tph_info *info, size_t ent_count);
> __rte_experimental
> > +int pci_vfio_tph_st_set(const struct rte_pci_device *dev,
> > +			struct rte_tph_info *info, size_t ent_count);
> > +
> >  int pci_vfio_is_enabled(void);
> >
> >  #endif
> > diff --git a/drivers/bus/pci/linux/pci_vfio.c
> > b/drivers/bus/pci/linux/pci_vfio.c
> > index 5317170231..bdbeb38658 100644
> > --- a/drivers/bus/pci/linux/pci_vfio.c
> > +++ b/drivers/bus/pci/linux/pci_vfio.c
> > @@ -12,6 +12,7 @@
> >  #include <stdbool.h>
> >
> >  #include <rte_log.h>
> > +#include <eal_export.h>
> >  #include <rte_pci.h>
> >  #include <rte_bus_pci.h>
> >  #include <rte_eal_paging.h>
> > @@ -1316,6 +1317,175 @@ pci_vfio_mmio_write(const struct rte_pci_device
> *dev, int bar,
> >  	return pwrite(fd, buf, len, offset + offs);  }
> >
> > +static int
> > +pci_vfio_tph_ioctl(const struct rte_pci_device *dev, struct
> > +vfio_pci_tph *pci_tph) {
> > +	const struct rte_intr_handle *intr_handle = dev->intr_handle;
> > +	int vfio_dev_fd = 0, ret = 0;
> > +
> > +	vfio_dev_fd = rte_intr_dev_fd_get(intr_handle);
> > +	if (vfio_dev_fd < 0) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	ret = ioctl(vfio_dev_fd, VFIO_DEVICE_PCI_TPH, pci_tph);
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int
> > +pci_vfio_tph_st_op(const struct rte_pci_device *dev,
> > +		    struct rte_tph_info *info, size_t count,
> > +		    enum rte_pci_st_op op)
> > +{
> > +	int ret = 0;
> > +	size_t argsz = 0, i;
> > +	struct vfio_pci_tph *pci_tph = NULL;
> > +	uint8_t mem_type = 0, hint = 0;
> > +
> > +	if (!count) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	argsz = sizeof(struct vfio_pci_tph) +
> > +		count * sizeof(struct vfio_pci_tph_entry);
> > +
> > +	pci_tph = rte_zmalloc(NULL, argsz, 0);
> 
> For ioctl we should not need pinned memory. Use regular malloc here.
> 

+1

> > +	if (!pci_tph) {
> 
> Coding style guidelines say to compare pointers explicitly to NULL.
> 

+1

> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	pci_tph->argsz = argsz;
> > +	pci_tph->count = count;
> > +
> > +	switch (op) {
> > +	case RTE_PCI_TPH_ST_GET:
> > +		pci_tph->flags = VFIO_DEVICE_TPH_GET_ST;
> > +		break;
> > +	case RTE_PCI_TPH_ST_SET:
> > +		pci_tph->flags = VFIO_DEVICE_TPH_SET_ST;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	for (i = 0; i < count; i++) {
> > +		pci_tph->ents[i].cpu_id = info[i].cpu_id;
> > +		pci_tph->ents[i].cache_level = info[i].cache_level;
> > +
> > +		mem_type = info[i].flags & RTE_PCI_TPH_MEM_TYPE_MASK;
> > +		switch (mem_type) {
> > +		case RTE_PCI_TPH_MEM_TYPE_VMEM:
> > +			pci_tph->ents[i].flags |= VFIO_TPH_MEM_TYPE_VMEM;
> > +			break;
> > +		case RTE_PCI_TPH_MEM_TYPE_PMEM:
> > +			pci_tph->ents[i].flags |= VFIO_TPH_MEM_TYPE_PMEM;
> > +			break;
> > +		default:
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		hint = info[i].flags & RTE_PCI_TPH_HINT_MASK;
> 
> As pointed out above, unshifted, this HINT_MASK overlaps with the TYPE_MASK.
> 
> > +		switch (hint) {
> > +		case RTE_PCI_TPH_HINT_BIDIR:
> > +			pci_tph->ents[i].flags |= VFIO_TPH_HINT_BIDIR;
> > +			break;
> > +		case RTE_PCI_TPH_HINT_REQSTR:
> > +			pci_tph->ents[i].flags |= VFIO_TPH_HINT_REQSTR;
> > +			break;
> > +		case RTE_PCI_TPH_HINT_TARGET:
> > +			pci_tph->ents[i].flags |= VFIO_TPH_HINT_TARGET;
> > +			break;
> > +		case RTE_PCI_TPH_HINT_TARGET_PRIO:
> > +			pci_tph->ents[i].flags |=
> VFIO_TPH_HINT_TARGET_PRIO;
> > +			break;
> > +		default:
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +
> > +		if (op == RTE_PCI_TPH_ST_SET)
> > +			pci_tph->ents[i].index = info[i].index;
> > +	}
> > +
> > +	ret = pci_vfio_tph_ioctl(dev, pci_tph);
> > +	if (ret)
> 
> Again, check explicitly for "== 0".
> 

+1

> > +		goto out;
> > +
> > +	/*
> > +	 * Kernel returns steering-tag and ph-ignore bits for
> > +	 * RTE_PCI_TPH_ST_SET too, therefore copy output for
> > +	 * both RTE_PCI_TPH_ST_SET and RTE_PCI_TPH_ST_GET
> > +	 * cases.
> > +	 */
> > +	for (i = 0; i < count; i++) {
> > +		info[i].st = pci_tph->ents[i].st;
> > +		info[i].ph_ignore = pci_tph->ents[i].ph_ignore;
> > +	}
> > +
> > +out:
> > +	if (pci_tph)
> > +		rte_free(pci_tph);
> 
> Free functions work fine with null pointers, so just call free without a null check.
> 

+1

> > +	return ret;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(pci_vfio_tph_enable, 25.07) int
> > +pci_vfio_tph_enable(const struct rte_pci_device *dev, int mode) {
> > +	int ret;
> > +
> > +	if (!(mode ^ (mode & VFIO_TPH_ST_MODE_MASK))) {
> 
> So it's an error to twice set the mode to the same thing? should it not just be a
> no-op?
> 

This doesn't make sense; I will fix this in next version.

> > +		ret = -EINVAL;
> > +		goto out;
> > +	} else
> > +		mode &= VFIO_TPH_ST_MODE_MASK;
> > +
> > +	struct vfio_pci_tph pci_tph = {
> > +		.argsz = sizeof(struct vfio_pci_tph),
> > +		.flags = VFIO_DEVICE_TPH_ENABLE | mode,
> > +		.count = 0
> > +	};
> > +
> > +	ret = pci_vfio_tph_ioctl(dev, &pci_tph);
> > +out:
> > +	return ret;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(pci_vfio_tph_disable, 25.07) int
> > +pci_vfio_tph_disable(const struct rte_pci_device *dev) {
> 
> Check here, or in caller to see if it's already enabled?
> 

This check should happen in rte_pci_tph_disable() rather than here which is
specific for Linux. I will do the check where you pointed it out earlier.

> > +	struct vfio_pci_tph pci_tph = {
> > +		.argsz = sizeof(struct vfio_pci_tph),
> > +		.flags = VFIO_DEVICE_TPH_DISABLE,
> > +		.count = 0
> > +	};
> > +
> > +	return pci_vfio_tph_ioctl(dev, &pci_tph); }
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(pci_vfio_tph_st_get, 25.07) int
> > +pci_vfio_tph_st_get(const struct rte_pci_device *dev,
> > +		    struct rte_tph_info *info, size_t count) {
> > +	return pci_vfio_tph_st_op(dev, info, count, RTE_PCI_TPH_ST_GET); }
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(pci_vfio_tph_st_set, 25.07) int
> > +pci_vfio_tph_st_set(const struct rte_pci_device *dev,
> > +		    struct rte_tph_info *info, size_t count) {
> > +	return pci_vfio_tph_st_op(dev, info, count, RTE_PCI_TPH_ST_SET); }
> > +
> >  int
> >  pci_vfio_is_enabled(void)
> >  {
> > diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> > index 38109844b9..d2ec370320 100644
> > --- a/drivers/bus/pci/private.h
> > +++ b/drivers/bus/pci/private.h
> > @@ -335,4 +335,12 @@ rte_pci_dev_iterate(const void *start,  int
> > rte_pci_devargs_parse(struct rte_devargs *da);
> >
> > +/*
> > + * TPH Steering-Tag operation types.
> > + */
> > +enum rte_pci_st_op {
> > +	RTE_PCI_TPH_ST_SET, /* Set TPH Steering - Tags */
> > +	RTE_PCI_TPH_ST_GET  /* Get TPH Steering - Tags */ };
> > +
> >  #endif /* _PCI_PRIVATE_H_ */
> > diff --git a/drivers/bus/pci/rte_bus_pci.h
> > b/drivers/bus/pci/rte_bus_pci.h index 19a7b15b99..e4d4780f54 100644
> > --- a/drivers/bus/pci/rte_bus_pci.h
> > +++ b/drivers/bus/pci/rte_bus_pci.h
> > @@ -31,6 +31,7 @@ extern "C" {
> >  struct rte_pci_device;
> >  struct rte_pci_driver;
> >  struct rte_pci_ioport;
> > +struct rte_tph_info;
> >
> >  struct rte_devargs;
> >
> > @@ -312,6 +313,72 @@ void rte_pci_ioport_read(struct rte_pci_ioport
> > *p,  void rte_pci_ioport_write(struct rte_pci_ioport *p,
> >  		const void *data, size_t len, off_t offset);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Enable TLP Processing Hints (TPH) in the endpoint device.
> > + *
> > + * @param dev
> > + *   A pointer to a rte_pci_device structure describing the device
> > + *   to use.
> > + * @param mode
> > + *   TPH mode the device must operate in.
> > + */
> > +__rte_experimental
> > +int rte_pci_tph_enable(struct rte_pci_device *dev, int mode);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Disable TLP Processing Hints (TPH) in the endpoint device.
> > + *
> > + * @param dev
> > + *   A pointer to a rte_pci_device structure describing the device
> > + *   to use.
> > + */
> > +__rte_experimental
> > +int rte_pci_tph_disable(struct rte_pci_device *dev);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Get PCI Steering-Tags (STs) for a list of stashing targets.
> > + *
> > + * @param mode
> > + *   TPH mode the device must operate in.
> > + * @param info
> > + *   An array of rte_tph_info objects, each describing the target
> > + *   cpu-id, cache-level, etc. Steering-tags for each target is
> > + *   eturned via info array.
> > + * @param count
> > + *   The number of elements in the info array.
> > + */
> > +__rte_experimental
> > +int rte_pci_tph_st_get(const struct rte_pci_device *dev,
> > +		struct rte_tph_info *info, size_t count);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Set PCI Steering-Tags (STs) for a list of stashing targets.
> > + *
> > + * @param mode
> > + *   TPH mode the device must operate in.
> > + * @param info
> > + *   An array of rte_tph_info objects, each describing the target
> > + *   cpu-id, cache-level, etc. Steering-tags for each target is
> > + *   eturned via info array.
> > + * @param count
> > + *   The number of elements in the info array.
> > + */
> > +__rte_experimental
> > +int rte_pci_tph_st_set(const struct rte_pci_device *dev,
> > +		struct rte_tph_info *info, size_t count);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/drivers/bus/pci/windows/pci.c
> > b/drivers/bus/pci/windows/pci.c index e7e449306e..218e667a5a 100644
> > --- a/drivers/bus/pci/windows/pci.c
> > +++ b/drivers/bus/pci/windows/pci.c
> > @@ -511,3 +511,46 @@ rte_pci_scan(void)
> >
> >  	return ret;
> >  }
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_enable, 25.07) int
> > +rte_pci_tph_enable(struct rte_pci_device *dev, int mode) {
> > +	RTE_SET_USED(dev);
> > +	RTE_SET_USED(mode);
> > +	/* This feature is not yet implemented for windows */
> > +	return -1;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_disable, 25.07) int
> > +rte_pci_tph_disable(struct rte_pci_device *dev) {
> > +	RTE_SET_USED(dev);
> > +	/* This feature is not yet implemented for windows */
> > +	return -1;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_st_get, 25.07) int
> > +rte_pci_tph_st_get(const struct rte_pci_device *dev,
> > +		   struct rte_tph_info *info, size_t count) {
> > +	RTE_SET_USED(dev);
> > +	RTE_SET_USED(info);
> > +	RTE_SET_USED(count);
> > +	/* This feature is not yet implemented for windows */
> > +	return -1;
> > +}
> > +
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pci_tph_st_set, 25.07) int
> > +rte_pci_tph_st_set(const struct rte_pci_device *dev,
> > +		   struct rte_tph_info *info, size_t count) {
> > +	RTE_SET_USED(dev);
> > +	RTE_SET_USED(info);
> > +	RTE_SET_USED(count);
> > +	/* This feature is not yet implemented for windows */
> > +	return -1;
> > +}
> > diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h index
> > 9a50a12142..da9cd666bf 100644
> > --- a/lib/pci/rte_pci.h
> > +++ b/lib/pci/rte_pci.h
> > @@ -137,6 +137,21 @@ extern "C" {
> >  /* Process Address Space ID (RTE_PCI_EXT_CAP_ID_PASID) */
> >  #define RTE_PCI_PASID_CTRL		0x06    /* PASID control register */
> >
> > +/* TPH Requester */
> > +#define RTE_PCI_TPH_CAP            4       /* capability register */
> > +#define RTE_PCI_TPH_CAP_ST_NS      0x00000001 /* No ST Mode Supported
> */
> > +#define RTE_PCI_TPH_CAP_ST_IV      0x00000002 /* Interrupt Vector Mode
> Supported */
> > +#define RTE_PCI_TPH_CAP_ST_DS      0x00000004 /* Device Specific Mode
> Supported */
> > +#define RTE_PCI_TPH_CAP_EXT_TPH    0x00000100 /* Ext TPH Requester
> Supported */
> > +#define RTE_PCI_TPH_CAP_LOC_MASK   0x00000600 /* ST Table Location */
> > +#define RTE_PCI_TPH_LOC_NONE       0x00000000 /* Not present */
> > +#define RTE_PCI_TPH_LOC_CAP        0x00000200 /* In capability */
> > +#define RTE_PCI_TPH_LOC_MSIX       0x00000400 /* In MSI-X */
> > +#define RTE_PCI_TPH_CAP_ST_MASK    0x07FF0000 /* ST Table Size */
> > +#define RTE_PCI_TPH_CAP_ST_SHIFT   16      /* ST Table Size shift */
> > +#define RTE_PCI_TPH_BASE_SIZEOF    0xc     /* Size with no ST table */
> > +
> > +
> 
> Where are all these values used? They don't seem to be needed by this patch. If
> needed in later patches, I'd suggest adding them there.
> 

RTE_PCI_TPH_CAP_ST_NS, RTE_PCI_TPH_CAP_ST_IV and RTE_PCI_TPH_CAP_ST_DS
are used by drivers. I40e patch uses RTE_PCI_TPH_CAP_ST_DS.
I will remove the rest, added here for completeness.

> >  /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
> > #define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> > #define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")
> > --
> > 2.43.0
> >