diff mbox series

cxl/regs: Fix size of CXL Capability Header Register

Message ID 20220201153437.2873-1-Jonathan.Cameron@huawei.com
State Accepted
Commit 74b0fe80409733055971bbfaf33c80a33fddeeb3
Headers show
Series cxl/regs: Fix size of CXL Capability Header Register | expand

Commit Message

Jonathan Cameron Feb. 1, 2022, 3:34 p.m. UTC
In CXL 2.0, 8.2.5.1 CXL Capability Header Register: this register
is given as 32 bits.

8.2.3 which covers the CXL 2.0 Component registers, including the
CXL Capability Header Register states that access restrictions
specified in Section 8.2.2 apply.

8.2.2 includes:
* A 32 bit register shall be accessed as a 4 Byte quantity.
...
If these rules are not followed, the behavior is undefined.

Discovered during review of CXL QEMU emulation. Alex Bennée pointed
out there was a comment saying that 4 byte registers must be read
with a 4 byte read, but 8 byte reads were being emulated.

https://lore.kernel.org/qemu-devel/87bkzyd3c7.fsf@linaro.org/

Fixing that, led to this code failing. Whilst a given hardware
implementation 'might' work with an 8 byte read, it should not be relied
upon. The QEMU emulation v5 will return 0 and log the wrong access width.

The code moved, so one fixes tag for where this will directly apply and
also a reference to the earlier introduction of the code for backports.

Fixes: 0f06157e0135 ("cxl/core: Move register mapping infrastructure")
Fixes: 08422378c4ad ("cxl/pci: Add HDM decoder capabilities")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alex Bennée <alex.bennee@linaro.org>

---
 drivers/cxl/core/regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ben Widawsky Feb. 1, 2022, 6:29 p.m. UTC | #1
On 22-02-01 15:34:37, Jonathan Cameron wrote:
> In CXL 2.0, 8.2.5.1 CXL Capability Header Register: this register
> is given as 32 bits.
> 
> 8.2.3 which covers the CXL 2.0 Component registers, including the
> CXL Capability Header Register states that access restrictions
> specified in Section 8.2.2 apply.
> 
> 8.2.2 includes:
> * A 32 bit register shall be accessed as a 4 Byte quantity.
> ...
> If these rules are not followed, the behavior is undefined.
> 
> Discovered during review of CXL QEMU emulation. Alex Bennée pointed
> out there was a comment saying that 4 byte registers must be read
> with a 4 byte read, but 8 byte reads were being emulated.
> 
> https://lore.kernel.org/qemu-devel/87bkzyd3c7.fsf@linaro.org/
> 
> Fixing that, led to this code failing. Whilst a given hardware
> implementation 'might' work with an 8 byte read, it should not be relied
> upon. The QEMU emulation v5 will return 0 and log the wrong access width.
> 
> The code moved, so one fixes tag for where this will directly apply and
> also a reference to the earlier introduction of the code for backports.
> 
> Fixes: 0f06157e0135 ("cxl/core: Move register mapping infrastructure")
> Fixes: 08422378c4ad ("cxl/pci: Add HDM decoder capabilities")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>

oops.
Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

[snip]
diff mbox series

Patch

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index e37e23bf4355..125303ac2a6f 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -35,7 +35,7 @@  void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 			      struct cxl_component_reg_map *map)
 {
 	int cap, cap_count;
-	u64 cap_array;
+	u32 cap_array;
 
 	*map = (struct cxl_component_reg_map) { 0 };
 
@@ -45,7 +45,7 @@  void cxl_probe_component_regs(struct device *dev, void __iomem *base,
 	 */
 	base += CXL_CM_OFFSET;
 
-	cap_array = readq(base + CXL_CM_CAP_HDR_OFFSET);
+	cap_array = readl(base + CXL_CM_CAP_HDR_OFFSET);
 
 	if (FIELD_GET(CXL_CM_CAP_HDR_ID_MASK, cap_array) != CM_CAP_HDR_CAP_ID) {
 		dev_err(dev,