[v6,1/2] bus/pci: add function to enable/disable PASID
Checks
Commit Message
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
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(+)
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>
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
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
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>
>
>
>
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
>
>+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.
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
>>
>
@@ -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,
@@ -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
@@ -36,6 +36,7 @@ INTERNAL {
global:
rte_pci_get_sysfs_path;
+ rte_pci_pasid_ena_dis;
rte_pci_register;
rte_pci_unregister;
};
@@ -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 */