Re: [PATCH v2] hw/sensor: add stub for ADM1266 blackbox

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

Hi Titus,

On 28/8/25 20:49, Ben Hillier wrote:
> From: Titus Rwantare <titusr@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 <titusr@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.