[v1] bus/pci: revise support PASID control

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

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

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

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
  

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

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
>