Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34590 )
Change subject: vc/cavium/{bdk,include}: Clean up bdk_phys_to_ptr() calls ......................................................................
vc/cavium/{bdk,include}: Clean up bdk_phys_to_ptr() calls
The bdk_phys_to_ptr() function converts a uint64_t address to a void * pointer. Judging by the comments, the old implementation had a check that would refuse to convert a null pointer, which required several workarounds when trying to convert the address 0 to a pointer. This isn't the case for coreboot though, which implements this function as a simple (void *) cast, so we can remove the old workarounds.
Change-Id: I6537d1699e6726c1fb155d69a51e14da856232de Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1393962 --- M src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c M src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c M src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c M src/vendorcode/cavium/bdk/libdram/libdram.c M src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h 5 files changed, 8 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/34590/1
diff --git a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c index 834ade4..afa1ed2 100644 --- a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c +++ b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c @@ -60,7 +60,7 @@
/* Clear our work area. Checking for aliases later could get false positives if it matched stale data */ - void *ptr = (area) ? bdk_phys_to_ptr(area) : NULL; + void *ptr = bdk_phys_to_ptr(area); bdk_zero_memory(ptr, max_address - area); __bdk_dram_flush_to_mem_range(area, max_address);
diff --git a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c index 5e6e4bc..9c78667 100644 --- a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c +++ b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c @@ -101,9 +101,7 @@ void __bdk_dram_flush_to_mem(uint64_t address) { BDK_MB; - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - char *ptr = (void*)address; + char *ptr = bdk_phys_to_ptr(address); BDK_CACHE_WBI_L2(ptr); }
@@ -116,10 +114,8 @@ */ void __bdk_dram_flush_to_mem_range(uint64_t area, uint64_t max_address) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - char *ptr = (void*)area; - char *end = (void*)max_address; + char *ptr = bdk_phys_to_ptr(area); + char *end = bdk_phys_to_ptr(max_address); BDK_MB; while (ptr < end) { diff --git a/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c b/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c index 6c163da..aa7d658 100644 --- a/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c +++ b/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c @@ -141,7 +141,7 @@ len += start & BDK_CACHE_LINE_MASK; start &= ~BDK_CACHE_LINE_MASK; len = (len + BDK_CACHE_LINE_MASK) & ~BDK_CACHE_LINE_MASK; - void *ptr = (start) ? bdk_phys_to_ptr(start) : NULL; + void *ptr = bdk_phys_to_ptr(start);
while (len > 0) { diff --git a/src/vendorcode/cavium/bdk/libdram/libdram.c b/src/vendorcode/cavium/bdk/libdram/libdram.c index 214aa05..740de84 100644 --- a/src/vendorcode/cavium/bdk/libdram/libdram.c +++ b/src/vendorcode/cavium/bdk/libdram/libdram.c @@ -79,10 +79,7 @@ write to the cache line isn't good enough because partial LMC writes may be enabled */ ddr_print("N%d: Rewriting DRAM: start 0 length 0x%llx\n", node, skip); - volatile uint64_t *ptr = bdk_phys_to_ptr(bdk_numa_get_address(node, 8)); - /* The above pointer got address 8 to avoid NULL pointer checking - in bdk_phys_to_ptr(). Correct it here */ - ptr--; + volatile uint64_t *ptr = bdk_phys_to_ptr(bdk_numa_get_address(node, 0)); uint64_t *end = bdk_phys_to_ptr(bdk_numa_get_address(node, skip)); while (ptr < end) { diff --git a/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h b/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h index 60f07fa..168ef03 100644 --- a/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h +++ b/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h @@ -161,17 +161,13 @@
static inline void __bdk_dram_write64(uint64_t address, uint64_t data) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - volatile uint64_t *ptr = (void*)address; + volatile uint64_t *ptr = bdk_phys_to_ptr(address); *ptr = data; }
static inline uint64_t __bdk_dram_read64(uint64_t address) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - volatile uint64_t *ptr = (void*)address; + volatile uint64_t *ptr = bdk_phys_to_ptr(address); return *ptr; }
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34590 )
Change subject: vc/cavium/{bdk,include}: Clean up bdk_phys_to_ptr() calls ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34590 )
Change subject: vc/cavium/{bdk,include}: Clean up bdk_phys_to_ptr() calls ......................................................................
vc/cavium/{bdk,include}: Clean up bdk_phys_to_ptr() calls
The bdk_phys_to_ptr() function converts a uint64_t address to a void * pointer. Judging by the comments, the old implementation had a check that would refuse to convert a null pointer, which required several workarounds when trying to convert the address 0 to a pointer. This isn't the case for coreboot though, which implements this function as a simple (void *) cast, so we can remove the old workarounds.
Change-Id: I6537d1699e6726c1fb155d69a51e14da856232de Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 1393962 Reviewed-on: https://review.coreboot.org/c/coreboot/+/34590 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c M src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c M src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c M src/vendorcode/cavium/bdk/libdram/libdram.c M src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h 5 files changed, 8 insertions(+), 19 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c index 834ade4..afa1ed2 100644 --- a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c +++ b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test-addrbus.c @@ -60,7 +60,7 @@
/* Clear our work area. Checking for aliases later could get false positives if it matched stale data */ - void *ptr = (area) ? bdk_phys_to_ptr(area) : NULL; + void *ptr = bdk_phys_to_ptr(area); bdk_zero_memory(ptr, max_address - area); __bdk_dram_flush_to_mem_range(area, max_address);
diff --git a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c index 5e6e4bc..9c78667 100644 --- a/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c +++ b/src/vendorcode/cavium/bdk/libbdk-dram/bdk-dram-test.c @@ -101,9 +101,7 @@ void __bdk_dram_flush_to_mem(uint64_t address) { BDK_MB; - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - char *ptr = (void*)address; + char *ptr = bdk_phys_to_ptr(address); BDK_CACHE_WBI_L2(ptr); }
@@ -116,10 +114,8 @@ */ void __bdk_dram_flush_to_mem_range(uint64_t area, uint64_t max_address) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - char *ptr = (void*)area; - char *end = (void*)max_address; + char *ptr = bdk_phys_to_ptr(area); + char *end = bdk_phys_to_ptr(max_address); BDK_MB; while (ptr < end) { diff --git a/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c b/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c index 6c163da..aa7d658 100644 --- a/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c +++ b/src/vendorcode/cavium/bdk/libbdk-hal/bdk-l2c.c @@ -141,7 +141,7 @@ len += start & BDK_CACHE_LINE_MASK; start &= ~BDK_CACHE_LINE_MASK; len = (len + BDK_CACHE_LINE_MASK) & ~BDK_CACHE_LINE_MASK; - void *ptr = (start) ? bdk_phys_to_ptr(start) : NULL; + void *ptr = bdk_phys_to_ptr(start);
while (len > 0) { diff --git a/src/vendorcode/cavium/bdk/libdram/libdram.c b/src/vendorcode/cavium/bdk/libdram/libdram.c index 214aa05..740de84 100644 --- a/src/vendorcode/cavium/bdk/libdram/libdram.c +++ b/src/vendorcode/cavium/bdk/libdram/libdram.c @@ -79,10 +79,7 @@ write to the cache line isn't good enough because partial LMC writes may be enabled */ ddr_print("N%d: Rewriting DRAM: start 0 length 0x%llx\n", node, skip); - volatile uint64_t *ptr = bdk_phys_to_ptr(bdk_numa_get_address(node, 8)); - /* The above pointer got address 8 to avoid NULL pointer checking - in bdk_phys_to_ptr(). Correct it here */ - ptr--; + volatile uint64_t *ptr = bdk_phys_to_ptr(bdk_numa_get_address(node, 0)); uint64_t *end = bdk_phys_to_ptr(bdk_numa_get_address(node, skip)); while (ptr < end) { diff --git a/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h b/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h index 60f07fa..168ef03 100644 --- a/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h +++ b/src/vendorcode/cavium/include/bdk/libbdk-dram/bdk-dram-test.h @@ -161,17 +161,13 @@
static inline void __bdk_dram_write64(uint64_t address, uint64_t data) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - volatile uint64_t *ptr = (void*)address; + volatile uint64_t *ptr = bdk_phys_to_ptr(address); *ptr = data; }
static inline uint64_t __bdk_dram_read64(uint64_t address) { - /* The DRAM code doesn't use the normal bdk_phys_to_ptr() because of the - NULL check in it. This greatly slows down the memory tests */ - volatile uint64_t *ptr = (void*)address; + volatile uint64_t *ptr = bdk_phys_to_ptr(address); return *ptr; }
-
Jacob Garber (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)