Re: [PATCH v3 08/12] hw/nvram: NPCM7xx OTP device model

[Havard Skinnemoen]

On Fri, Jul 3, 2020 at 2:07 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
>
> On Fri, Jul 3, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 6/27/20 1:55 AM, Havard Skinnemoen wrote:
> > > +static void npcm7xx_otp_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    NPCM7xxOTPClass *oc = NPCM7XX_OTP_GET_CLASS(dev);
> > > +    NPCM7xxOTPState *s = NPCM7XX_OTP(dev);
> > > +    SysBusDevice *sbd = &s->parent;
> > > +
> > > +    s->array = g_malloc0(NPCM7XX_OTP_ARRAY_BYTES);
> >
> > The possibility to reuse persistent OTP seems important.
> > This can be added later of course.
>
> Agree, it's an important part of the behavior of the module. But it's
> not essential to be able to boot a BMC firmware image, so I left it
> out initially.
>
> > See simple example in hw/nvram/eeprom_at24c.c which use
> > a BlockBackend so the OTP content is not lost after reset
> > or migration.
>
> I'll take a look at that, thanks!
>
> > > +
> > > +    memory_region_init_io(&s->mmio, OBJECT(s), oc->mmio_ops, s, "regs",
> > > +                          NPCM7XX_OTP_REGS_SIZE);
> > > +    sysbus_init_mmio(sbd, &s->mmio);
> > > +}
> > > +
> > > +static void npcm7xx_otp_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +
> >
> > Missing migration vmstate for NPCM7xxOTPState::regs[].
>
> Ah, you're right. This is probably true for most of the peripherals in
> this series. I'll see if I can get it sorted out for the next
> iteration.

Btw, do you have any advice on how to test device migration? I'm not
planning to do actual migration of BMCs in the near future, so I'm not
sure if I'd notice if I get it wrong.