DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v7 0/2] *** Disable PASID for DLB Device ***
@ 2023-11-06 17:05 Abdullah Sevincer
  2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Abdullah Sevincer @ 2023-11-06 17:05 UTC (permalink / raw)
  To: dev; +Cc: jerinj, mike.ximing.chen, bruce.richardson, thomas, Abdullah Sevincer

This series implement an internal API to disable 
PASID and calls the api to disable PASID in event/dlb2 device.

Abdullah Sevincer (2):
  bus/pci: support PASID control
  event/dlb2: fix disable PASID

 drivers/bus/pci/pci_common.c      |  7 +++++++
 drivers/bus/pci/rte_bus_pci.h     | 13 +++++++++++++
 drivers/bus/pci/version.map       |  1 +
 drivers/event/dlb2/pf/dlb2_main.c | 11 +++++++++++
 lib/pci/rte_pci.h                 |  4 ++++
 5 files changed, 36 insertions(+)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-06 17:05 [PATCH v7 0/2] *** Disable PASID for DLB Device *** Abdullah Sevincer
@ 2023-11-06 17:05 ` Abdullah Sevincer
  2023-11-06 18:30   ` David Marchand
  2023-11-13 17:27   ` [PATCH v1] bus/pci: revise " Abdullah Sevincer
  2023-11-06 17:05 ` [PATCH v7 2/2] event/dlb2: fix disable PASID Abdullah Sevincer
  2023-11-06 17:50 ` [PATCH v7 0/2] *** Disable PASID for DLB Device *** Thomas Monjalon
  2 siblings, 2 replies; 18+ messages in thread
From: Abdullah Sevincer @ 2023-11-06 17:05 UTC (permalink / raw)
  To: dev; +Cc: jerinj, mike.ximing.chen, bruce.richardson, thomas, Abdullah Sevincer

Add an internal API to control PASID for a given PCIe device.

For kernels when PASID enabled by default it breaks DLB functionality,
hence disabling PASID is required for DLB to function properly.

PASID capability is not exposed to users hence offset can not be
retrieved by rte_pci_find_ext_capability() api. Therefore, api
implemented in this commit accepts an offset for PASID with an enable
flag which is used to enable/disable PASID.

Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
---
 drivers/bus/pci/pci_common.c  |  7 +++++++
 drivers/bus/pci/rte_bus_pci.h | 13 +++++++++++++
 drivers/bus/pci/version.map   |  1 +
 lib/pci/rte_pci.h             |  4 ++++
 4 files changed, 25 insertions(+)

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 921d957bf6..ecf080c5d7 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -938,6 +938,13 @@ rte_pci_set_bus_master(const struct rte_pci_device *dev, bool enable)
 	return 0;
 }
 
+int
+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;
+}
+
 struct rte_pci_bus rte_pci_bus = {
 	.bus = {
 		.scan = rte_pci_scan,
diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
index 21e234abf0..6d836e771a 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -295,6 +295,19 @@ void rte_pci_ioport_read(struct rte_pci_ioport *p,
 void rte_pci_ioport_write(struct rte_pci_ioport *p,
 		const void *data, size_t len, off_t offset);
 
+/**
+ * Enable/Disable PASID.
+ *
+ * @param dev
+ *   A pointer to a rte_pci_device structure.
+ * @param offset
+ *   Offset of the PASID external capability.
+ * @param enable
+ *   Flag to enable or disable PASID.
+ */
+__rte_internal
+int rte_pci_pasid_set_state(const struct rte_pci_device *dev, off_t offset, bool enable);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/drivers/bus/pci/version.map b/drivers/bus/pci/version.map
index 74c5b075d5..9fad086bdf 100644
--- a/drivers/bus/pci/version.map
+++ b/drivers/bus/pci/version.map
@@ -37,5 +37,6 @@ INTERNAL {
 
 	rte_pci_get_sysfs_path;
 	rte_pci_register;
+	rte_pci_pasid_set_state;
 	rte_pci_unregister;
 };
diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h
index 69e932d910..d195f01950 100644
--- a/lib/pci/rte_pci.h
+++ b/lib/pci/rte_pci.h
@@ -101,6 +101,10 @@ 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 */
+
+/* Process Address Space ID */
+#define RTE_PCI_PASID_CTRL		0x06    /* PASID control register */
 
 /* Advanced Error Reporting (RTE_PCI_EXT_CAP_ID_ERR) */
 #define RTE_PCI_ERR_UNCOR_STATUS	0x04	/* Uncorrectable Error Status */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v7 2/2] event/dlb2: fix disable PASID
  2023-11-06 17:05 [PATCH v7 0/2] *** Disable PASID for DLB Device *** Abdullah Sevincer
  2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
@ 2023-11-06 17:05 ` Abdullah Sevincer
  2023-11-06 17:50 ` [PATCH v7 0/2] *** Disable PASID for DLB Device *** Thomas Monjalon
  2 siblings, 0 replies; 18+ messages in thread
From: Abdullah Sevincer @ 2023-11-06 17:05 UTC (permalink / raw)
  To: dev
  Cc: jerinj, mike.ximing.chen, bruce.richardson, thomas,
	Abdullah Sevincer, stable

In vfio-pci driver when PASID is enabled by default DLB hardware puts
DLB in SIOV mode. This breaks DLB PF-PMD mode. For DLB PF-PMD mode to
function properly PASID needs to be disabled.

In this commit this issue is addressed and PASID is disabled by writing
a zero to PASID control register.

Fixes: 5433956d5185 ("event/dlb2: add eventdev probe")
Cc: stable@dpdk.org

Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
---
 drivers/event/dlb2/pf/dlb2_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/event/dlb2/pf/dlb2_main.c b/drivers/event/dlb2/pf/dlb2_main.c
index aa03e4c311..61a7b39eef 100644
--- a/drivers/event/dlb2/pf/dlb2_main.c
+++ b/drivers/event/dlb2/pf/dlb2_main.c
@@ -26,6 +26,7 @@
 #define PF_ID_ZERO 0	/* PF ONLY! */
 #define NO_OWNER_VF 0	/* PF ONLY! */
 #define NOT_VF_REQ false /* PF ONLY! */
+#define DLB2_PCI_PASID_CAP_OFFSET        0x148   /* PASID capability offset */
 
 static int
 dlb2_pf_init_driver_state(struct dlb2_dev *dlb2_dev)
@@ -514,6 +515,16 @@ 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)) {
+		DLB2_LOG_ERR("[%s()] failed to write the pcie config space at offset %d\n",
+				__func__, (int)off);
+		return -1;
+	}
+
 	return 0;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v7 0/2] *** Disable PASID for DLB Device ***
  2023-11-06 17:05 [PATCH v7 0/2] *** Disable PASID for DLB Device *** Abdullah Sevincer
  2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
  2023-11-06 17:05 ` [PATCH v7 2/2] event/dlb2: fix disable PASID Abdullah Sevincer
@ 2023-11-06 17:50 ` Thomas Monjalon
  2 siblings, 0 replies; 18+ messages in thread
From: Thomas Monjalon @ 2023-11-06 17:50 UTC (permalink / raw)
  To: Abdullah Sevincer
  Cc: dev, jerinj, mike.ximing.chen, bruce.richardson, chenbox

06/11/2023 18:05, Abdullah Sevincer:
> This series implement an internal API to disable 
> PASID and calls the api to disable PASID in event/dlb2 device.
> 
> Abdullah Sevincer (2):
>   bus/pci: support PASID control
>   event/dlb2: fix disable PASID

Moved things in the right place/order and added PASID definition in Doxygen.

Applied, thanks.

PS: please keep all versions in the same mail thread.



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
@ 2023-11-06 18:30   ` David Marchand
  2023-11-06 18:50     ` Sevincer, Abdullah
  2023-11-13 17:27   ` [PATCH v1] bus/pci: revise " Abdullah Sevincer
  1 sibling, 1 reply; 18+ messages in thread
From: David Marchand @ 2023-11-06 18:30 UTC (permalink / raw)
  To: Abdullah Sevincer, thomas; +Cc: dev, jerinj, mike.ximing.chen, bruce.richardson

On Mon, Nov 6, 2023 at 6:05 PM Abdullah Sevincer
<abdullah.sevincer@intel.com> wrote:
>
> Add an internal API to control PASID for a given PCIe device.
>
> For kernels when PASID enabled by default it breaks DLB functionality,
> hence disabling PASID is required for DLB to function properly.
>
> PASID capability is not exposed to users hence offset can not be
> retrieved by rte_pci_find_ext_capability() api. Therefore, api
> implemented in this commit accepts an offset for PASID with an enable
> flag which is used to enable/disable PASID.
>
> Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
> ---
>  drivers/bus/pci/pci_common.c  |  7 +++++++
>  drivers/bus/pci/rte_bus_pci.h | 13 +++++++++++++
>  drivers/bus/pci/version.map   |  1 +
>  lib/pci/rte_pci.h             |  4 ++++
>  4 files changed, 25 insertions(+)
>
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 921d957bf6..ecf080c5d7 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -938,6 +938,13 @@ rte_pci_set_bus_master(const struct rte_pci_device *dev, bool enable)
>         return 0;
>  }
>
> +int
> +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;
> +}

I don't see much point in providing a wrapper that does nothing more
than call rte_pci_write_config() and let the driver pass the right
offsets.

If anything, can't this wrapper find out about the pasid offset itself?
There is a extended capability for this, so I would expect it can be used.

Something like (only compile tested):

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index ba5e280d33..2ca28bd4d4 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -939,13 +939,18 @@ rte_pci_set_bus_master(const struct
rte_pci_device *dev, bool enable)
 }

 int
-rte_pci_pasid_set_state(const struct rte_pci_device *dev,
-               off_t offset, bool enable)
+rte_pci_pasid_set_state(const struct rte_pci_device *dev, bool enable)
 {
-       uint16_t pasid = enable;
-       return rte_pci_write_config(dev, &pasid, sizeof(pasid), offset) < 0
-               ? -1
-               : 0;
+       uint16_t state = enable;
+       off_t pasid_offset;
+       int ret = -1;
+
+       pasid_offset = rte_pci_find_ext_capability(dev,
RTE_PCI_EXT_CAP_ID_PASID);
+       if (pasid_offset >= 0 && rte_pci_write_config(dev, &state,
sizeof(state),
+                       pasid_offset + RTE_PCI_PASID_CTRL) == sizeof(state))
+               ret = 0;
+
+       return ret;
 }

 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..6d5dbc1d50 100644
--- a/drivers/bus/pci/rte_bus_pci.h
+++ b/drivers/bus/pci/rte_bus_pci.h
@@ -160,14 +160,14 @@ 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.
  * @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,
-               off_t offset, bool enable);
+int rte_pci_pasid_set_state(const struct rte_pci_device *dev, bool enable);

 /**
  * Read PCI config space.
diff --git a/drivers/event/dlb2/pf/dlb2_main.c
b/drivers/event/dlb2/pf/dlb2_main.c
index 61a7b39eef..bd1ee4af27 100644
--- a/drivers/event/dlb2/pf/dlb2_main.c
+++ b/drivers/event/dlb2/pf/dlb2_main.c
@@ -26,7 +26,6 @@
 #define PF_ID_ZERO 0   /* PF ONLY! */
 #define NO_OWNER_VF 0  /* PF ONLY! */
 #define NOT_VF_REQ false /* PF ONLY! */
-#define DLB2_PCI_PASID_CAP_OFFSET        0x148   /* PASID capability offset */

 static int
 dlb2_pf_init_driver_state(struct dlb2_dev *dlb2_dev)
@@ -518,8 +517,7 @@ 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)) {
+       if (rte_pci_pasid_set_state(pdev, false)) {
                DLB2_LOG_ERR("[%s()] failed to write the pcie config
space at offset %d\n",
                                __func__, (int)off);
                return -1;


-- 
David Marchand


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-06 18:30   ` David Marchand
@ 2023-11-06 18:50     ` Sevincer, Abdullah
  2023-11-10  8:03       ` David Marchand
  0 siblings, 1 reply; 18+ messages in thread
From: Sevincer, Abdullah @ 2023-11-06 18:50 UTC (permalink / raw)
  To: Marchand, David, thomas; +Cc: dev, jerinj, Chen, Mike Ximing, Richardson, Bruce


>+I don't see much point in providing a wrapper that does nothing more than call rte_pci_write_config() and let the driver pass the right offsets.

>+If anything, can't this wrapper find out about the pasid offset itself?
>+There is a extended capability for this, so I would expect it can be used.

>+Something like (only compile tested):

>+diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c index ba5e280d33..2ca28bd4d4 100644
>+--- a/drivers/bus/pci/pci_common.c
>++++ b/drivers/bus/pci/pci_common.c
>+@@ -939,13 +939,18 @@ rte_pci_set_bus_master(const struct rte_pci_device *dev, bool enable)  }

>+ int
>+-rte_pci_pasid_set_state(const struct rte_pci_device *dev,
>+-               off_t offset, bool enable)
>++rte_pci_pasid_set_state(const struct rte_pci_device *dev, bool enable)
>+ {
>+-       uint16_t pasid = enable;
>+-       return rte_pci_write_config(dev, &pasid, sizeof(pasid), offset) < 0
>+-               ? -1
>+-               : 0;
>++       uint16_t state = enable;
>++       off_t pasid_offset;
>++       int ret = -1;
>++
>++       pasid_offset = rte_pci_find_ext_capability(dev,
>+RTE_PCI_EXT_CAP_ID_PASID);
>++       if (pasid_offset >= 0 && rte_pci_write_config(dev, &state,
>+sizeof(state),
>++                       pasid_offset + RTE_PCI_PASID_CTRL) == sizeof(state))
>++               ret = 0;
>++
>++       return ret;
 >+}

 >+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..6d5dbc1d50 100644
>+--- a/drivers/bus/pci/rte_bus_pci.h
>++++ b/drivers/bus/pci/rte_bus_pci.h
>+@@ -160,14 +160,14 @@ 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.
>+  * @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,
>+-               off_t offset, bool enable);
>++int rte_pci_pasid_set_state(const struct rte_pci_device *dev, bool 
>++enable);

 >+/**
  >+* Read PCI config space.
>+diff --git a/drivers/event/dlb2/pf/dlb2_main.c
>+b/drivers/event/dlb2/pf/dlb2_main.c
>+index 61a7b39eef..bd1ee4af27 100644
>+--- a/drivers/event/dlb2/pf/dlb2_main.c
>++++ b/drivers/event/dlb2/pf/dlb2_main.c
>+@@ -26,7 +26,6 @@
>+ #define PF_ID_ZERO 0   /* PF ONLY! */
 >+#define NO_OWNER_VF 0  /* PF ONLY! */
>+ #define NOT_VF_REQ false /* PF ONLY! */
>+-#define DLB2_PCI_PASID_CAP_OFFSET        0x148   /* PASID capability offset */

 >+static int
 >+dlb2_pf_init_driver_state(struct dlb2_dev *dlb2_dev) @@ -518,8 +517,7 @@ 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)) {
>++       if (rte_pci_pasid_set_state(pdev, false)) {
 >+               DLB2_LOG_ERR("[%s()] failed to write the pcie config space at offset %d\n",
 >+                               __func__, (int)off);
 >+               return -1;

Hi David,
>++       pasid_offset = rte_pci_find_ext_capability(dev,
>+RTE_PCI_EXT_CAP_ID_PASID);

That  rte_pci_find_ext_capability() api does not work for PASID since PASID is not exposed to user from kernel.
So, we can not retrieve offset. Instead we came up with a solution that passes an offset to an internal function to disable PASID and make the function internal so we can change it later.
When the linux limitation is lifted we can re-write the functions and use rte_pci_find_ext_capability api to retrieve offset and your 
solution above can be done.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-06 18:50     ` Sevincer, Abdullah
@ 2023-11-10  8:03       ` David Marchand
  2023-11-13 15:51         ` Sevincer, Abdullah
  0 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2023-11-10  8:03 UTC (permalink / raw)
  To: Sevincer, Abdullah
  Cc: thomas, dev, jerinj, Chen, Mike Ximing, Richardson, Bruce,
	Chenbo Xia, Nipun Gupta

Hello,

On Mon, Nov 6, 2023 at 7:50 PM Sevincer, Abdullah
<abdullah.sevincer@intel.com> wrote:
> Hi David,
> >++       pasid_offset = rte_pci_find_ext_capability(dev,
> >+RTE_PCI_EXT_CAP_ID_PASID);
>
> That  rte_pci_find_ext_capability() api does not work for PASID since PASID is not exposed to user from kernel.
> So, we can not retrieve offset. Instead we came up with a solution that passes an offset to an internal function to disable PASID and make the function internal so we can change it later.
> When the linux limitation is lifted we can re-write the functions and use rte_pci_find_ext_capability api to retrieve offset and your
> solution above can be done.

Adding PCI bus maintainers, Chenbo and Nipun.

Ok, that is indeed an issue.
I found some patches exposing this capability with vfio-pci but I am
not sure what is the latest work on the topic.
Do you have pointers to the latest kernel patches?


In any case, even if, in the future, the kernel exposes this
capability, we need to live with the current behavior (and probably
for a long time).
As the discovery of pasid offset is not possible, the common API merit
is low, but at least it shows what is being done by the driver.

Can we make a change so that this new API takes only the offset to the
pasid *structure* and not to the exact register controlling the
feature?
It should be something like:

$ git diff
diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index ba5e280d33..c66cefcd63 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -943,9 +943,9 @@ 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;
 }

 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..b1d17996cb 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) {
                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..94219792de 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 */



-- 
David Marchand


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-10  8:03       ` David Marchand
@ 2023-11-13 15:51         ` Sevincer, Abdullah
  2023-11-13 17:36           ` Sevincer, Abdullah
  0 siblings, 1 reply; 18+ messages in thread
From: Sevincer, Abdullah @ 2023-11-13 15:51 UTC (permalink / raw)
  To: Marchand, David
  Cc: thomas, dev, jerinj, Chen, Mike Ximing, Richardson, Bruce,
	Chenbo Xia, Nipun Gupta


>+Ok, that is indeed an issue.
>+I found some patches exposing this capability with vfio-pci but I am not sure what is the latest work on the topic.

That's right, some kernels may expose the capability some not. We realized it when Bruce reported my earlier patch with the api rte_pci_find_ext_capability() did not
work for him but it was working for me. We had different versions/flavors of kernel.

>+Do you have pointers to the latest kernel patches?
I don’t have any pointers to the latest kernel patches, though I have come across some internet search that was submitted to expose the capability but I am not sure what's the latest status on those patches. 
We can ask people involved there and get feedback.
For example below:
https://lists.linuxfoundation.org/pipermail/iommu/2020-June/045531.html

>+In any case, even if, in the future, the kernel exposes this capability, we need to live with the current behavior (and probably for a long time).
>+As the discovery of pasid offset is not possible, the common API merit is low, but at least it shows what is being done by the driver.

>+Can we make a change so that this new API takes only the offset to the pasid *structure* and not to the exact register controlling the feature?
>+It should be something like:

We can change it the way you described. The patch is already merged though 😊.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1] bus/pci: revise support PASID control
  2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
  2023-11-06 18:30   ` David Marchand
@ 2023-11-13 17:27   ` Abdullah Sevincer
  2023-11-14 13:59     ` Chenbo Xia
  2023-11-14 17:36     ` [PATCH v2] " Abdullah Sevincer
  1 sibling, 2 replies; 18+ messages in thread
From: Abdullah Sevincer @ 2023-11-13 17:27 UTC (permalink / raw)
  To: dev
  Cc: jerinj, mike.ximing.chen, bruce.richardson, thomas,
	david.marchand, Abdullah Sevincer

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

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;
 }
 
 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) {
 		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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v7 1/2] bus/pci: support PASID control
  2023-11-13 15:51         ` Sevincer, Abdullah
@ 2023-11-13 17:36           ` Sevincer, Abdullah
  0 siblings, 0 replies; 18+ messages in thread
From: Sevincer, Abdullah @ 2023-11-13 17:36 UTC (permalink / raw)
  To: Sevincer, Abdullah, Marchand, David
  Cc: thomas, dev, jerinj, Chen, Mike Ximing, Richardson, Bruce,
	Chenbo Xia, Nipun Gupta

I have pushed another patch addressing David's comments.

https://patches.dpdk.org/project/dpdk/patch/20231113172759.3529518-1-abdullah.sevincer@intel.com/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] bus/pci: revise support PASID control
  2023-11-13 17:27   ` [PATCH v1] bus/pci: revise " Abdullah Sevincer
@ 2023-11-14 13:59     ` Chenbo Xia
  2023-11-14 17:39       ` Sevincer, Abdullah
  2023-11-14 17:36     ` [PATCH v2] " Abdullah Sevincer
  1 sibling, 1 reply; 18+ messages in thread
From: Chenbo Xia @ 2023-11-14 13:59 UTC (permalink / raw)
  To: Abdullah Sevincer
  Cc: dev, jerinj, mike.ximing.chen, bruce.richardson,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	David Marchand, nipun.gupta

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2] bus/pci: revise support PASID control
  2023-11-13 17:27   ` [PATCH v1] bus/pci: revise " Abdullah Sevincer
  2023-11-14 13:59     ` Chenbo Xia
@ 2023-11-14 17:36     ` Abdullah Sevincer
  2023-11-17  6:55       ` Chenbo Xia
  1 sibling, 1 reply; 18+ messages in thread
From: Abdullah Sevincer @ 2023-11-14 17:36 UTC (permalink / raw)
  To: dev
  Cc: jerinj, mike.ximing.chen, bruce.richardson, thomas,
	david.marchand, nipun.gupta, chenbox, Abdullah Sevincer

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.
Please see commit 5a6878335b81 ("event/dlb2: disable PASID") and
commit 60ea19609aec ("bus/pci: add PASID control").

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..889a48d2af 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) != sizeof(pasid) ? -1 : 0;
 }
 
 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) {
 		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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v1] bus/pci: revise support PASID control
  2023-11-14 13:59     ` Chenbo Xia
@ 2023-11-14 17:39       ` Sevincer, Abdullah
  2023-11-15  1:53         ` Chenbo Xia
  0 siblings, 1 reply; 18+ messages in thread
From: Sevincer, Abdullah @ 2023-11-14 17:39 UTC (permalink / raw)
  To: Chenbo Xia
  Cc: dev, jerinj, Chen, Mike Ximing, Richardson, Bruce,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	Marchand, David, nipun.gupta


>+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?

Yes, may be different offsets for different devices.
As of now it is not exposed to user. Bruce's test was on 6.2 generic kernel (6.2.0-36-generic)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] bus/pci: revise support PASID control
  2023-11-14 17:39       ` Sevincer, Abdullah
@ 2023-11-15  1:53         ` Chenbo Xia
  2023-11-16 17:43           ` Chen, Mike Ximing
  0 siblings, 1 reply; 18+ messages in thread
From: Chenbo Xia @ 2023-11-15  1:53 UTC (permalink / raw)
  To: Sevincer, Abdullah
  Cc: dev, jerinj, Chen, Mike Ximing, Richardson, Bruce,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	Marchand, David, nipun.gupta

On Nov 15, 2023, at 01:39, Sevincer, Abdullah <abdullah.sevincer@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
>> +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?
> 
> Yes, may be different offsets for different devices.

But why? It’s not standard capability? In my understanding, standard cap should have the same
offset definitions for all devices.

> As of now it is not exposed to user. Bruce's test was on 6.2 generic kernel (6.2.0-36-generic)

Will kernel plan to support that? I can see the related work was done by Intel but somehow it’s
not merged into kernel. Could you give more information on this?

If kernel does not want this to be exposed, it means userspace should not access this. No?

/Chenbo 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* RE: [PATCH v1] bus/pci: revise support PASID control
  2023-11-15  1:53         ` Chenbo Xia
@ 2023-11-16 17:43           ` Chen, Mike Ximing
  2023-11-17  6:48             ` Chenbo Xia
  0 siblings, 1 reply; 18+ messages in thread
From: Chen, Mike Ximing @ 2023-11-16 17:43 UTC (permalink / raw)
  To: Chenbo Xia, Sevincer, Abdullah
  Cc: dev, jerinj, Richardson, Bruce,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	Marchand, David, nipun.gupta



> -----Original Message-----
> From: Chenbo Xia <chenbox@nvidia.com>
> Sent: Tuesday, November 14, 2023 8:54 PM
> To: Sevincer, Abdullah <abdullah.sevincer@intel.com>
> Cc: dev@dpdk.org; jerinj@marvell.com; Chen, Mike Ximing
> <mike.ximing.chen@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
> <thomas@monjalon.net>; Marchand, David <david.marchand@redhat.com>;
> nipun.gupta@amd.com
> Subject: Re: [PATCH v1] bus/pci: revise support PASID control
> 
> On Nov 15, 2023, at 01:39, Sevincer, Abdullah <abdullah.sevincer@intel.com>
> wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> >> +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?
> >
> > Yes, may be different offsets for different devices.
> 
> But why? It’s not standard capability? In my understanding, standard cap should
> have the same offset definitions for all devices.

PASID is a part of extended capabilities. Its offset can be different for different devices.

> 
> > As of now it is not exposed to user. Bruce's test was on 6.2 generic
> > kernel (6.2.0-36-generic)
> 
> Will kernel plan to support that? I can see the related work was done by Intel but
> somehow it’s not merged into kernel. Could you give more information on this?
> 
Hi Chenbo,
As you may know there has been a lot of changes in iommu/vfio/SVA/pasid/SIOV 
support in Linux kernel recently. The PASID used to be disabled, but starting with
kernel 6.2 it is enabled in vfio-pci driver by default.  We did contact the kernel developers
on this issue. They seem to insist that enabling PASID is needed for whatever new features
they are developing. This breaks the DLB PF PMD as DLB HW requires the PASID to be
disable for PF to operate properly (otherwise the HW put DLB in a different mode). We 
will continue to talk to the kernel developers on this issue, but in the meantime would like
to provide this patch so that DPDK PF PMD can still work with latest kernels.

In term of exposing the PASID capability to the user space. We are aware of some patches
Submitted in conjunction to the changes mentioned above, for example, 
https://lkml.iu.edu/hypermail/linux/kernel/2309.3/02380.html
But we don’t know when and if it will be accepted into the kernel. Hopefully the patch will
be accepted so we don’t have to use the hard coded offset.

> If kernel does not want this to be exposed, it means userspace should not access
> this. No?
> 
The action (disabling PASID) only applies the targeted device. In the DLB PF PMD case,
the DPDK has full control of the device via vfio-pci.  It does not affect kernel and any
other device's operation.

Thanks
Mike
> /Chenbo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1] bus/pci: revise support PASID control
  2023-11-16 17:43           ` Chen, Mike Ximing
@ 2023-11-17  6:48             ` Chenbo Xia
  0 siblings, 0 replies; 18+ messages in thread
From: Chenbo Xia @ 2023-11-17  6:48 UTC (permalink / raw)
  To: Chen, Mike Ximing
  Cc: Sevincer, Abdullah, dev, jerinj, Richardson, Bruce,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	Marchand, David, nipun.gupta

On Nov 17, 2023, at 01:43, Chen, Mike Ximing <mike.ximing.chen@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
>> -----Original Message-----
>> From: Chenbo Xia <chenbox@nvidia.com>
>> Sent: Tuesday, November 14, 2023 8:54 PM
>> To: Sevincer, Abdullah <abdullah.sevincer@intel.com>
>> Cc: dev@dpdk.org; jerinj@marvell.com; Chen, Mike Ximing
>> <mike.ximing.chen@intel.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>; NBU-Contact-Thomas Monjalon (EXTERNAL)
>> <thomas@monjalon.net>; Marchand, David <david.marchand@redhat.com>;
>> nipun.gupta@amd.com
>> Subject: Re: [PATCH v1] bus/pci: revise support PASID control
>> 
>> On Nov 15, 2023, at 01:39, Sevincer, Abdullah <abdullah.sevincer@intel.com>
>> wrote:
>>> 
>>> External email: Use caution opening links or attachments
>>> 
>>> 
>>>> +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?
>>> 
>>> Yes, may be different offsets for different devices.
>> 
>> But why? It’s not standard capability? In my understanding, standard cap should
>> have the same offset definitions for all devices.
> 
> PASID is a part of extended capabilities. Its offset can be different for different devices.
> 
>> 
>>> As of now it is not exposed to user. Bruce's test was on 6.2 generic
>>> kernel (6.2.0-36-generic)
>> 
>> Will kernel plan to support that? I can see the related work was done by Intel but
>> somehow it’s not merged into kernel. Could you give more information on this?
>> 
> Hi Chenbo,
> As you may know there has been a lot of changes in iommu/vfio/SVA/pasid/SIOV
> support in Linux kernel recently. The PASID used to be disabled, but starting with
> kernel 6.2 it is enabled in vfio-pci driver by default.  We did contact the kernel developers
> on this issue. They seem to insist that enabling PASID is needed for whatever new features
> they are developing. This breaks the DLB PF PMD as DLB HW requires the PASID to be
> disable for PF to operate properly (otherwise the HW put DLB in a different mode). We
> will continue to talk to the kernel developers on this issue, but in the meantime would like
> to provide this patch so that DPDK PF PMD can still work with latest kernels.
> 
> In term of exposing the PASID capability to the user space. We are aware of some patches
> Submitted in conjunction to the changes mentioned above, for example,
> https://lkml.iu.edu/hypermail/linux/kernel/2309.3/02380.html
> But we don’t know when and if it will be accepted into the kernel. Hopefully the patch will
> be accepted so we don’t have to use the hard coded offset.
> 
>> If kernel does not want this to be exposed, it means userspace should not access
>> this. No?
>> 
> The action (disabling PASID) only applies the targeted device. In the DLB PF PMD case,
> the DPDK has full control of the device via vfio-pci.  It does not affect kernel and any
> other device's operation.

Thanks for the long explanation.

Hope to see follow-up in DPDK when this capability get exposed to user later :)

/Chenbo

> 
> Thanks
> Mike
>> /Chenbo
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] bus/pci: revise support PASID control
  2023-11-14 17:36     ` [PATCH v2] " Abdullah Sevincer
@ 2023-11-17  6:55       ` Chenbo Xia
  2023-11-21 15:50         ` Thomas Monjalon
  0 siblings, 1 reply; 18+ messages in thread
From: Chenbo Xia @ 2023-11-17  6:55 UTC (permalink / raw)
  To: Abdullah Sevincer
  Cc: dev, jerinj, mike.ximing.chen, bruce.richardson,
	NBU-Contact-Thomas Monjalon (EXTERNAL),
	David Marchand, nipun.gupta


> On Nov 15, 2023, at 01:36, 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.
> Please see commit 5a6878335b81 ("event/dlb2: disable PASID") and
> commit 60ea19609aec ("bus/pci: add PASID control").
> 
> 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..889a48d2af 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) != sizeof(pasid) ? -1 : 0;
> }
> 
> 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) {
>                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
> 

Reviewed-by: Chenbo Xia <chenbox@nvidia.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2] bus/pci: revise support PASID control
  2023-11-17  6:55       ` Chenbo Xia
@ 2023-11-21 15:50         ` Thomas Monjalon
  0 siblings, 0 replies; 18+ messages in thread
From: Thomas Monjalon @ 2023-11-21 15:50 UTC (permalink / raw)
  To: Abdullah Sevincer
  Cc: dev, jerinj, mike.ximing.chen, bruce.richardson, David Marchand,
	nipun.gupta, Chenbo Xia

17/11/2023 07:55, Chenbo Xia:
> 
> > On Nov 15, 2023, at 01:36, 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.
> > Please see commit 5a6878335b81 ("event/dlb2: disable PASID") and
> > commit 60ea19609aec ("bus/pci: add PASID control").
> > 
> > Signed-off-by: Abdullah Sevincer <abdullah.sevincer@intel.com>
> 
> Reviewed-by: Chenbo Xia <chenbox@nvidia.com>

Applied, thanks.




^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-11-21 15:50 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-06 17:05 [PATCH v7 0/2] *** Disable PASID for DLB Device *** Abdullah Sevincer
2023-11-06 17:05 ` [PATCH v7 1/2] bus/pci: support PASID control Abdullah Sevincer
2023-11-06 18:30   ` David Marchand
2023-11-06 18:50     ` Sevincer, Abdullah
2023-11-10  8:03       ` David Marchand
2023-11-13 15:51         ` Sevincer, Abdullah
2023-11-13 17:36           ` Sevincer, Abdullah
2023-11-13 17:27   ` [PATCH v1] bus/pci: revise " Abdullah Sevincer
2023-11-14 13:59     ` Chenbo Xia
2023-11-14 17:39       ` Sevincer, Abdullah
2023-11-15  1:53         ` Chenbo Xia
2023-11-16 17:43           ` Chen, Mike Ximing
2023-11-17  6:48             ` Chenbo Xia
2023-11-14 17:36     ` [PATCH v2] " Abdullah Sevincer
2023-11-17  6:55       ` Chenbo Xia
2023-11-21 15:50         ` Thomas Monjalon
2023-11-06 17:05 ` [PATCH v7 2/2] event/dlb2: fix disable PASID Abdullah Sevincer
2023-11-06 17:50 ` [PATCH v7 0/2] *** Disable PASID for DLB Device *** Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).