Re: [PATCH v2 3/5] ppc/amigaone: Add default environment

[BALATON Zoltan]

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 <balaton@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));
__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");
> > +    }
> >   }