diff mbox series

[V2,2/9] PCI: Add TPH related register definition

Message ID 20240531213841.3246055-3-wei.huang2@amd.com (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series PCIe TPH and cache direct injection support | expand

Commit Message

Wei Huang May 31, 2024, 9:38 p.m. UTC 
Linux has some basic, but incomplete, definition for the TPH Requester
capability registers. Also the control registers of TPH Requester and
the TPH Completer are missing. This patch adds all required definitions
to support TPH enablement.

Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
---
 drivers/vfio/pci/vfio_pci_config.c |  7 +++---
 include/uapi/linux/pci_regs.h      | 35 ++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 7 deletions(-)

Comments

Jonathan Cameron June 7, 2024, 4:17 p.m. UTC | #1 
On Fri, 31 May 2024 16:38:34 -0500
Wei Huang <wei.huang2@amd.com> wrote:

> Linux has some basic, but incomplete, definition for the TPH Requester
> capability registers. Also the control registers of TPH Requester and
> the TPH Completer are missing. This patch adds all required definitions
> to support TPH enablement.
> 
> Co-developed-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Eric Van Tassell <Eric.VanTassell@amd.com>
> Signed-off-by: Wei Huang <wei.huang2@amd.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> 
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>

As below, you can't modify uapi headers because we have no idea what
userspace code is already using them.
Also, (annoyingly) the field contents in this header tend (or maybe always are)
in the shifted form.

> ---
>  drivers/vfio/pci/vfio_pci_config.c |  7 +++---
>  include/uapi/linux/pci_regs.h      | 35 ++++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 97422aafaa7b..de622cdfc2a4 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1434,14 +1434,15 @@ static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
>  		if (ret)
>  			return pcibios_err_to_errno(ret);
>  
> -		if ((dword & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
> +		if (((dword & PCI_TPH_CAP_LOC_MASK) >> PCI_TPH_CAP_LOC_SHIFT)
> +			== PCI_TPH_LOC_CAP) {
>  			int sts;
>  
>  			sts = dword & PCI_TPH_CAP_ST_MASK;
>  			sts >>= PCI_TPH_CAP_ST_SHIFT;
> -			return PCI_TPH_BASE_SIZEOF + (sts * 2) + 2;
> +			return PCI_TPH_ST_TABLE + (sts * 2) + 2;
>  		}
> -		return PCI_TPH_BASE_SIZEOF;
> +		return PCI_TPH_ST_TABLE;
>  	case PCI_EXT_CAP_ID_DVSEC:
>  		ret = pci_read_config_dword(pdev, epos + PCI_DVSEC_HEADER1, &dword);
>  		if (ret)
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 94c00996e633..ae1cf048b04a 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -657,6 +657,7 @@
>  #define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
>  #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
>  #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
> +#define  PCI_EXP_DEVCAP2_TPH_COMP	0x00003000 /* TPH completer support */
>  #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
>  #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
>  #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
> @@ -1020,15 +1021,41 @@
>  #define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
>  #define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
>  
> +/* TPH Completer Support */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
> +
>  /* TPH Requester */
>  #define PCI_TPH_CAP		4	/* capability register */
> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
> +#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
> +#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
> +#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
> +#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
>  #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
> -#define   PCI_TPH_LOC_NONE	0x000	/* no location */
> -#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
> -#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */

It's a userspace header, relatively unlikely to be safe to change it...
This would also be inconsistent with how some other registers are defined in here.

I'd love it if we could tidy this up, but we are stuck by this being
in uapi.

> +#define  PCI_TPH_CAP_LOC_SHIFT	9	/* location shift */
> +#define   PCI_TPH_LOC_NONE	0x0	/*  no ST Table */
> +#define   PCI_TPH_LOC_CAP	0x1	/*  ST Table in extended capability */
> +#define   PCI_TPH_LOC_MSIX	0x2	/*  ST table in MSI-X table */
>  #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
>  #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
> -#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
> +
> +#define PCI_TPH_CTRL		0x8	/* control register */
> +#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x7	/* ST Model Select mask */
> +#define  PCI_TPH_CTRL_MODE_SEL_SHIFT	0x0	/* ST Model Select shift */
> +#define   PCI_TPH_NO_ST_MODE		0x0	/*  No ST Mode */
> +#define   PCI_TPH_INT_VEC_MODE		0x1	/*  Interrupt Vector Mode */
> +#define   PCI_TPH_DEV_SPEC_MODE		0x2	/*  Device Specific Mode */
> +#define  PCI_TPH_CTRL_REQ_EN_MASK	0x300	/* TPH Requester mask */
> +#define  PCI_TPH_CTRL_REQ_EN_SHIFT	8	/* TPH Requester shift */
> +#define   PCI_TPH_REQ_DISABLE		0x0	/*  No TPH request allowed */
> +#define   PCI_TPH_REQ_TPH_ONLY		0x1	/*  8-bit TPH tags allowed */
> +#define   PCI_TPH_REQ_EXT_TPH		0x3	/*  16-bit TPH tags allowed */
> +
> +#define PCI_TPH_ST_TABLE	0xc	/* base of ST table */
>  
>  /* Downstream Port Containment */
>  #define PCI_EXP_DPC_CAP			0x04	/* DPC Capability */
Bjorn Helgaas June 7, 2024, 4:42 p.m. UTC | #2 
On Fri, May 31, 2024 at 04:38:34PM -0500, Wei Huang wrote:
> Linux has some basic, but incomplete, definition for the TPH Requester
> capability registers. Also the control registers of TPH Requester and
> the TPH Completer are missing. This patch adds all required definitions
> to support TPH enablement.

s/This patch adds/Add/

> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */

Drop the _SHIFT definitions and use FIELD_GET() and FIELD_PREP()
instead.

>  /* TPH Requester */
>  #define PCI_TPH_CAP		4	/* capability register */
> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */

Drop _SHIFT and show full register width for PCI_TPH_CAP_NO_ST, e.g.,

  #define  PCI_TPH_CAP_NO_ST 0x00000001

The existing PCI_TPH_CAP_* definitions don't follow that convention,
but the rest of the file does, and this should match.
Wei Huang June 10, 2024, 8 p.m. UTC | #3 
On 6/7/24 11:17, Jonathan Cameron wrote:
> On Fri, 31 May 2024 16:38:34 -0500
> Wei Huang <wei.huang2@amd.com> wrote:
>>  /* TPH Requester */
>>  #define PCI_TPH_CAP		4	/* capability register */
>> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
>> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
>> +#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
>> +#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
>> +#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
>> +#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
>>  #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
>> -#define   PCI_TPH_LOC_NONE	0x000	/* no location */
>> -#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
>> -#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
> 
> It's a userspace header, relatively unlikely to be safe to change it...
> This would also be inconsistent with how some other registers are defined in here.
> 
> I'd love it if we could tidy this up, but we are stuck by this being
> in uapi.

Alex Williamson had a similar comment in another email. In V3, I will
only add (necessary) new definitions and refrain from touching the
existing ones.
Wei Huang June 10, 2024, 8:04 p.m. UTC | #4 
On 6/7/24 11:42, Bjorn Helgaas wrote:
> On Fri, May 31, 2024 at 04:38:34PM -0500, Wei Huang wrote:
>> Linux has some basic, but incomplete, definition for the TPH Requester
>> capability registers. Also the control registers of TPH Requester and
>> the TPH Completer are missing. This patch adds all required definitions
>> to support TPH enablement.
> 
> s/This patch adds/Add/
> 
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
>> +#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
> 
> Drop the _SHIFT definitions and use FIELD_GET() and FIELD_PREP()
> instead.
> 
>>  /* TPH Requester */
>>  #define PCI_TPH_CAP		4	/* capability register */
>> +#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
>> +#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
> 
> Drop _SHIFT and show full register width for PCI_TPH_CAP_NO_ST, e.g.,
> 
>   #define  PCI_TPH_CAP_NO_ST 0x00000001
> 
> The existing PCI_TPH_CAP_* definitions don't follow that convention,
> but the rest of the file does, and this should match.

Will fix all of the above in V3.
diff mbox series

Patch

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 97422aafaa7b..de622cdfc2a4 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -1434,14 +1434,15 @@  static int vfio_ext_cap_len(struct vfio_pci_core_device *vdev, u16 ecap, u16 epo
 		if (ret)
 			return pcibios_err_to_errno(ret);
 
-		if ((dword & PCI_TPH_CAP_LOC_MASK) == PCI_TPH_LOC_CAP) {
+		if (((dword & PCI_TPH_CAP_LOC_MASK) >> PCI_TPH_CAP_LOC_SHIFT)
+			== PCI_TPH_LOC_CAP) {
 			int sts;
 
 			sts = dword & PCI_TPH_CAP_ST_MASK;
 			sts >>= PCI_TPH_CAP_ST_SHIFT;
-			return PCI_TPH_BASE_SIZEOF + (sts * 2) + 2;
+			return PCI_TPH_ST_TABLE + (sts * 2) + 2;
 		}
-		return PCI_TPH_BASE_SIZEOF;
+		return PCI_TPH_ST_TABLE;
 	case PCI_EXT_CAP_ID_DVSEC:
 		ret = pci_read_config_dword(pdev, epos + PCI_DVSEC_HEADER1, &dword);
 		if (ret)
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 94c00996e633..ae1cf048b04a 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -657,6 +657,7 @@ 
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP64	0x00000100 /* 64b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_ATOMIC_COMP128	0x00000200 /* 128b AtomicOp completion */
 #define  PCI_EXP_DEVCAP2_LTR		0x00000800 /* Latency tolerance reporting */
+#define  PCI_EXP_DEVCAP2_TPH_COMP	0x00003000 /* TPH completer support */
 #define  PCI_EXP_DEVCAP2_OBFF_MASK	0x000c0000 /* OBFF support mechanism */
 #define  PCI_EXP_DEVCAP2_OBFF_MSG	0x00040000 /* New message signaling */
 #define  PCI_EXP_DEVCAP2_OBFF_WAKE	0x00080000 /* Re-use WAKE# for OBFF */
@@ -1020,15 +1021,41 @@ 
 #define  PCI_DPA_CAP_SUBSTATE_MASK	0x1F	/* # substates - 1 */
 #define PCI_DPA_BASE_SIZEOF	16	/* size with 0 substates */
 
+/* TPH Completer Support */
+#define PCI_EXP_DEVCAP2_TPH_COMP_SHIFT		12
+#define PCI_EXP_DEVCAP2_TPH_COMP_NONE		0x0 /* None */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_ONLY	0x1 /* TPH only */
+#define PCI_EXP_DEVCAP2_TPH_COMP_TPH_AND_EXT	0x3 /* TPH and Extended TPH */
+
 /* TPH Requester */
 #define PCI_TPH_CAP		4	/* capability register */
+#define  PCI_TPH_CAP_NO_ST	0x1	/* no ST mode supported */
+#define  PCI_TPH_CAP_NO_ST_SHIFT	0x0	/* no ST mode supported shift */
+#define  PCI_TPH_CAP_INT_VEC	0x2	/* interrupt vector mode supported */
+#define  PCI_TPH_CAP_INT_VEC_SHIFT	0x1	/* interrupt vector mode supported shift */
+#define  PCI_TPH_CAP_DS		0x4	/* device specific mode supported */
+#define  PCI_TPH_CAP_DS_SHIFT	0x4	/* device specific mode supported shift */
 #define  PCI_TPH_CAP_LOC_MASK	0x600	/* location mask */
-#define   PCI_TPH_LOC_NONE	0x000	/* no location */
-#define   PCI_TPH_LOC_CAP	0x200	/* in capability */
-#define   PCI_TPH_LOC_MSIX	0x400	/* in MSI-X */
+#define  PCI_TPH_CAP_LOC_SHIFT	9	/* location shift */
+#define   PCI_TPH_LOC_NONE	0x0	/*  no ST Table */
+#define   PCI_TPH_LOC_CAP	0x1	/*  ST Table in extended capability */
+#define   PCI_TPH_LOC_MSIX	0x2	/*  ST table in MSI-X table */
 #define PCI_TPH_CAP_ST_MASK	0x07FF0000	/* ST table mask */
 #define PCI_TPH_CAP_ST_SHIFT	16	/* ST table shift */
-#define PCI_TPH_BASE_SIZEOF	0xc	/* size with no ST table */
+
+#define PCI_TPH_CTRL		0x8	/* control register */
+#define  PCI_TPH_CTRL_MODE_SEL_MASK	0x7	/* ST Model Select mask */
+#define  PCI_TPH_CTRL_MODE_SEL_SHIFT	0x0	/* ST Model Select shift */
+#define   PCI_TPH_NO_ST_MODE		0x0	/*  No ST Mode */
+#define   PCI_TPH_INT_VEC_MODE		0x1	/*  Interrupt Vector Mode */
+#define   PCI_TPH_DEV_SPEC_MODE		0x2	/*  Device Specific Mode */
+#define  PCI_TPH_CTRL_REQ_EN_MASK	0x300	/* TPH Requester mask */
+#define  PCI_TPH_CTRL_REQ_EN_SHIFT	8	/* TPH Requester shift */
+#define   PCI_TPH_REQ_DISABLE		0x0	/*  No TPH request allowed */
+#define   PCI_TPH_REQ_TPH_ONLY		0x1	/*  8-bit TPH tags allowed */
+#define   PCI_TPH_REQ_EXT_TPH		0x3	/*  16-bit TPH tags allowed */
+
+#define PCI_TPH_ST_TABLE	0xc	/* base of ST table */
 
 /* Downstream Port Containment */
 #define PCI_EXP_DPC_CAP			0x04	/* DPC Capability */