On Fri, 29 Aug 2025 at 09:23, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Hi Titus,
>
> On 28/8/25 20:49, Ben Hillier wrote:
> > From: Titus Rwantare <tit...@google.com>
> >
> > This patch fixes a bug that causes a kernel panic when calling
> > ADM1266_BLACKBOX_INFORMATION.
>
> But you also implement BLACKBOX_CONFIG / READ_BLACKBOX...
>
> > Log Message:
> >
> > adm1266 5-0042: Could not read blackbox!
> > Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in:
> > adm1266_nvmem_read+0x168/0x170
> >
> > Signed-off-by: Titus Rwantare <tit...@google.com>
> > ---
> > hw/sensor/adm1266.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/hw/sensor/adm1266.c b/hw/sensor/adm1266.c
> > index 9017ce6116..cd11526c2b 100644
> > --- a/hw/sensor/adm1266.c
> > +++ b/hw/sensor/adm1266.c
> > @@ -75,6 +75,7 @@ typedef struct ADM1266State {
> > static const uint8_t adm1266_ic_device_id[] = {0x03, 0x41, 0x12, 0x66};
> > static const uint8_t adm1266_ic_device_rev[] = {0x08, 0x01, 0x08, 0x07,
> > 0x0,
> > 0x0, 0x07, 0x41, 0x30};
> > +static const uint8_t adm1266_blackbox_info[] = {0x04, 0x00, 0x00, 0x00,
> > 0x00};
> {
> 0x04, /* length */
> 0x00, 0x00, /* latest ID */
> 0x00, /* latest record logic index */
> 0x00 /* latest record count */
> };
>
> >
> > static void adm1266_exit_reset(Object *obj, ResetType type)
> > {
> > @@ -125,6 +126,17 @@ static uint8_t adm1266_read_byte(PMBusDevice *pmdev)
> > pmbus_send(pmdev, adm1266_ic_device_rev,
> > sizeof(adm1266_ic_device_rev));
> > break;
> >
> > + case ADM1266_BLACKBOX_CONFIG:
> > + return 0;
>
> I'd was expecting pmbus_send(pmdev, "\0\0", 2) for CYCLIC_RECORD_DISABLED.
>
> > +
> > + case ADM1266_READ_BLACKBOX:
>
> Here I'm confused, as len+data is 65bytes. Is the guest
> reading a stream of 0xff?
>
> > + return 0;
> > +
> > + case ADM1266_BLACKBOX_INFORMATION:
> > + pmbus_send(pmdev, adm1266_blackbox_info,
> > + sizeof(adm1266_blackbox_info));
> > + break;
>
> OK.
>
> > default:
> > qemu_log_mask(LOG_UNIMP,
> > "%s: reading from unimplemented register: 0x%02x\n",
> > @@ -154,6 +166,9 @@ static int adm1266_write_data(PMBusDevice *pmdev, const
> > uint8_t *buf,
> > pmbus_receive_block(pmdev, (uint8_t *)s->mfr_rev,
> > sizeof(s->mfr_rev));
> > break;
> >
> > + case ADM1266_BLACKBOX_CONFIG:
> Do we want to check, if lduw_p(buf) != 0 -> log(UNIMP)?
>
> > + case ADM1266_READ_BLACKBOX:
>
> Do we want to check, if (buf[0] != 1) -> log(GUEST_ERROR),
> and log(UNIMP) regardless?
>
> > + case ADM1266_BLACKBOX_INFORMATION:
>
> log(GUEST_ERROR);
>
> > case ADM1266_SET_RTC: /* do nothing */
> > break;
> >
> Regards,
>
> Phil.
Thanks for the feedback, I wound up implementing more of the blackbox
and implementing a test for it.
-Titus
