[PATCH 1/3] cxl/mem: Fix register block offset calculation

[PATCH 1/3] cxl/mem: Fix register block offset calculation

[Ben Widawsky]

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

[PATCH 2/3] cxl/mem: Print unknown capability IDs as hex

[Ben Widawsky]

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

Re: [PATCH 2/3] cxl/mem: Print unknown capability IDs as hex

[Verma, Vishal L]

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

[PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Ben Widawsky]

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

[PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Ben Widawsky]

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

Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Verma, Vishal L]

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

Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Ben Widawsky]

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

Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Dan Williams]

On Thu, Apr 15, 2021 at 4:27 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.
>
> 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);

This wants the same %#x fixup that Vishal noted on patch2 [1], and I
think it would be useful to clarify that the second number is indeed
an offset: "Unknown cap_id: %#x offset: %#x\n"

[1]: http://lore.kernel.org/r/40063fe52fcaa066a42d352b13128b6762277542.camel@intel.com

Re: [PATCH 3/3] cxl/mem: Demarcate vendor specific capability IDs

[Dan Williams]

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.

Re: [PATCH 1/3] cxl/mem: Fix register block offset calculation

[Dan Williams]

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

[PATCH v2] cxl/mem: Fix register block offset calculation

[Ben Widawsky]

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