Subject: Re: [PATCH V2 2/9] PCI: Add TPH related register definition

From: Jonathan Cameron <[email-masked]>
Date: Fri, 7 Jun 2024 17:17:16 +0100

On Fri, 31 May 2024 16:38:34 -0500
Wei Huang <[email-masked]> 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 <[email-masked]>
> Signed-off-by: Eric Van Tassell <[email-masked]>
> Signed-off-by: Wei Huang <[email-masked]>
> Reviewed-by: Ajit Khaparde <[email-masked]>
> Reviewed-by: Somnath Kotur <[email-masked]> 
> Reviewed-by: Andy Gospodarek <[email-masked]>

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