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/riscv/cpu/jh7110/dram.c#L29
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").
