[M] Change in coreboot[main]: soc/intel: Enable crashlog IP for both 32-bit and 64-bit architectures
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Pratikkumar V Prajapati, Sowmya Aralguppe, Subrata Banik, Tarun.
Angel Pons has posted comments on this change by Sowmya Aralguppe. ( https://review.coreboot.org/c/coreboot/+/83106?usp=email )
Change subject: soc/intel: Enable crashlog IP for both 32-bit and 64-bit architectures ......................................................................
Patch Set 19:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83106/comment/2a09481d_b71ca4ff?usp... : PS19, Line 14: tested Could you please elaborate on what was tested? As per my review comments, I doubt that reading CrashLog data from the PMC works on the current patchset.
File src/soc/intel/common/block/crashlog/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83106/comment/f6381258_59323eb6?usp... : PS15, Line 321: sizeof(uintptr_t)
I have changed dest_addr to be u32 *,IMHO that is right solution
isn't `u32 *dest_addr` also 8-byte in 64-bit arch?
It's a pointer type, but what matters is that it points to 32-bit values. Incrementing the pointer will increment it by `sizeof(u32)`.
What I prefer in these cases is to make the pointer const (`u32 *const dest_addr`) and treat it like an array (`dest_addr[i]`).
if we decide to do DW by DW copy then we need to ensure that both src and dest addr are getting incremented by 4-bytes post copy.
Looking at `dest_addr` i felt this is okay as the declaration and increment both would be 4-bytes irrespective of 32-bit vs 64bit arch.
but looks like `src_addr` is not using fixed data type.
32-bit systems: uintptr_t is 32 bits (4 bytes) 64-bit systems: uintptr_t is 64 bits (8 bytes)
Let's say src_addr initially holds the value 0x1000.
32-bit: After src_addr += sizeof(uintptr_t);, it will hold the value 0x1004. 64-bit: After src_addr += sizeof(uintptr_t);, it will hold the value 0x1008.
can you please fix this problem?
+1, my original comment still applies. This needs to use 32-bit accesses because it's reading from the PMC's address space.
-
Angel Pons (Code Review)