On Fri, 7 Mar 2025, Philippe Mathieu-Daudé wrote:
On 7/3/25 15:46, BALATON Zoltan wrote:
On Fri, 7 Mar 2025, Philippe Mathieu-Daudé wrote:
Hi Zoltan,
Minor review comments in case you respin (not blocking).
On 27/2/25 17:39, BALATON Zoltan wrote:
Initialise empty NVRAM with default values. This also enables IDE UDMA
mode in AmigaOS that is faster but has to be enabled in environment
due to problems with real hardware but that does not affect emulation
so we can use faster defaults here.
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
hw/ppc/amigaone.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 849c9fc6e0..5c5585d39a 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -52,6 +52,28 @@ static const char dummy_fw[] = {
#define NVRAM_ADDR 0xfd0e0000
#define NVRAM_SIZE (4 * KiB)
+static char default_env[] =
'const'
OK. Could be fixed up on merge by Nick or I can send a new version if
needed.
+ "baudrate=115200\0"
+ "stdout=vga\0"
+ "stdin=ps2kbd\0"
+ "bootcmd=boota; menu; run menuboot_cmd\0"
+ "boot1=ide\0"
+ "boot2=cdrom\0"
+ "boota_timeout=3\0"
+ "ide_doreset=on\0"
+ "pci_irqa=9\0"
+ "pci_irqa_select=level\0"
+ "pci_irqb=10\0"
+ "pci_irqb_select=level\0"
+ "pci_irqc=11\0"
+ "pci_irqc_select=level\0"
+ "pci_irqd=7\0"
+ "pci_irqd_select=level\0"
+ "a1ide_irq=1111\0"
+ "a1ide_xfer=FFFF\0";
+#define CRC32_DEFAULT_ENV 0xb5548481
+#define CRC32_ALL_ZEROS 0x603b0489
+
#define TYPE_A1_NVRAM "a1-nvram"
OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
@@ -94,7 +116,7 @@ static void nvram_realize(DeviceState *dev, Error
**errp)
{
A1NVRAMState *s = A1_NVRAM(dev);
void *p;
- uint32_t *c;
+ uint32_t crc, *c;
memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s,
"nvram",
NVRAM_SIZE, &error_fatal);
@@ -113,12 +135,25 @@ static void nvram_realize(DeviceState *dev, Error
**errp)
return;
}
}
+ crc = crc32(0, p + 4, NVRAM_SIZE - 4);
+ if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default
*/
+ *c = cpu_to_be32(CRC32_DEFAULT_ENV);
Prefer the ld/st API over cpu_to/from:
stl_be_p(c, CRC32_DEFAULT_ENV);
+ /* Also copies terminating \0 as env is terminated by \0\0 */
+ memcpy(p + 4, default_env, sizeof(default_env));
+ if (s->blk) {
+ blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p,
0);
+ }
+ return;
+ }
if (*c == 0) {
*c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
if (s->blk) {
blk_pwrite(s->blk, 0, 4, p, 0);
}
}
+ if (be32_to_cpu(*c) != crc) {
if (ldl_be_p(c) != crc) {
Why? Here we want to convert a value from host CPU endianness to a specific
endianness and vice versa in code running on the host. (We are not
accessing guest memory, we operate on the memory region pointer. The guest
is not even running yet.)
Also:
static inline int ldl_be_p(const void *ptr)
{
return be_bswap(ldl_he_p(ptr), 32);
}
static inline int ldl_he_p(const void *ptr)
{
int32_t r;
__builtin_memcpy(&r, ptr, sizeof(r));
return r;
}
#define be_bswap(v, size) glue(__builtin_bswap, size)(v)
so this is
int32_t r;
__builtin_memcpy(&r, c, sizeof(r));
This call makes the address alignment access safe.
Sometimes we use similar API doing unaligned access and static
analyzers complain [*]. Rather than maintaining 2 differents APIs
with some corner cases in one, we could always use the reliable
one.
[*] see for example commit 5814c084679 ("hw/net/virtio-net.c: Don't assume IP
length field is aligned")
That concern does not apply here as it's unlikely to have a memory region
allocated unaligned so I'd keep this until it's removed from everywhere.
We have a lot of uses of cpu_to_, _to_cpu now so it's not likely it would
go away soon. I see no advantage in introducing unneeded complexity here
to prevent something that cannot happen.
Regards,
BALATON Zoltan
__builtin_bswap32(r);
versus
static inline type endian ## size ## _to_cpu(type v)
{
return glue(endian, _bswap)(v, size);
}
which is just
__builtin_bswap32(*c);
The second one makes more sense to me and don't see why I'd want to do it
in a more cumbersome way when we end up with the same result but simpler.
Regards,
BALATON Zoltan
+ warn_report("NVRAM checksum mismatch");
+ }
}
