[v6,1/2] bus/pci: add function to enable/disable PASID

Message ID 20231103182933.2831662-2-abdullah.sevincer@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series *** Disable PASID for DLB Device *** |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Abdullah Sevincer Nov. 3, 2023, 6:29 p.m. UTC
This commit implements an internal api to enable and disable PASID for
a device e.g. device driver event/dlb2.

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

Comments

Jerin Jacob Nov. 4, 2023, 7:32 a.m. UTC | #1
On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer
<abdullah.sevincer@intel.com> wrote:
>
> This commit implements an internal api to enable and disable PASID for
> a device e.g. device driver event/dlb2.

git comment can be reworded when apply.

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

Acked-by: Jerin Jacob <jerinj@marvell.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(+)
  
Thomas Monjalon Nov. 4, 2023, 9:19 a.m. UTC | #2
04/11/2023 08:32, Jerin Jacob:
> On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer
> <abdullah.sevincer@intel.com> wrote:
> >
> > This commit implements an internal api to enable and disable PASID for
> > a device e.g. device driver event/dlb2.
> 
> git comment can be reworded when apply.

What do you mean Jerin?


> > 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>
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>
  
Bruce Richardson Nov. 4, 2023, 2:01 p.m. UTC | #3
On Fri, Nov 03, 2023 at 01:29:32PM -0500, Abdullah Sevincer wrote:
> This commit implements an internal api to enable and disable PASID for
> a device e.g. device driver event/dlb2.
> 
> 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..5aac2406f1 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_ena_dis(const struct rte_pci_device *dev, off_t offset, bool enable)

While I realise we are now at v6 of this patchset, and the name was
suggested on v4, seeing it implemented I'm afraid I think
rte_pci_pasid_ena_dis is not a great name! I also agree that the pasid_set
name was a bit misleading too, leaving us with a naming problem.
I have two suggestions:

* if we want to keep one function - "rte_pci_pasid_set_state", which makes
  it clear we are not setting the pasid, but the pasid state.
* separate this explicitly into rte_pci_pasid_enable() and
  rte_pci_pasid_disable() functions. This is the cleanest solution but it
  doesn't align with some of the other functions in pci lib which set the
  state.

Jerin, any further thoughts? and sorry for late feedback.

/Bruce
  
Jerin Jacob Nov. 5, 2023, 5:43 a.m. UTC | #4
On Sat, Nov 4, 2023 at 7:31 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Nov 03, 2023 at 01:29:32PM -0500, Abdullah Sevincer wrote:
> > This commit implements an internal api to enable and disable PASID for
> > a device e.g. device driver event/dlb2.
> >
> > 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..5aac2406f1 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_ena_dis(const struct rte_pci_device *dev, off_t offset, bool enable)
>
> While I realise we are now at v6 of this patchset, and the name was
> suggested on v4, seeing it implemented I'm afraid I think
> rte_pci_pasid_ena_dis is not a great name! I also agree that the pasid_set
> name was a bit misleading too, leaving us with a naming problem.
> I have two suggestions:
>
> * if we want to keep one function - "rte_pci_pasid_set_state", which makes
>   it clear we are not setting the pasid, but the pasid state.
> * separate this explicitly into rte_pci_pasid_enable() and
>   rte_pci_pasid_disable() functions. This is the cleanest solution but it
>   doesn't align with some of the other functions in pci lib which set the
>   state.
>
> Jerin, any further thoughts? and sorry for late feedback.

Yes. Above two functions are better than ena_dis().

Other option could be, rte_pci_pasid_ctrl() which is aligned with PCI
register name.

No strong opinion for the name. Feel free to pick one from above three options.


>
> /Bruce
  
Jerin Jacob Nov. 5, 2023, 5:48 a.m. UTC | #5
On Sat, Nov 4, 2023 at 2:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 04/11/2023 08:32, Jerin Jacob:
> > On Sat, Nov 4, 2023 at 4:47 AM Abdullah Sevincer
> > <abdullah.sevincer@intel.com> wrote:
> > >
> > > This commit implements an internal api to enable and disable PASID for
> > > a device e.g. device driver event/dlb2.
> >
> > git comment can be reworded when apply.
>
> What do you mean Jerin?

Since I am not applying this patch, I thought you can reword something
like following

bus/pci: support PASID control

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>
> >
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
>
>
>
  
Chenbo Xia Nov. 6, 2023, 1:16 a.m. UTC | #6
Sorry I missed all previous versions…

+ARM guy

> On Nov 4, 2023, at 02:29, Abdullah Sevincer <abdullah.sevincer@intel.com> wrote:
> 
> External email: Use caution opening links or attachments
> 
> 
> This commit implements an internal api to enable and disable PASID for
> a device e.g. device driver event/dlb2.
> 
> 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>

Is PASID now part of PCIe spec? This APIs should both work for x86/arm?
Not sure ARM is OK with the naming, previously they are calling it more as
Sub Stream ID (SSID)

> ---
> 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..5aac2406f1 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_ena_dis(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..d45b7bf2ab 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_ena_dis(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..01e6a09eb6 100644
> --- a/drivers/bus/pci/version.map
> +++ b/drivers/bus/pci/version.map
> @@ -36,6 +36,7 @@ INTERNAL {
>        global:
> 
>        rte_pci_get_sysfs_path;
> +       rte_pci_pasid_ena_dis;
>        rte_pci_register;
>        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 */

Align with old definitions will looks better. Using TAB?

Thanks,
Chenbo

> 
> /* Advanced Error Reporting (RTE_PCI_EXT_CAP_ID_ERR) */
> #define RTE_PCI_ERR_UNCOR_STATUS       0x04    /* Uncorrectable Error Status */
> --
> 2.25.1
>
  
Abdullah Sevincer Nov. 6, 2023, 3:44 p.m. UTC | #7
>+Is PASID now part of PCIe spec? This APIs should both work for x86/arm?
>+Not sure ARM is OK with the naming, previously they are calling it more as Sub Stream ID (SSID)
For reference, Look for PASID definitions in the PCIe spec.
The API takes in offset which might be different for other devices. Part of the reason why we defined the API this way.

>+Align with old definitions will looks better. Using TAB?
I will align.

For v7 I will use naming rte_pci_pasid_set_state, is everyone okay with that?

Thanks.
Abdullah.
  
Ruifeng Wang Nov. 7, 2023, 8:17 a.m. UTC | #8
On 2023/11/6 9:16 AM, Chenbo Xia wrote:
> Sorry I missed all previous versions…
> 
> +ARM guy

Thanks for CCing.

> 
>> On Nov 4, 2023, at 02:29, Abdullah Sevincer <abdullah.sevincer@intel.com> wrote:
>>
>> External email: Use caution opening links or attachments
>>
>>
>> This commit implements an internal api to enable and disable PASID for
>> a device e.g. device driver event/dlb2.
>>
>> 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>
> 
> Is PASID now part of PCIe spec? This APIs should both work for x86/arm?
> Not sure ARM is OK with the naming, previously they are calling it more as
> Sub Stream ID (SSID)

PASID is fine to ARM.
SSID is a term used in SMMU(IOMMU) which PASID is mapped to.

>  >> ---
>> 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..5aac2406f1 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_ena_dis(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..d45b7bf2ab 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_ena_dis(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..01e6a09eb6 100644
>> --- a/drivers/bus/pci/version.map
>> +++ b/drivers/bus/pci/version.map
>> @@ -36,6 +36,7 @@ INTERNAL {
>>         global:
>>
>>         rte_pci_get_sysfs_path;
>> +       rte_pci_pasid_ena_dis;
>>         rte_pci_register;
>>         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 */
> 
> Align with old definitions will looks better. Using TAB?
> 
> Thanks,
> Chenbo
> 
>>
>> /* Advanced Error Reporting (RTE_PCI_EXT_CAP_ID_ERR) */
>> #define RTE_PCI_ERR_UNCOR_STATUS       0x04    /* Uncorrectable Error Status */
>> --
>> 2.25.1
>>
>
  

Patch

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 921d957bf6..5aac2406f1 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_ena_dis(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..d45b7bf2ab 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_ena_dis(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..01e6a09eb6 100644
--- a/drivers/bus/pci/version.map
+++ b/drivers/bus/pci/version.map
@@ -36,6 +36,7 @@  INTERNAL {
 	global:
 
 	rte_pci_get_sysfs_path;
+	rte_pci_pasid_ena_dis;
 	rte_pci_register;
 	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 */