Re: [PATCH v1] bus/pci: revise support PASID control

[Chenbo Xia <chenbox@nvidia.com>]

+Nipun

Please cc me and Nipun if there is a new version.

> On Nov 14, 2023, at 01:27, Abdullah Sevincer <abdullah.sevincer@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> This commit revises PASID control function to accept PASID offset to
> pasid *structure* instead of taking exact register for controlling the
> feature.
> 
> PASID control function was introduced in earlier commit.
> Pls see commit 5a6878335b81 ("event/dlb2: disable PASID") and
> commit 60ea19609aec ("bus/pci: add PASID control").

Pls -> Please

> 
> Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
> ---
> drivers/bus/pci/pci_common.c      | 5 ++---
> drivers/bus/pci/rte_bus_pci.h     | 5 ++++-
> drivers/event/dlb2/pf/dlb2_main.c | 4 ++--
> lib/pci/rte_pci.h                 | 2 +-
> 4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index ba5e280d33..dbe647d15d 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -943,9 +943,8 @@ rte_pci_pasid_set_state(const struct rte_pci_device *dev,
>                off_t offset, bool enable)
> {
>        uint16_t pasid = enable;
> -       return rte_pci_write_config(dev, &pasid, sizeof(pasid), offset) < 0
> -               ? -1
> -               : 0;
> +       return rte_pci_write_config(dev, &pasid, sizeof(pasid),
> +                       offset + RTE_PCI_PASID_CTRL) < 0 ? -1 : 0;
> }

Compare the return value of rte_pci_write_config with sizeof(pasid) will be good.
Think about one case that user specify a wrong offset and rte_pci_write_config
returns a value smaller than sizeof(pasid). It will be taken as success but actually
it’s wrong. 


> 
> struct rte_pci_bus rte_pci_bus = {
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index f07bf9b588..35d07d8294 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -161,9 +161,12 @@ int rte_pci_set_bus_master(const struct rte_pci_device *dev, bool enable);
>  * @param dev
>  *   A pointer to a rte_pci_device structure.
>  * @param offset
> - *   Offset of the PASID external capability.
> + *   Offset of the PASID external capability structure.
>  * @param enable
>  *   Flag to enable or disable PASID.
> + *
> + * @return
> + * 0 on success, -1 on error in PCI config space read/write.
>  */
> __rte_internal
> int rte_pci_pasid_set_state(const struct rte_pci_device *dev,
> diff --git a/drivers/event/dlb2/pf/dlb2_main.c b/drivers/event/dlb2/pf/dlb2_main.c
> index 61a7b39eef..a95d3227a4 100644
> --- a/drivers/event/dlb2/pf/dlb2_main.c
> +++ b/drivers/event/dlb2/pf/dlb2_main.c
> @@ -518,8 +518,8 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
>        /* Disable PASID if it is enabled by default, which
>         * breaks the DLB if enabled.
>         */
> -       off = DLB2_PCI_PASID_CAP_OFFSET + RTE_PCI_PASID_CTRL;
> -       if (rte_pci_pasid_set_state(pdev, off, false)) {
> +       off = DLB2_PCI_PASID_CAP_OFFSET;
> +       if (rte_pci_pasid_set_state(pdev, off, false) < 0) {


I don’t know about the details, so it means for different devices that support PASID,
they have different offsets?

Btw, Is this cap still not exposed to user space in latest kernel?

Thanks,
Chenbo 

>                DLB2_LOG_ERR("[%s()] failed to write the pcie config space at offset %d\n",
>                                __func__, (int)off);
>                return -1;
> diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h
> index 0d2d8d8fed..c26fc77209 100644
> --- a/lib/pci/rte_pci.h
> +++ b/lib/pci/rte_pci.h
> @@ -101,7 +101,7 @@ extern "C" {
> #define RTE_PCI_EXT_CAP_ID_ACS         0x0d    /* Access Control Services */
> #define RTE_PCI_EXT_CAP_ID_SRIOV       0x10    /* SR-IOV */
> #define RTE_PCI_EXT_CAP_ID_PRI         0x13    /* Page Request Interface */
> -#define RTE_PCI_EXT_CAP_ID_PASID       0x1B    /* Process Address Space ID */
> +#define RTE_PCI_EXT_CAP_ID_PASID       0x1b    /* Process Address Space ID */
> 
> /* Advanced Error Reporting (RTE_PCI_EXT_CAP_ID_ERR) */
> #define RTE_PCI_ERR_UNCOR_STATUS       0x04    /* Uncorrectable Error Status */
> --
> 2.25.1
>