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