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

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

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Thursday, June 5, 2025 2:51 AM
> To: Wathsala Wathawana Vithanage <wathsala.vithanage@arm.com>
> Cc: Chenbo Xia <chenbox@nvidia.com>; Nipun Gupta <nipun.gupta@amd.com>;
> Anatoly Burakov <anatoly.burakov@intel.com>; Gaetan Rivet <grive@u256.net>;
> dev@dpdk.org; nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Dhruv Tripathi <Dhruv.Tripathi@arm.com>
> Subject: Re: [PATCH v5 2/4] bus/pci: introduce the PCIe TLP Processing Hints API
> 
> 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.vithanag
> > e@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.
> 

+1

--wathsala
> /Bruce