On Thu Feb 27, 2025 at 12:18 PM AEST, BALATON Zoltan wrote:
> On Thu, 27 Feb 2025, Nicholas Piggin wrote:
>> On Sun Feb 23, 2025 at 3:52 AM AEST, 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.
>>
>> So this overwrites a blank NVRAM file. Okay I suppose if that works.
>
> We're emulating what U-Boot does. If it does not find a valid environment
> it will overwrite with defaults.
AFAIKS u-boot provides a default environment if the CRC does not match.
If all-zeros env was created with correct CRC, IMO it should be
accepted.
Does u-boot write back a default environment or corrected CRC to NVRAM
if it was missing/bad?
> These defaults are to get the same
> behaviour and additionally to enable UDMA for IDE driver that until now
> had to be done manually to get the same speed as with pegasos2 where this
> is enabled by default. (That's because these VIA VT82C686B chips had some
> issues with DMA even in PCs but the emulated devices work so can be
> enabled and that's faster on QEMU too.)
>
>> You could have a property to supply the default environment
>> alternatively.
>
> U-Boot has the defaults hard coded. I set and use it from the same file so
> I don't see the need to send this through a property. (Properties are also
> listed in QEMU Monitor with info qtree and I don't know how a long string
> with embedded zeros would look there so I don't think that's a good idea.)
>
>> Anywhere to document this behaviour for users?
>
> I've added docs in the cover letter that I hope would end up in the
> changelog and I have a separate page for this which I'll update at
> qmiga.codeberg.page (Haven't done that yet but would do it by the
> release once this is merged.)
Okay. It would still be better to add QEMU options to docs/ files and
point the amiga pages to that rather than the other way around, since
this is QEMU specific stuff. Doc in cover letter unfortunately is
not very good since it just gets lost.
>
>>> 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 5273543460..35e4075cc3 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[] =
>>> + "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"
Hmm, the u-boot default env (before it was removed) selected
edge for these. Was that wrong?
>>> + "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)
>>>
>>> @@ -97,7 +119,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);
>>> @@ -116,12 +138,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);
>>> + /* 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) {
>>> + warn_report("NVRAM checksum mismatch");
>>> + }
>>
>> Maybe the default environment should be set if there is no CRC? If there
>> is a CRC already then that seems to indicate a valid rom file was
>> supplied and user wanted it blank.
>
> The idea is to allow supplying an environment via the backing file or let
> users edit it and clear the checksum in which case it will take the
> supplied data and only fix the checksum. Unlike U-Boot it will only
> replace empty backing file with defaults. (This is kind of undocumented
> for advanced users only but that's why the above code looks like that.)
> Having an empty NVRAM does not make much sense as U-Boot always has some
> values there so it's never empty. If it finds empty NVRAM then installs
> default env which is to help new users so they only need to create an
> empty file to get started. It also works without backing file in which
> case we want to use defaults and keep it persistent only for the session.
>
>> This can also be rewritten:
>>
>> crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>> if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>> /* Also copies terminating \0 as env is terminated by \0\0 */
>> memcpy(p + 4, default_env, sizeof(default_env));
>> crc = CRC32_DEFAULT_ENV;
>> }
>> if (*c == 0) {
>> *c = cpu_to_be32(crc);
>> if (s->blk) {
>> blk_pwrite(s->blk, 0, 4, p, 0);
>> }
>> } else if (be32_to_cpu(*c) != crc) {
>> warn_report("NVRAM checksum mismatch");
>> }
>
> I have the checksum check in separate if to warn if there is a checksum
> but it's wrong because I only want it to fix checksum if it's zero but not
> touch it otherwise. Basically it will only overwrite zero values but leave
> nonzero values there. (Might be useful for testing what does the guest do
> if the checksum is wrong, although on real machine U-Boot would have
> replaced the env with default in that case so this would only happen if
> the NVRAM is changed during runtime without updating the checksum.)
Right, you can just rewrite it a bit simpler to have only one exit point
and only do the blk_pwrite once etc I think (although my version might
have had a bug it was a bit hasty but you get the idea).
Thanks,
Nick
