On Mon, Jun 02, 2025 at 10:38:02PM +0000, Wathsala Vithanage wrote:
> Extend the PCI bus driver to enable or disable TPH capability and set or
> get PCI Steering-Tags (STs) on an endpoint device. The functions
> rte_pci_tph_{enable, disable,st_set,st_get} provide the primary
> interface for DPDK device drivers. Implementation of the interface is OS
> dependent. For Linux, the kernel VFIO driver provides the
> implementation. rte_pci_tph_{enable, disable} functions enable and
> disable TPH capability, respectively. rte_pci_tph_enable enables TPH on
> the device in either of the device-specific, interrupt-vector, or
> no-steering-tag modes.
>
> rte_pci_tph_st_{get, set} functions take an array of rte_tph_info
> objects with cpu-id, cache-level, flags (processing-hint, memory-type).
> The index in rte_tph_info is the MSI-X/MSI vector/ST-table index if TPH
> was enabled in the interrupt-vector mode; the rte_pci_tph_st_get
> function ignores it. Both rte_pci_tph_st_{set, get} functions return the
> steering-tag (st) and processing-hint-ignored (ph_ignore) fields via the
> same rte_tph_info object passed into them.
>
> 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.
>
> 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
> @@ -650,3 +650,46 @@ 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)
> +{
> + RTE_SET_USED(dev);
> + RTE_SET_USED(mode);
> + /* This feature is not yet implemented for BSD */
> + 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 BSD */
> + 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 BSD */
> + 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 BSD */
> + return -1;
> +}
> 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 */
> };
>
> /**
> @@ -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*/
> +
> + /* Output */
> + uint16_t st; /*Steering tag returned by the platform*/
> + uint8_t ph_ignore; /*Platform ignores PH for the returned ST*/
> +};
Looking at the driver implementation in patch 4, I realised this use of the
info struct for "get" API is very confusing. You partially populate the
structure, then make an API call, passing that struct as input, and output
is also filled into other fields of the same structure.
I dislike having the input and output fields of the structure mixed. Can we
separate out the last two fields here into a separate output struct, or
alternatively just drop them from the struct and have the get API take two
additional arrays as output, one for the tags and another for the ph_ignore
fields.
[Note: documentation also needs to cover what PH is]
/Bruce
