> > 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.vithan...@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.vithan...@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
> > Reviewed-by: Dhruv Tripathi <dhruv.tripa...@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
> >
