Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] cxl/mem: Fix register block offset calculation
@ 2021-04-15 23:26 Ben Widawsky
  2021-04-15 23:26 ` [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex Ben Widawsky
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Ben Widawsky @ 2021-04-15 23:26 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, linux-pci, linux-acpi, ira.weiny, vishal.l.verma,
	alison.schofield, dan.j.williams, linux-kernel

The offset for the register block should be a 64K aligned value, and
therefore FIELD_GET (which will shift) is not correct for the
calculation.

From 8.1.9.1 of the CXL 2.0 spec:
  A[31:16] of offset from the address contained by one of the Function's
  Base Address Registers to point to the base of the Register Block.
  Register Block Offset is 64K aligned. Hence A[15:0] is zero

Fix this by simply using a mask.

This wasn't found earlier because the primary development done in the
QEMU environment only uses 0 offsets

Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index e3003f49b329..1b5078311f7d 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -998,7 +998,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 		return NULL;
 	}
 
-	offset = ((u64)reg_hi << 32) | FIELD_GET(CXL_REGLOC_ADDR_MASK, reg_lo);
+	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
 	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
 
 	/* Basic sanity check that BAR is big enough */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex
  2021-04-15 23:26 [PATCH 1/3] cxl/mem: Fix register block offset calculation Ben Widawsky
@ 2021-04-15 23:26 ` Ben Widawsky
  2021-04-15 23:34   ` Verma, Vishal L
  2021-04-15 23:26 ` [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs Ben Widawsky
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Ben Widawsky @ 2021-04-15 23:26 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, linux-pci, linux-acpi, ira.weiny, vishal.l.verma,
	alison.schofield, dan.j.williams, linux-kernel

Trivial. The spec lists these as hex, so do the same here to make
debugging easier.

Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 1b5078311f7d..c05617b0ba4b 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -939,7 +939,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			cxlm->memdev_regs = register_block;
 			break;
 		default:
-			dev_dbg(dev, "Unknown cap ID: %d (0x%x)\n", cap_id, offset);
+			dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
 			break;
 		}
 	}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs
  2021-04-15 23:26 [PATCH 1/3] cxl/mem: Fix register block offset calculation Ben Widawsky
  2021-04-15 23:26 ` [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex Ben Widawsky
@ 2021-04-15 23:26 ` Ben Widawsky
  2021-04-15 23:27   ` Ben Widawsky
  2021-04-16  0:29   ` Dan Williams
  2021-04-16  0:03 ` [PATCH 1/3] cxl/mem: Fix register block offset calculation Dan Williams
  2021-04-16  2:49 ` [PATCH v2] " Ben Widawsky
  3 siblings, 2 replies; 10+ messages in thread
From: Ben Widawsky @ 2021-04-15 23:26 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, linux-pci, linux-acpi, ira.weiny, vishal.l.verma,
	alison.schofield, dan.j.williams, linux-kernel

Vendor capabilities occupy 0x8000 to 0xFFFF according to CXL 2.0 spec
8.2.8.2.1 CXL Device Capabilities. While they are not defined by the
spec, they are allowed and not "unknown". Call this detail out in the
logs to let users easily distinguish the difference.

Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c05617b0ba4b..0909f73db994 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -939,7 +939,10 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			cxlm->memdev_regs = register_block;
 			break;
 		default:
-			dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
+			if (cap_id > 0x8000)
+				dev_dbg(dev, "Vendor cap ID: %x (0x%x)\n", cap_id, offset);
+			else
+				dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
 			break;
 		}
 	}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs
  2021-04-15 23:26 ` [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs Ben Widawsky
@ 2021-04-15 23:27   ` Ben Widawsky
  2021-04-15 23:37     ` Verma, Vishal L
  2021-04-16  0:29   ` Dan Williams
  1 sibling, 1 reply; 10+ messages in thread
From: Ben Widawsky @ 2021-04-15 23:27 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, linux-pci, linux-acpi, ira.weiny, vishal.l.verma,
	alison.schofield, dan.j.williams, linux-kernel

Vendor capabilities occupy 0x8000 to 0xFFFF according to CXL 2.0 spec
8.2.8.2.1 CXL Device Capabilities. While they are not defined by the
spec, they are allowed and not "unknown". Call this detail out in the
logs to let users easily distinguish the difference.

v2: Should be greater than or equal to (Ben)

Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c05617b0ba4b..28c7c29567b3 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -939,7 +939,10 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
 			cxlm->memdev_regs = register_block;
 			break;
 		default:
-			dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
+			if (cap_id >= 0x8000)
+				dev_dbg(dev, "Vendor cap ID: %x (0x%x)\n", cap_id, offset);
+			else
+				dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
 			break;
 		}
 	}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex
  2021-04-15 23:26 ` [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex Ben Widawsky
@ 2021-04-15 23:34   ` Verma, Vishal L
  0 siblings, 0 replies; 10+ messages in thread
From: Verma, Vishal L @ 2021-04-15 23:34 UTC (permalink / raw)
  To: Widawsky, Ben, linux-cxl
  Cc: Williams, Dan J, linux-pci, Schofield, Alison, linux-kernel,
	linux-acpi, Weiny, Ira

On Thu, 2021-04-15 at 16:26 -0700, Ben Widawsky wrote:
> Trivial. The spec lists these as hex, so do the same here to make
> debugging easier.
> 
> Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 1b5078311f7d..c05617b0ba4b 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -939,7 +939,7 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  			cxlm->memdev_regs = register_block;
>  			break;
>  		default:
> -			dev_dbg(dev, "Unknown cap ID: %d (0x%x)\n", cap_id, offset);
> +			dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);

Would %#x be better just for making it unambiguous? Maybe also change
the adjacent 0x%x to %#x while at it.

>  			break;
>  		}
>  	}


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs
  2021-04-15 23:27   ` Ben Widawsky
@ 2021-04-15 23:37     ` Verma, Vishal L
  2021-04-15 23:50       ` Ben Widawsky
  0 siblings, 1 reply; 10+ messages in thread
From: Verma, Vishal L @ 2021-04-15 23:37 UTC (permalink / raw)
  To: Widawsky, Ben, linux-cxl
  Cc: Williams, Dan J, linux-pci, Schofield, Alison, linux-kernel,
	linux-acpi, Weiny, Ira

On Thu, 2021-04-15 at 16:27 -0700, Ben Widawsky wrote:
> Vendor capabilities occupy 0x8000 to 0xFFFF according to CXL 2.0 spec
> 8.2.8.2.1 CXL Device Capabilities. While they are not defined by the
> spec, they are allowed and not "unknown". Call this detail out in the
> logs to let users easily distinguish the difference.
> 
> v2: Should be greater than or equal to (Ben)

If there's a v3, drop this to below the '---'. Otherwise note for Dan to
drop when applying I guess :)

> 
> Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/mem.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c05617b0ba4b..28c7c29567b3 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -939,7 +939,10 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
>  			cxlm->memdev_regs = register_block;
>  			break;
>  		default:
> -			dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
> +			if (cap_id >= 0x8000)
> +				dev_dbg(dev, "Vendor cap ID: %x (0x%x)\n", cap_id, offset);
> +			else
> +				dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
>  			break;
>  		}
>  	}


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs
  2021-04-15 23:37     ` Verma, Vishal L
@ 2021-04-15 23:50       ` Ben Widawsky
  0 siblings, 0 replies; 10+ messages in thread
From: Ben Widawsky @ 2021-04-15 23:50 UTC (permalink / raw)
  To: Verma, Vishal L
  Cc: linux-cxl, Williams, Dan J, linux-pci, Schofield, Alison,
	linux-kernel, linux-acpi, Weiny, Ira

On 21-04-15 16:37:01, Verma, Vishal L wrote:
> On Thu, 2021-04-15 at 16:27 -0700, Ben Widawsky wrote:
> > Vendor capabilities occupy 0x8000 to 0xFFFF according to CXL 2.0 spec
> > 8.2.8.2.1 CXL Device Capabilities. While they are not defined by the
> > spec, they are allowed and not "unknown". Call this detail out in the
> > logs to let users easily distinguish the difference.
> >
> > v2: Should be greater than or equal to (Ben)
> 
> If there's a v3, drop this to below the '---'. Otherwise note for Dan to
> drop when applying I guess :)
> 

Thanks... This one is an old habit. I'll point out too I messed up the subject
here.

> >
> > Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")
> > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > ---
> >  drivers/cxl/mem.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index c05617b0ba4b..28c7c29567b3 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -939,7 +939,10 @@ static int cxl_mem_setup_regs(struct cxl_mem *cxlm)
> >  cxlm->memdev_regs = register_block;
> >  break;
> >  default:
> > -dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
> > +if (cap_id >= 0x8000)
> > +dev_dbg(dev, "Vendor cap ID: %x (0x%x)\n", cap_id, offset);
> > +else
> > +dev_dbg(dev, "Unknown cap ID: %x (0x%x)\n", cap_id, offset);
> >  break;
> >  }
> >  }
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] cxl/mem: Fix register block offset calculation
  2021-04-15 23:26 [PATCH 1/3] cxl/mem: Fix register block offset calculation Ben Widawsky
  2021-04-15 23:26 ` [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex Ben Widawsky
  2021-04-15 23:26 ` [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs Ben Widawsky
@ 2021-04-16  0:03 ` Dan Williams
  2021-04-16  2:49 ` [PATCH v2] " Ben Widawsky
  3 siblings, 0 replies; 10+ messages in thread
From: Dan Williams @ 2021-04-16  0:03 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Linux ACPI, Weiny, Ira, Vishal L Verma,
	Schofield, Alison, Linux Kernel Mailing List

On Thu, Apr 15, 2021 at 4:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> The offset for the register block should be a 64K aligned value, and
> therefore FIELD_GET (which will shift) is not correct for the
> calculation.
>
> From 8.1.9.1 of the CXL 2.0 spec:
>   A[31:16] of offset from the address contained by one of the Function's
>   Base Address Registers to point to the base of the Register Block.
>   Register Block Offset is 64K aligned. Hence A[15:0] is zero
>
> Fix this by simply using a mask.

The above reads slightly funny to me, is this any clearer?

The "Register Offset Low" register of a "DVSEC Register Locator"
contains the 64K aligned offset for the registers along with the BAR
indicator and an id. The implementation was treating the "Register
Block Offset Low" field a value rather than as a pre-aligned component
of the 64-bit offset. So, just mask, don't mask and shift (FIELD_GET).

>
> This wasn't found earlier because the primary development done in the
> QEMU environment only uses 0 offsets
>
> Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")

As I've learned, linux-next will flag this as the wrong format.

Fixes: 8adaf747c9f0 ("cxl/mem: Find device capabilities")

...i.e. looks like your core.abbrev setting is 13 rather than 12 per
Documentation/process/submitting-patches.rst

Other than that, fix looks good to me.

> Reported-by: Vishal Verma <vishal.l.verma@intel.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  drivers/cxl/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index e3003f49b329..1b5078311f7d 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -998,7 +998,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
>                 return NULL;
>         }
>
> -       offset = ((u64)reg_hi << 32) | FIELD_GET(CXL_REGLOC_ADDR_MASK, reg_lo);
> +       offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
>         bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
>
>         /* Basic sanity check that BAR is big enough */
> --
> 2.31.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs
  2021-04-15 23:26 ` [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs Ben Widawsky
  2021-04-15 23:27   ` Ben Widawsky
@ 2021-04-16  0:29   ` Dan Williams
  1 sibling, 0 replies; 10+ messages in thread
From: Dan Williams @ 2021-04-16  0:29 UTC (permalink / raw)
  To: Ben Widawsky
  Cc: linux-cxl, Linux PCI, Linux ACPI, Weiny, Ira, Vishal L Verma,
	Schofield, Alison, Linux Kernel Mailing List

On Thu, Apr 15, 2021 at 4:26 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> Vendor capabilities occupy 0x8000 to 0xFFFF according to CXL 2.0 spec
> 8.2.8.2.1 CXL Device Capabilities. While they are not defined by the
> spec, they are allowed and not "unknown". Call this detail out in the
> logs to let users easily distinguish the difference.
>
> Fixes: 8adaf747c9f0b ("cxl/mem: Find device capabilities")

Would need the abbrev length fixup if this was a fix, but I don't
think I can justify this to Linus as a fix v5.12. It's new development
for the next merge window.

Same comment for patch2.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] cxl/mem: Fix register block offset calculation
  2021-04-15 23:26 [PATCH 1/3] cxl/mem: Fix register block offset calculation Ben Widawsky
                   ` (2 preceding siblings ...)
  2021-04-16  0:03 ` [PATCH 1/3] cxl/mem: Fix register block offset calculation Dan Williams
@ 2021-04-16  2:49 ` Ben Widawsky
  3 siblings, 0 replies; 10+ messages in thread
From: Ben Widawsky @ 2021-04-16  2:49 UTC (permalink / raw)
  To: linux-cxl
  Cc: Ben Widawsky, linux-pci, linux-acpi, ira.weiny, vishal.l.verma,
	alison.schofield, dan.j.williams, linux-kernel

The offset for the register block should be a 64K aligned value, and
therefore FIELD_GET (which will shift) is not correct for the
calculation.

From 8.1.9.1 of the CXL 2.0 spec:
  A[31:16] of offset from the address contained by one of the Function's
  Base Address Registers to point to the base of the Register Block.
  Register Block Offset is 64K aligned. Hence A[15:0] is zero

The "Register Offset Low" register of a "DVSEC Register Locator"
contains the 64K aligned offset for the registers along with the BAR
indicator and an ID. The implementation was treating the "Register Block
Offset Low" field a value rather than as a pre-aligned component of the
64-bit offset. So, just mask, don't mask and shift (FIELD_GET).

This wasn't found earlier because the primary development done in the
QEMU environment only uses 0 offsets

Fixes: 8adaf747c9f0 ("cxl/mem: Find device capabilities")
Reported-by: Vishal Verma <vishal.l.verma@intel.com>
Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
---
 drivers/cxl/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index e3003f49b329..1b5078311f7d 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -998,7 +998,7 @@ static struct cxl_mem *cxl_mem_create(struct pci_dev *pdev, u32 reg_lo,
 		return NULL;
 	}
 
-	offset = ((u64)reg_hi << 32) | FIELD_GET(CXL_REGLOC_ADDR_MASK, reg_lo);
+	offset = ((u64)reg_hi << 32) | (reg_lo & CXL_REGLOC_ADDR_MASK);
 	bar = FIELD_GET(CXL_REGLOC_BIR_MASK, reg_lo);
 
 	/* Basic sanity check that BAR is big enough */
-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 23:26 [PATCH 1/3] cxl/mem: Fix register block offset calculation Ben Widawsky
2021-04-15 23:26 ` [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex Ben Widawsky
2021-04-15 23:34   ` Verma, Vishal L
2021-04-15 23:26 ` [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs Ben Widawsky
2021-04-15 23:27   ` Ben Widawsky
2021-04-15 23:37     ` Verma, Vishal L
2021-04-15 23:50       ` Ben Widawsky
2021-04-16  0:29   ` Dan Williams
2021-04-16  0:03 ` [PATCH 1/3] cxl/mem: Fix register block offset calculation Dan Williams
2021-04-16  2:49 ` [PATCH v2] " Ben Widawsky

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git