On 4/27/23 7:25 PM, Yanhong Wang wrote:
+struct starfive_eeprom {
- struct eeprom_header header;
- struct starfive_eeprom_atom1 atom1;
- struct starfive_eeprom_atom4 atom4;
+};
+static uchar eeprom_buf[STARFIVE_EEPROM_HATS_SIZE_MAX];
+struct starfive_eeprom *pbuf = (struct starfive_eeprom *)eeprom_buf;
+/* Set to 1 if we've read EEPROM into memory */ +static int has_been_read;
`pbuf` is only used as a const pointer variable, so why not just define pbuf as the buffer? Something like:
static union { struct starfive_eeprom { struct eeprom_header header; struct starfive_eeprom_atom1 atom1; struct starfive_eeprom_atom4 atom4; }; uchar eeprom_buf[STARFIVE_EEPROM_HATS_SIZE_MAX]; } pbuf;
Also all global variables in this file should have __section(".data") attribute. This is because these uninitialized variables will by default get linked in .bss section. When calling this function early, before .bss gets initialized, we could get random data, or corrupt data that's not yet relocated from .bss region. This problem is more obvious when we have CONFIG_OF_SEPARATE=y. When set, device-tree is expected to get loaded by previous stage, and it's expected to be loaded after _end, which is before __bss_start. In general, we should not touch anything in .bss (either read/write) before `board_init_f` completes.
I'm pretty confident on this because I've got a VF2 4GB board, and I hacked opensbi/u-boot to detect this kind of issue using PMP. I was using https://github.com/starfive-tech/u-boot/tree/JH7110_VisionFive2_devel. The eeprom implementation there were roughly the same as this patch. I was able to observe PMP access violation fault when I only allow R/X permission to u-boot image region and R/W to u-boot stack. The fault was originating from https://github.com/starfive-tech/u-boot/blob/JH7110_VisionFive2_devel/arch/r... Although this patch is different than the github version, I still think we must avoid the same kind of problem. Many other board/drivers are already doing __section(".data").