Re: [PATCH 4/4] hw/nvram/eeprom_at24c: Reorganise init to avoid overwriting values

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 3/3/25 13:40, Philippe Mathieu-Daudé wrote:
> On 3/3/25 13:32, BALATON Zoltan wrote:
>> On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> On 1/3/25 15:35, BALATON Zoltan wrote:
>>>> The init_rom can write values to the beginning of the memory but these
>>>> are overwritten by values from a backing file that covers the whole
>>>> memory. Do the init_rom handling only if it would not be overwritten.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/nvram/eeprom_at24c.c | 6 ++----
>>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
>>>> index 78c81bea77..ff7a21eee7 100644
>>>> --- a/hw/nvram/eeprom_at24c.c
>>>> +++ b/hw/nvram/eeprom_at24c.c
>>>> @@ -191,10 +191,6 @@ static void at24c_eeprom_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>         ee->mem = g_malloc0(ee->rsize);
>>>>   -    if (ee->init_rom) {
>>>> -        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee- 
>>>> >rsize));
>>>> -    }
>>>> -
>>>>       if (ee->blk) {
>>>>           int ret = blk_pread(ee->blk, 0, ee->rsize, ee->mem, 0);
>>>>   @@ -204,6 +200,8 @@ static void at24c_eeprom_realize(DeviceState 
>>>> *dev, Error **errp)
>>>>               return;
>>>>           }
>>>>           DPRINTK("Reset read backing file\n");
>>>> +    } else if (ee->init_rom) {
>>>
>>> Don't you want to keep overwritting the init_rom[] buffer?
>>>
>>> IOW why not s/else//?
>>
>> I've tried to explain that in the commit message. Current behaviour is 
>> to use backing file content overwriting init_rom content. Removing 
>> else here would change that and init_rom would overwrite data read 
>> from backing file. I think normally 
> 
> OK, I'll amend in description:
> 
> ---
>> init_rom is used only if there's no backing file (provides default 
>> content) but should not overwrite backing file content (especially 
>> leaving the file unchanged and only change it in memory).
> ---

Reworded as:

---
The init_rom[] can write values to the beginning of the memory but
these are overwritten by values from a backing file that covers the
whole memory.

init_rom[] is used only if there's no backing file (provides default
content) but should not overwrite backing file content (especially
leaving the file unchanged and only change it in memory).
Do the init_rom[] handling only if it would not be overwritten.
---

> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> So I don't see why would that be useful.
>>
>> Regards,
>> BALATON Zoltan
>>
>>>> +        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee- 
>>>> >rsize));
>>>>       }
>>>>         /*
>>>
>>>
>