Re: [PATCH v3 2/9] hw/i2c: pmbus: guard against out of range accesses

[Titus Rwantare]

Ack. All errors for PMBus should
ideally be reflected in status and status_cml registers instead of
carrying meaning in return values. I'll have to separately go through
the existing code to make it consistent.


On Fri, 4 Mar 2022 at 16:08, Philippe Mathieu-Daudé
<philippe.mathieu.daude@gmail.com> wrote:
>
> On 2/3/22 02:50, Titus Rwantare wrote:
> > Signed-off-by: Titus Rwantare <titusr@google.com>
> > ---
> >   hw/i2c/pmbus_device.c | 41 ++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 40 insertions(+), 1 deletion(-)
>
> >   static uint8_t pmbus_receive_byte(SMBusDevice *smd)
> >   {
> >       PMBusDevice *pmdev = PMBUS_DEVICE(smd);
> >       PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev);
> >       uint8_t ret = 0xFF;
> > -    uint8_t index = pmdev->page;
> > +    uint8_t index;
> >
> >       if (pmdev->out_buf_len != 0) {
> >           ret = pmbus_out_buf_pop(pmdev);
> >           return ret;
> >       }
> >
> > +    /*
> > +     * Reading from all pages will return the value from page 0,
> > +     * this is unspecified behaviour in general.
> > +     */
> > +    if (pmdev->page == PB_ALL_PAGES) {
> > +        index = 0;
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: tried to read from all pages\n",
> > +                      __func__);
> > +        pmbus_cml_error(pmdev);
> > +    } else if (pmdev->page > pmdev->num_pages - 1) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: page %d is out of range\n",
> > +                      __func__, pmdev->page);
> > +        pmbus_cml_error(pmdev);
> > +        return -1;
>
> This file returns a mix of 0xFF/-1 for error. It would be nice
> to pick one. Adding a definition (PMBUS_ERR_BYTE?) could help.
>
> Preferably with error unified:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>