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

[Havard Skinnemoen]

On Fri, Jul 3, 2020 at 3:24 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/3/20 11:07 PM, Havard Skinnemoen 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.
>
> This is not a problem until someone else try to use it. And OTP/NVRAM
> are devices currently in hype. Meanwhile we have another technical debt.
> I am just giving generic review comments.
>
> What bugs me particularly is the OTP content zeroed at reset without
> any warning. Maybe you can add something like:
>
>   qemu_log_mask(LOG_UNIMP,
>                 "Persistence not supported, OTP content erased!\n");
>
> in npcm7xx_otp_enter_reset().

Ah, but it only zeroes the registers. The OTP array is only zeroed in realize().

But I see your point about persistence. I will work on that.

One possible complication is that real boards are expected to be
programmed a certain way, so some reasonable defaults need to be
loaded when the user doesn't specify an image (and perhaps also if the
user specifies an "uninitialized" image, if there's any way to detect
that). Would it make sense to add pc-bios blobs that can be loaded by
default, and used as a starting point or backing file for users that
need persistent OTP arrays? Or maybe just OR the factory-programmed
bits into the user-provided image, since that would match the OTP
nature of those bits.

> >
> >> 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.
>
> IIUC Peter is ready to queue your series on Monday if you fix the
> license, but he usually doesn't accept devices without vmstate via
> his tree, you got lucky ;) See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg643089.html
>
> Adding the vmstate is not a hard task. If you need to respin,
> I strongly recommend you to consider adding them.

Yeah, I'll add vmstate for v4. Thanks for the blog link, that looks
like what I need.