Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] 32-bit platform ABI mismatch in shared telemetry buffer layout due to missing struct padding, leading to memory corruption. - [Medium] RUSD telemetry structures use native integer types, masking endianness mismatches on big-endian architectures. --
--- Patch [1]: [PATCH 1/6] drm/nouveau/gsp: vendor the RUSD header --- commit c1a3b8ad153588da7ba08ef117defa77f7ba5f2d Author: Mohamed Ahmed <[email protected]> drm/nouveau/gsp: vendor the RUSD header RM User Shared Data (RUSD) is GSP-RM's telemetry channel: the kernel hands the firmware a buffer and the firmware fills it with telemetry data (temperatures, power draw, power limits, clocks, utilizations, etc) on its own timer. This commit vendors the ABI needed to use it from the 570.144 tag of NVIDIA's open-gpu-kernel-modules. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/rusd.h > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/rusd.h > new file mode 100644 > index 0000000000000..9f1099a064301 > --- /dev/null > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/nvrm/rusd.h [ ... ] > +typedef enum RUSD_TEMPERATURE_TYPE { > + RUSD_TEMPERATURE_TYPE_GPU, > + RUSD_TEMPERATURE_TYPE_HBM, > + RUSD_TEMPERATURE_TYPE_MAX > +} RUSD_TEMPERATURE_TYPE; > + > +typedef struct RUSD_TEMPERATURE { > + volatile NvU64 lastModifiedTimestamp; > + NvTemp temperature; > +} RUSD_TEMPERATURE; [Severity: High] Does this struct lack explicit padding, causing an ABI mismatch on 32-bit platforms? On 32-bit platforms (like x86-32), NvU64 has a 4-byte natural alignment, making sizeof(RUSD_TEMPERATURE) exactly 12 bytes. On 64-bit systems and the 64-bit RISC-V GSP firmware, it requires padding and is 16 bytes. [ ... ] > +typedef struct NV00DE_SHARED_DATA { > + NV_DECLARE_ALIGNED(RUSD_BAR1_MEMORY_INFO bar1MemoryInfo, 8); [Severity: Medium] Will these fields be read incorrectly on big-endian architectures? The shared data structures define fields with native integer types (NvU32, NvU64) instead of explicit little-endian types (__le32, __le64). On a big-endian host, the natively-read values will be byte-swapped versions of the little-endian data written by the RISC-V GSP firmware. [ ... ] > + // POLL_POWER > + // Module Limit is not supported on Ampere/Hopper > + NV_DECLARE_ALIGNED(RUSD_POWER_LIMITS powerLimitGpu, 8); > + > + // POLL_THERMAL > + NV_DECLARE_ALIGNED(RUSD_TEMPERATURE > temperatures[RUSD_TEMPERATURE_TYPE_MAX], 8); [Severity: High] Will the missing padding in RUSD_TEMPERATURE cause memory corruption here? Because temperatures is declared as an array of 3 elements, the array occupies 36 bytes on 32-bit versus 48 bytes on 64-bit. This difference in array stride shifts all subsequent fields by 8 bytes. The kernel will allocate a smaller buffer than the firmware expects, causing the 64-bit firmware to write past the end of the allocation when writing telemetry data. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
