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 <[email protected]>
Signed-off-by: Ben Widawsky <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
> }
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 <[email protected]>
> > ---
> > 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;
> > }
> > }
>
On Thu, Apr 15, 2021 at 4:26 PM Ben Widawsky <[email protected]> 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 <[email protected]>
> Signed-off-by: Ben Widawsky <[email protected]>
> ---
> 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
On Thu, Apr 15, 2021 at 4:26 PM Ben Widawsky <[email protected]> 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.
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
> }
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 <[email protected]>
Signed-off-by: Ben Widawsky <[email protected]>
---
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
On Thu, Apr 15, 2021 at 4:27 PM Ben Widawsky <[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]