[PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Abdullah Sevincer]

vfio-pci driver in Linux kernel 6.2 enables PASID by default.
In DLB hardware, enabling PASID puts DLB in SIOV mode. This
breaks DLB PF-PMD mode. For DLB PF-PMD mode to function properly
PASID needs to be disabled for kernel 6.2.

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 | 27 +++++++++++++++++++++++++++
 lib/pci/rte_pci.h                 |  5 +++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/event/dlb2/pf/dlb2_main.c b/drivers/event/dlb2/pf/dlb2_main.c
index aa03e4c311..34e47a4e33 100644
--- a/drivers/event/dlb2/pf/dlb2_main.c
+++ b/drivers/event/dlb2/pf/dlb2_main.c
@@ -190,6 +190,7 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
 	uint16_t rt_ctl_word;
 	uint32_t pri_reqs_dword;
 	uint16_t pri_ctrl_word;
+	uint16_t pasid_ctrl;
 
 	off_t pcie_cap_offset;
 	int pri_cap_offset;
@@ -197,6 +198,7 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
 	int err_cap_offset;
 	int acs_cap_offset;
 	int wait_count;
+	int pasid_cap_offset;
 
 	uint16_t devsta_busy_word;
 	uint16_t devctl_word;
@@ -514,6 +516,31 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
 		}
 	}
 
+	/* TODO - The current Linux kernel 6.2 vfio driver does not expose PASID capability to
+	 * users. It also enables PASID by default, which breaks DLB PF PMD. We have
+	 * to use the hardcoded offset for now to disable PASID. It may be different for
+	 * other device drivers since they may have different design. When PASID capability
+	 * is exposed to users, please revise this part and add api to disable PASID through
+	 * pci common code.
+	 */
+	pasid_cap_offset = RTE_PCI_PASID_CAP_OFFSET;
+
+	off = pasid_cap_offset + RTE_PCI_PASID_CTRL;
+	if (rte_pci_read_config(pdev, &pasid_ctrl, 2, off) != 2)
+		pasid_ctrl = 0;
+
+	if (pasid_ctrl) {
+		DLB2_INFO(dlb2_dev, "DLB2 disabling pasid...\n");
+
+		pasid_ctrl = 0;
+		ret = rte_pci_write_config(pdev, &pasid_ctrl, 2, off);
+		if (ret != 2) {
+			DLB2_LOG_ERR("[%s()] failed to write the pcie config space at offset %d\n",
+				__func__, (int)off);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h
index 69e932d910..772a8d5622 100644
--- a/lib/pci/rte_pci.h
+++ b/lib/pci/rte_pci.h
@@ -101,6 +101,11 @@ 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 */
+#define RTE_PCI_PASID_CAP_OFFSET        0x148   /* PASID capability offset */
 
 /* Advanced Error Reporting (RTE_PCI_EXT_CAP_ID_ERR) */
 #define RTE_PCI_ERR_UNCOR_STATUS	0x04	/* Uncorrectable Error Status */
-- 
2.25.1

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Jerin Jacob]

On Tue, Oct 31, 2023 at 4:12 AM Abdullah Sevincer
<abdullah.sevincer@intel.com> wrote:
>
> vfio-pci driver in Linux kernel 6.2 enables PASID by default.
> In DLB hardware, enabling PASID puts DLB in SIOV mode. This
> breaks DLB PF-PMD mode. For DLB PF-PMD mode to function properly
> PASID needs to be disabled for kernel 6.2.
>
> 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 | 27 +++++++++++++++++++++++++++
>  lib/pci/rte_pci.h                 |  5 +++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/drivers/event/dlb2/pf/dlb2_main.c b/drivers/event/dlb2/pf/dlb2_main.c
> index aa03e4c311..34e47a4e33 100644
> --- a/drivers/event/dlb2/pf/dlb2_main.c
> +++ b/drivers/event/dlb2/pf/dlb2_main.c
> @@ -190,6 +190,7 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
>         uint16_t rt_ctl_word;
>         uint32_t pri_reqs_dword;
>         uint16_t pri_ctrl_word;
> +       uint16_t pasid_ctrl;
>
>         off_t pcie_cap_offset;
>         int pri_cap_offset;
> @@ -197,6 +198,7 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
>         int err_cap_offset;
>         int acs_cap_offset;
>         int wait_count;
> +       int pasid_cap_offset;
>
>         uint16_t devsta_busy_word;
>         uint16_t devctl_word;
> @@ -514,6 +516,31 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev)
>                 }
>         }
>
> +       /* TODO - The current Linux kernel 6.2 vfio driver does not expose PASID capability to
> +        * users. It also enables PASID by default, which breaks DLB PF PMD. We have
> +        * to use the hardcoded offset for now to disable PASID. It may be different for
> +        * other device drivers since they may have different design. When PASID capability
> +        * is exposed to users, please revise this part and add api to disable PASID through
> +        * pci common code.
> +        */
> +       pasid_cap_offset = RTE_PCI_PASID_CAP_OFFSET;
> +
> +       off = pasid_cap_offset + RTE_PCI_PASID_CTRL;
> +       if (rte_pci_read_config(pdev, &pasid_ctrl, 2, off) != 2)
> +               pasid_ctrl = 0;
> +
> +       if (pasid_ctrl) {
> +               DLB2_INFO(dlb2_dev, "DLB2 disabling pasid...\n");
> +
> +               pasid_ctrl = 0;
> +               ret = rte_pci_write_config(pdev, &pasid_ctrl, 2, off);
> +               if (ret != 2) {
> +                       DLB2_LOG_ERR("[%s()] failed to write the pcie config space at offset %d\n",
> +                               __func__, (int)off);
> +                       return ret;
> +               }
> +       }
> +
>         return 0;
>  }

This patch can be splited as two,
1) Generic PCIe function to enable/disable PASID
2) Call generic function to disable PASID in drivers/event/dlb2/. Also
mention which Linux kernel commit
is introducing this issue in the git commit log.

RE: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Sevincer, Abdullah]

> +This patch can be splited as two,
> +1) Generic PCIe function to enable/disable PASID
> +2) Call generic function to disable PASID in drivers/event/dlb2/. Also mention which Linux kernel commit is introducing this issue in the git commit log.

Hi Jerrin,
I think I need to provide more information here, then we can decide which way we will go would be good for now. I agree to having 2 functions in pci common 
code to enable/disable PASID, but we need to have hardcoded PASID cap offset inside these functions as well since PASID capability is not exposed to user. Hence, to be more specific 
main reason to have hardcoded PASID is, rte_pci_find_ext_capability() function to retrieve the offset returns '0' since PASID is not exposed to user yet. 

We can see this is vfio_pci_config.c in kernel code where PASID is not exposed to user.
[PCI_EXT_CAP_ID_PASID]	=	0,	/* not yet */

So if it is okay to go with hardcoded offset now in these functions I will move the implementation to pci_common file.

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Jerin Jacob]

On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
<abdullah.sevincer@intel.com> wrote:
>
>
> > +This patch can be splited as two,
> > +1) Generic PCIe function to enable/disable PASID
> > +2) Call generic function to disable PASID in drivers/event/dlb2/. Also mention which Linux kernel commit is introducing this issue in the git commit log.
>
> Hi Jerrin,
> I think I need to provide more information here, then we can decide which way we will go would be good for now. I agree to having 2 functions in pci common
> code to enable/disable PASID, but we need to have hardcoded PASID cap offset inside these functions as well since PASID capability is not exposed to user. Hence, to be more specific
> main reason to have hardcoded PASID is, rte_pci_find_ext_capability() function to retrieve the offset returns '0' since PASID is not exposed to user yet.
>
> We can see this is vfio_pci_config.c in kernel code where PASID is not exposed to user.
> [PCI_EXT_CAP_ID_PASID]  =       0,      /* not yet */
>
> So if it is okay to go with hardcoded offset now in these functions I will move the implementation to pci_common file.

I would suggest, add argument option to API whether to probe the
capability or not? - 0 means probe and- non zero means specific PASID
cap offset till Linux VFIO is exposing it.

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Bruce Richardson]

On Tue, Oct 31, 2023 at 10:36:04PM +0530, Jerin Jacob wrote:
> On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
> <abdullah.sevincer@intel.com> wrote:
> >
> >
> > > +This patch can be splited as two,
> > > +1) Generic PCIe function to enable/disable PASID
> > > +2) Call generic function to disable PASID in drivers/event/dlb2/. Also mention which Linux kernel commit is introducing this issue in the git commit log.
> >
> > Hi Jerrin,
> > I think I need to provide more information here, then we can decide which way we will go would be good for now. I agree to having 2 functions in pci common
> > code to enable/disable PASID, but we need to have hardcoded PASID cap offset inside these functions as well since PASID capability is not exposed to user. Hence, to be more specific
> > main reason to have hardcoded PASID is, rte_pci_find_ext_capability() function to retrieve the offset returns '0' since PASID is not exposed to user yet.
> >
> > We can see this is vfio_pci_config.c in kernel code where PASID is not exposed to user.
> > [PCI_EXT_CAP_ID_PASID]  =       0,      /* not yet */
> >
> > So if it is okay to go with hardcoded offset now in these functions I will move the implementation to pci_common file.
> 
> I would suggest, add argument option to API whether to probe the
> capability or not? - 0 means probe and- non zero means specific PASID
> cap offset till Linux VFIO is exposing it.

That doesn't seem particularly useful to me. The calling-API in the DPDK
PMD (assuming it's PMD who use this), is no more likely to know whether
probing will work. Therefore, I think we just hard-code the offset for now.
We can decide what the best approach is later on once kernel actually
exposes the value to users. Only then will we know if it's possible to
detect that exposure or not.

/Bruce

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Jerin Jacob]

On Tue, Oct 31, 2023 at 10:45 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 31, 2023 at 10:36:04PM +0530, Jerin Jacob wrote:
> > On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
> > <abdullah.sevincer@intel.com> wrote:
> > >
> > >
> > > > +This patch can be splited as two,
> > > > +1) Generic PCIe function to enable/disable PASID
> > > > +2) Call generic function to disable PASID in drivers/event/dlb2/. Also mention which Linux kernel commit is introducing this issue in the git commit log.
> > >
> > > Hi Jerrin,
> > > I think I need to provide more information here, then we can decide which way we will go would be good for now. I agree to having 2 functions in pci common
> > > code to enable/disable PASID, but we need to have hardcoded PASID cap offset inside these functions as well since PASID capability is not exposed to user. Hence, to be more specific
> > > main reason to have hardcoded PASID is, rte_pci_find_ext_capability() function to retrieve the offset returns '0' since PASID is not exposed to user yet.
> > >
> > > We can see this is vfio_pci_config.c in kernel code where PASID is not exposed to user.
> > > [PCI_EXT_CAP_ID_PASID]  =       0,      /* not yet */
> > >
> > > So if it is okay to go with hardcoded offset now in these functions I will move the implementation to pci_common file.
> >
> > I would suggest, add argument option to API whether to probe the
> > capability or not? - 0 means probe and- non zero means specific PASID
> > cap offset till Linux VFIO is exposing it.
>
> That doesn't seem particularly useful to me. The calling-API in the DPDK
> PMD (assuming it's PMD who use this), is no more likely to know whether
> probing will work. Therefore, I think we just hard-code the offset for now.

I think, there are three things here
1) Whether to have common API for dealing with generic function like
enabling PASID or not? - I think, we are in agreement to have common
public function(Implementation could be hard-coded or probe)
2) Since it is public PCIe API, I thought of adding probing in API as
it is just LINUX limitation now. No strong opinion on inclusion of
probe in on this as Linux is main EAL which supports PCIe now.
3) Since it is PCIe capability, In my understanding the offset will
change based on the number of capabilities available in PCIe config
space for a given device. _if so_, an additional argument for the
offset
needs to be passed from PMD to common PCI API(I.e it can not be
hard-coded in common PCI code)

> We can decide what the best approach is later on once kernel actually
> exposes the value to users. Only then will we know if it's possible to
> detect that exposure or not.
>
> /Bruce

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Bruce Richardson]

On Wed, Nov 01, 2023 at 12:12:02AM +0530, Jerin Jacob wrote:
> On Tue, Oct 31, 2023 at 10:45 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Oct 31, 2023 at 10:36:04PM +0530, Jerin Jacob wrote:
> > > On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
> > > <abdullah.sevincer@intel.com> wrote:
> > > >
> > > >
> > > > > +This patch can be splited as two, +1) Generic PCIe function to
> > > > > enable/disable PASID +2) Call generic function to disable PASID
> > > > > in drivers/event/dlb2/. Also mention which Linux kernel commit is
> > > > > introducing this issue in the git commit log.
> > > >
> > > > Hi Jerrin, I think I need to provide more information here, then we
> > > > can decide which way we will go would be good for now. I agree to
> > > > having 2 functions in pci common code to enable/disable PASID, but
> > > > we need to have hardcoded PASID cap offset inside these functions
> > > > as well since PASID capability is not exposed to user. Hence, to be
> > > > more specific main reason to have hardcoded PASID is,
> > > > rte_pci_find_ext_capability() function to retrieve the offset
> > > > returns '0' since PASID is not exposed to user yet.
> > > >
> > > > We can see this is vfio_pci_config.c in kernel code where PASID is
> > > > not exposed to user.  [PCI_EXT_CAP_ID_PASID]  =       0,      /*
> > > > not yet */
> > > >
> > > > So if it is okay to go with hardcoded offset now in these functions
> > > > I will move the implementation to pci_common file.
> > >
> > > I would suggest, add argument option to API whether to probe the
> > > capability or not? - 0 means probe and- non zero means specific PASID
> > > cap offset till Linux VFIO is exposing it.
> >
> > That doesn't seem particularly useful to me. The calling-API in the
> > DPDK PMD (assuming it's PMD who use this), is no more likely to know
> > whether probing will work. Therefore, I think we just hard-code the
> > offset for now.
> 
> I think, there are three things here 1) Whether to have common API for
> dealing with generic function like enabling PASID or not? - I think, we
> are in agreement to have common public function(Implementation could be
> hard-coded or probe) 2) Since it is public PCIe API, I thought of adding
> probing in API as it is just LINUX limitation now. No strong opinion on
> inclusion of probe in on this as Linux is main EAL which supports PCIe
> now.  3) Since it is PCIe capability, In my understanding the offset will
> change based on the number of capabilities available in PCIe config space
> for a given device. _if so_, an additional argument for the offset needs
> to be passed from PMD to common PCI API(I.e it can not be hard-coded in
> common PCI code)
> 

Given these constraints and how late we are in the release cycle, I
therefore suggest we take the driver-specific bug fix for now. DLB seems
the only driver affected right now (and I can confirm the issue having
encountered it myself when running DPDK on Ubuntu 23.04).

I think a general function is a good thing to have, but it seems that such
a general function should wait until we really can make it generic in the
future.

Just my 2c. at this point.

/Bruce

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Jerin Jacob]

On Wed, Nov 1, 2023 at 2:14 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Nov 01, 2023 at 12:12:02AM +0530, Jerin Jacob wrote:
> > On Tue, Oct 31, 2023 at 10:45 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Tue, Oct 31, 2023 at 10:36:04PM +0530, Jerin Jacob wrote:
> > > > On Tue, Oct 31, 2023 at 8:43 PM Sevincer, Abdullah
> > > > <abdullah.sevincer@intel.com> wrote:
> > > > >
> > > > >
> > > > > > +This patch can be splited as two, +1) Generic PCIe function to
> > > > > > enable/disable PASID +2) Call generic function to disable PASID
> > > > > > in drivers/event/dlb2/. Also mention which Linux kernel commit is
> > > > > > introducing this issue in the git commit log.
> > > > >
> > > > > Hi Jerrin, I think I need to provide more information here, then we
> > > > > can decide which way we will go would be good for now. I agree to
> > > > > having 2 functions in pci common code to enable/disable PASID, but
> > > > > we need to have hardcoded PASID cap offset inside these functions
> > > > > as well since PASID capability is not exposed to user. Hence, to be
> > > > > more specific main reason to have hardcoded PASID is,
> > > > > rte_pci_find_ext_capability() function to retrieve the offset
> > > > > returns '0' since PASID is not exposed to user yet.
> > > > >
> > > > > We can see this is vfio_pci_config.c in kernel code where PASID is
> > > > > not exposed to user.  [PCI_EXT_CAP_ID_PASID]  =       0,      /*
> > > > > not yet */
> > > > >
> > > > > So if it is okay to go with hardcoded offset now in these functions
> > > > > I will move the implementation to pci_common file.
> > > >
> > > > I would suggest, add argument option to API whether to probe the
> > > > capability or not? - 0 means probe and- non zero means specific PASID
> > > > cap offset till Linux VFIO is exposing it.
> > >
> > > That doesn't seem particularly useful to me. The calling-API in the
> > > DPDK PMD (assuming it's PMD who use this), is no more likely to know
> > > whether probing will work. Therefore, I think we just hard-code the
> > > offset for now.
> >
> > I think, there are three things here 1) Whether to have common API for
> > dealing with generic function like enabling PASID or not? - I think, we
> > are in agreement to have common public function(Implementation could be
> > hard-coded or probe) 2) Since it is public PCIe API, I thought of adding
> > probing in API as it is just LINUX limitation now. No strong opinion on
> > inclusion of probe in on this as Linux is main EAL which supports PCIe
> > now.  3) Since it is PCIe capability, In my understanding the offset will
> > change based on the number of capabilities available in PCIe config space
> > for a given device. _if so_, an additional argument for the offset needs
> > to be passed from PMD to common PCI API(I.e it can not be hard-coded in
> > common PCI code)
> >
>
> Given these constraints and how late we are in the release cycle, I
> therefore suggest we take the driver-specific bug fix for now. DLB seems
> the only driver affected right now (and I can confirm the issue having
> encountered it myself when running DPDK on Ubuntu 23.04).
>
> I think a general function is a good thing to have, but it seems that such
> a general function should wait until we really can make it generic in the
> future.

+ PCIe maintainers.

I will leave this up to @David Marchand  / @Thomas as this patch has
common code changes and needs to come via main tree.

Also in this case, The comment was given very early(Back in June 7)
for the same.
https://patches.dpdk.org/project/dpdk/patch/20230607210050.107944-1-abdullah.sevincer@intel.com/

>
> Just my 2c. at this point.
>
> /Bruce

RE: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Sevincer, Abdullah]

>++ PCIe maintainers.

>+I will leave this up to @David Marchand  / @Thomas as this patch has common code changes and needs to come via main tree.

>+Also in this case, The comment was given very early(Back in June 7) for the same.
>+https://patches.dpdk.org/project/dpdk/patch/20230607210050.107944-1-abdullah.sevincer@intel.com/

Thanks Jerrin and Bruce for the comments.
I will wait for opinion of PCI maintainers.

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Bruce Richardson]

On Wed, Nov 01, 2023 at 07:05:54PM +0000, Sevincer, Abdullah wrote:
> 
> >++ PCIe maintainers.
> 
> >+I will leave this up to @David Marchand  / @Thomas as this patch has common code changes and needs to come via main tree.
> 
> >+Also in this case, The comment was given very early(Back in June 7) for the same.
> >+https://patches.dpdk.org/project/dpdk/patch/20230607210050.107944-1-abdullah.sevincer@intel.com/
> 
> Thanks Jerrin and Bruce for the comments.
> I will wait for opinion of PCI maintainers.

Thinking on this more, any API for enable/disable pasid would be
internal-only, so therefore would not be subject to ABI/API change rules
AFAIK. This gives us more freedom to change it as more discovery
capabilities become available.
Therefore, an initial version of the function can take the offset as
parameter, and we can update it without API/ABI concerns later. I was
previously worried about trying to get the API correct first time, but for
internal functions, we don't need to.

WDYT, Jerin, Abdullah.

/Bruce

Re: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Thomas Monjalon]

02/11/2023 11:23, Bruce Richardson:
> On Wed, Nov 01, 2023 at 07:05:54PM +0000, Sevincer, Abdullah wrote:
> > 
> > >++ PCIe maintainers.
> > 
> > >+I will leave this up to @David Marchand  / @Thomas as this patch has common code changes and needs to come via main tree.
> > 
> > >+Also in this case, The comment was given very early(Back in June 7) for the same.
> > >+https://patches.dpdk.org/project/dpdk/patch/20230607210050.107944-1-abdullah.sevincer@intel.com/
> > 
> > Thanks Jerrin and Bruce for the comments.
> > I will wait for opinion of PCI maintainers.
> 
> Thinking on this more, any API for enable/disable pasid would be
> internal-only, so therefore would not be subject to ABI/API change rules
> AFAIK. This gives us more freedom to change it as more discovery
> capabilities become available.
> Therefore, an initial version of the function can take the offset as
> parameter, and we can update it without API/ABI concerns later. I was
> previously worried about trying to get the API correct first time, but for
> internal functions, we don't need to.

It looks a good option.
Better to add an internal PCI function than doing it in a PMD.

> WDYT, Jerin, Abdullah.

RE: [PATCH v3] event/dlb2: fix disable PASID for kernel 6.2

[Sevincer, Abdullah]

>+> WDYT, Jerin, Abdullah.

It is a good idea to make it internal, I will work on that and upstream a new one.