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