Re: [PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[BALATON Zoltan <balaton@eik.bme.hu>]

On Thu, 27 Feb 2025, Nicholas Piggin wrote:
> On Thu Feb 27, 2025 at 11:48 AM 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:
>>>> The board has a battery backed NVRAM where U-Boot environment is
>>>> stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
>>>> crashes without it having at least a valid checksum.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 113 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>>> index 4290d58613..5273543460 100644
>>>> --- a/hw/ppc/amigaone.c
>>>> +++ b/hw/ppc/amigaone.c
>>>> @@ -21,10 +21,13 @@
>>>>  #include "hw/ide/pci.h"
>>>>  #include "hw/i2c/smbus_eeprom.h"
>>>>  #include "hw/ppc/ppc.h"
>>>> +#include "system/block-backend.h"
>>>>  #include "system/qtest.h"
>>>>  #include "system/reset.h"
>>>>  #include "kvm_ppc.h"
>>>>
>>>> +#include <zlib.h> /* for crc32 */
>>>> +
>>>>  #define BUS_FREQ_HZ 100000000
>>>>
>>>>  /*
>>>> @@ -46,6 +49,103 @@ static const char dummy_fw[] = {
>>>>      0x4e, 0x80, 0x00, 0x20, /* blr */
>>>>  };
>>>>
>>>> +#define NVRAM_ADDR 0xfd0e0000
>>>> +#define NVRAM_SIZE (4 * KiB)
>>>> +
>>>> +#define TYPE_A1_NVRAM "a1-nvram"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>>> +
>>>> +struct A1NVRAMState {
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    MemoryRegion mr;
>>>> +    BlockBackend *blk;
>>>> +};
>>>> +
>>>> +/* read callback not used because of romd mode, only here just in case */
>>>
>>> Better make it g_assert_not_reached() then.
>>
>> There is a memory_region_rom_device_set_romd() function. It's not called
>> here so a read function should not be needed but it's also trivial and
>> would work if romd mode is turned off for some reason in the future so
>> adding it seems safer to me. The comment is just to note it's a romd
>> region so reads normally don't go through this function unless romd mode
>> is turned off.
>
> It's trivial to add back if you do turn it off. Adding dead code is no
> good. The memory API default won't get changed underneath you without
> proper code audit or deprecating the API so that's no problem.

OK, done in v2.

>>> Better make these addresses #defines at the top of the file with
>>> the NVRAM_ADDR?
>>
>> I don't have defines for these as these are single use constants to set up
>> memory map and with defines it's less obvious and has to be looked up
>> where these are while this way I can see it directly without having to
>> scroll up so I prefer this. I've added defines where the constant is used
>> more than once where it makes sense to keep consistency.
>
> But now you have to lok in two different places anyway because you have
> the NVRAM_ADDR etc defines at the top of the file. Seems like a good
> time to move all defines there so you can easily see the memory map in
> one place.

I don't think this makes it more readable at all but I've added a patch in 
v2 to add defines for these.

Regards,
BALATON Zoltan