Re: [PATCH v3 3/5] hw/nvram/eeprom_at24c: Add init_rom field and at24c_eeprom_init_rom helper

[Peter Delevoryas]

On Wed, Jan 18, 2023 at 02:22:01AM +0000, Joel Stanley wrote:
> On Tue, 17 Jan 2023 at 23:24, Peter Delevoryas <peter@pjd.dev> wrote:
> >
> > Allows users to specify binary data to initialize an EEPROM, allowing users 
> > to
> > emulate data programmed at manufacturing time.
> 
> I like it. Is there somewhere sensible to add a description to the
> code base? Perhaps as a comment to your new function?

Actually yes, I added the comment to the wrong patch in this series. I'll
submit a v4 really quick to correct that, and include the notes you mentioned
(like the fact that we'll only copy up-to rom-size bytes from the init_rom
data).

> 
> >
> > - Added init_rom and init_rom_size attributes to TYPE_AT24C_EE
> > - Added at24c_eeprom_init_rom helper function to initialize attributes
> > - If -drive property is provided, it overrides init_rom data
> >
> > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>

Thanks for the reviews!

> 
> > ---
> >  hw/nvram/eeprom_at24c.c         | 37 ++++++++++++++++++++++++++++-----
> >  include/hw/nvram/eeprom_at24c.h |  2 ++
> >  2 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> > index 98857e3626b9..f8d751fa278d 100644
> > --- a/hw/nvram/eeprom_at24c.c
> > +++ b/hw/nvram/eeprom_at24c.c
> > @@ -50,6 +50,9 @@ struct EEPROMState {
> >      uint8_t *mem;
> >
> >      BlockBackend *blk;
> > +
> > +    const uint8_t *init_rom;
> > +    uint32_t init_rom_size;
> >  };
> >
> >  static
> > @@ -131,19 +134,38 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
> >
> >  I2CSlave *at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t 
> > rom_size)
> >  {
> > -    I2CSlave *i2c_dev = i2c_slave_new(TYPE_AT24C_EE, address);
> > -    DeviceState *dev = DEVICE(i2c_dev);
> > +    return at24c_eeprom_init_rom(bus, address, rom_size, NULL, 0);
> > +}
> > +
> > +I2CSlave *at24c_eeprom_init_rom(I2CBus *bus, uint8_t address, uint32_t 
> > rom_size,
> > +                                const uint8_t *init_rom, uint32_t 
> > init_rom_size)
> > +{
> > +    EEPROMState *s;
> > +
> > +    s = AT24C_EE(qdev_new(TYPE_AT24C_EE));
> > +
> > +    qdev_prop_set_uint8(DEVICE(s), "address", address);
> > +    qdev_prop_set_uint32(DEVICE(s), "rom-size", rom_size);
> >
> > -    qdev_prop_set_uint32(dev, "rom-size", rom_size);
> > -    i2c_slave_realize_and_unref(i2c_dev, bus, &error_abort);
> > +    /* TODO: Model init_rom with QOM properties. */
> > +    s->init_rom = init_rom;
> > +    s->init_rom_size = init_rom_size;
> >
> > -    return i2c_dev;
> > +    i2c_slave_realize_and_unref(I2C_SLAVE(s), bus, &error_abort);
> > +
> > +    return I2C_SLAVE(s);
> >  }
> >
> >  static void at24c_eeprom_realize(DeviceState *dev, Error **errp)
> >  {
> >      EEPROMState *ee = AT24C_EE(dev);
> >
> > +    if (ee->init_rom_size > ee->rsize) {
> > +        error_setg(errp, "%s: init rom is larger than rom: %u > %u",
> > +                   TYPE_AT24C_EE, ee->init_rom_size, ee->rsize);
> > +        return;
> > +    }
> > +
> >      if (ee->blk) {
> >          int64_t len = blk_getlength(ee->blk);
> >
> > @@ -163,6 +185,7 @@ static void at24c_eeprom_realize(DeviceState *dev, 
> > Error **errp)
> >      }
> >
> >      ee->mem = g_malloc0(ee->rsize);
> > +
> >  }
> >
> >  static
> > @@ -176,6 +199,10 @@ void at24c_eeprom_reset(DeviceState *state)
> >
> >      memset(ee->mem, 0, ee->rsize);
> >
> > +    if (ee->init_rom) {
> > +        memcpy(ee->mem, ee->init_rom, MIN(ee->init_rom_size, ee->rsize));
> 
> I like the MIN here; good usability feature. It's worth documenting too.
> 
> > +    }
> > +
> >      if (ee->blk) {
> >          int ret = blk_pread(ee->blk, 0, ee->rsize, ee->mem, 0);
> >
> > diff --git a/include/hw/nvram/eeprom_at24c.h 
> > b/include/hw/nvram/eeprom_at24c.h
> > index 196db309d451..5c9149331144 100644
> > --- a/include/hw/nvram/eeprom_at24c.h
> > +++ b/include/hw/nvram/eeprom_at24c.h
> > @@ -19,5 +19,7 @@
> >   * @bus, and drop the reference to it (the device is realized).
> >   */
> >  I2CSlave *at24c_eeprom_init(I2CBus *bus, uint8_t address, uint32_t 
> > rom_size);
> > +I2CSlave *at24c_eeprom_init_rom(I2CBus *bus, uint8_t address, uint32_t 
> > rom_size,
> > +                                const uint8_t *init_rom, uint32_t 
> > init_rom_size);
> >
> >  #endif
> > --
> > 2.39.0
> >