Fix memtransport, move debug prints to utils #68
Conversation
src/wh_transport_mem.c
Outdated
| } | ||
|
|
||
| /* Ensure the memcpy is complete */ | ||
| XMEMFENCE(); |
There was a problem hiding this comment.
If we are always fencing after a memcpy, should we just pull this into wh_Utils_memcpy_flush()?
There was a problem hiding this comment.
This is a bug. The fence needed to be after the memset/cpy but before the flush. Updated.
| const void* wh_Utils_CacheInvalidate(const void* p, size_t n) | ||
| { | ||
| int len = (int)n; | ||
| const uint8_t* ptr = (const uint8_t*)p; | ||
| do { | ||
| XCACHEINVLD(ptr); | ||
| ptr += XCACHELINE; | ||
| len -= XCACHELINE; | ||
| } while (len > 0); | ||
| return p; | ||
| } | ||
|
|
||
| void* wh_Utils_CacheFlush(void* p, size_t n) | ||
| { | ||
| int len = (int)n; | ||
| uint8_t* ptr = (uint8_t*)p; | ||
| do { | ||
| XCACHEFLUSH(ptr); | ||
| ptr += XCACHELINE; | ||
| len -= XCACHELINE; | ||
| } while (len > 0); | ||
| return p; | ||
| } |
There was a problem hiding this comment.
I wonder if this should be a callback function left up to the port.... For example TC3xx lets you flush/invalidate the entire cache, a wordline, or a block, so that should be selectable based on the length passed in. But the HSM cache is totally different, so would use a different implementation.
There was a problem hiding this comment.
good call. updated.
src/wh_utils.c
Outdated
|
|
||
| void* wh_Utils_memcpy_invalidate(void* dst, const void* src, size_t n) | ||
| { | ||
| return memcpy(dst,wh_Utils_CacheInvalidate(src,n),n); |
src/wh_utils.c
Outdated
|
|
||
| void* wh_Utils_memcpy_flush(void* dst, const void* src , size_t n) | ||
| { | ||
| return wh_Utils_CacheFlush(memcpy(dst,src,n),n); |
src/wh_utils.c
Outdated
| #endif | ||
|
|
||
| #ifdef DEBUG_CRYPTOCB_VERBOSE | ||
| void _hexdump(const char* initial, uint8_t* ptr, size_t size) |
There was a problem hiding this comment.
no longer static, so might as well rename to wh_Utils_Hexdump
wolfhsm/wh_utils.h
Outdated
| /** Cache flushing and memory fencing synchronization primitives */ | ||
| /* Create a full sequential memory fence to ensure compiler memory ordering */ | ||
| #ifndef XMEMFENCE | ||
| #define XMEMFENCE() __atomic_thread_fence(__ATOMIC_SEQ_CST) |
There was a problem hiding this comment.
This is GCC specific - either needs to have a tree of compiler-specific #ifdefs. Also consider moving to wh_settings.h with a non-compiler-specific default value with the expectation it is overridden by ports in wolfhsm_config.h?
There was a problem hiding this comment.
Agree. This one is good for gcc's and clang and probably others too. Moved to settings.h
wolfhsm/wh_utils.h
Outdated
| #define XCACHELINE (32) | ||
| #endif | ||
|
|
||
| /* Flush the cache line at _p. Used after writing to ensure the memory is | ||
| * consistent. */ | ||
| #ifndef XCACHEFLUSH | ||
| #define XCACHEFLUSH(_p) (void)(_p) | ||
| /* PPC32: __asm__ volatile ("dcbf 0, %0" : : "r" (_p): "memory") */ | ||
| #endif | ||
|
|
||
| /* Invalidate the cache line at _p. Used prior to reading to ensure | ||
| * freshness. */ | ||
| #ifndef XCACHEINVLD | ||
| #define XCACHEINVLD(_p) (void)(_p) | ||
| /* PPC32: __asm__ volatile ("dcbi 0, %0" : : "r" (_p): "memory") */ | ||
| #endif |
There was a problem hiding this comment.
same thing as above for all these - we should discuss if they should live in wh_settings.h and document the best way for ports to define
There was a problem hiding this comment.
agreed. Moved already.
|
I did not test on the SPC. Maybe I'll fire that up and make sure I didn't break anything. I'll undo the custom transport and use this instead. |
Fix memtransport, move debug prints to utils
First stage of a sequence of fixes. This one adds cache flush/mem fencing support to mem transport, stolen from the Bernina private repo, which corrects the surprisingly relaxed memory consistency on Apple silicon when running the sim.