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

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jun 04, 2025 at 10:52:24PM +0000, Wathsala Wathawana Vithanage wrote:
> > > 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
> > 

<snip>

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

Having them all for completeness is fine. You can keep this as-is in next
version then.

/Bruce