From: Thomas Huth <thuth@redhat.com>

The pmbus_send_string() function contains an assert() statement that
can be triggered by the guest code when requesting too many data
without reading from the device in between. This should not be possible.
pmbus_send() already has a similar logic, but it simply ignores the
error after logging a message with qemu_log_mask(), so do the same now
in pmbus_send_string() to fix the issue.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3388
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/i2c/pmbus_device.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index b1f9843f52e..6aa608a2998 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -104,7 +104,12 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
     }
 
     size_t len = strlen(data);
-    g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
+    if (len + pmdev->out_buf_len >= SMBUS_DATA_MAX_LEN) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: requested too much data from PMBus device\n",
+                      __func__);
+        return;
+    }
     pmdev->out_buf[len + pmdev->out_buf_len] = len;
 
     for (int i = len - 1; i >= 0; i--) {
-- 
2.54.0

On Tue, 9 Jun 2026 at 11:56, Thomas Huth <thuth@redhat.com> wrote:
>
> From: Thomas Huth <thuth@redhat.com>
>
> The pmbus_send_string() function contains an assert() statement that
> can be triggered by the guest code when requesting too many data
> without reading from the device in between. This should not be possible.
> pmbus_send() already has a similar logic, but it simply ignores the
> error after logging a message with qemu_log_mask(), so do the same now
> in pmbus_send_string() to fix the issue.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3388
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/i2c/pmbus_device.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index b1f9843f52e..6aa608a2998 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -104,7 +104,12 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
>      }
>
>      size_t len = strlen(data);
> -    g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> +    if (len + pmdev->out_buf_len >= SMBUS_DATA_MAX_LEN) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: requested too much data from PMBus device\n",
> +                      __func__);
> +        return;
> +    }

I had a look at this bug earlier, but was not sure that this is the
right thing. What happens is that the guest does something that
causes the device to queue up data X ready for the guest to
read, but then instead of reading it, the guest does another
"I would like data X please" action. My guess is that the way
the hardware handles this is probably not "add the second
copy of data X after the first one". Perhaps it is "drop the
data the guest didn't read, so the next guest read gets the
second lot of data, not the first". But maybe the spec really
does say "you can do things in the order 'ask for A, ask for
B, read data for A, read data for B".

I couldn't conveniently find the pmbus spec to find out what
the hardware is supposed to do here. We need some input from
somebody who knows about pmbus.

thanks
-- PMM

On Tue, 9 Jun 2026 at 04:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Jun 2026 at 11:56, Thomas Huth <thuth@redhat.com> wrote:
> >
> > From: Thomas Huth <thuth@redhat.com>
> >
> > The pmbus_send_string() function contains an assert() statement that
> > can be triggered by the guest code when requesting too many data
> > without reading from the device in between. This should not be possible.
> > pmbus_send() already has a similar logic, but it simply ignores the
> > error after logging a message with qemu_log_mask(), so do the same now
> > in pmbus_send_string() to fix the issue.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3388
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  hw/i2c/pmbus_device.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> > index b1f9843f52e..6aa608a2998 100644
> > --- a/hw/i2c/pmbus_device.c
> > +++ b/hw/i2c/pmbus_device.c
> > @@ -104,7 +104,12 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> >      }
> >
> >      size_t len = strlen(data);
> > -    g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> > +    if (len + pmdev->out_buf_len >= SMBUS_DATA_MAX_LEN) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: requested too much data from PMBus device\n",
> > +                      __func__);
> > +        return;
> > +    }
>
> I had a look at this bug earlier, but was not sure that this is the
> right thing. What happens is that the guest does something that
> causes the device to queue up data X ready for the guest to
> read, but then instead of reading it, the guest does another
> "I would like data X please" action. My guess is that the way
> the hardware handles this is probably not "add the second
> copy of data X after the first one". Perhaps it is "drop the
> data the guest didn't read, so the next guest read gets the
> second lot of data, not the first". But maybe the spec really
> does say "you can do things in the order 'ask for A, ask for
> B, read data for A, read data for B".
>
> I couldn't conveniently find the pmbus spec to find out what
> the hardware is supposed to do here. We need some input from
> somebody who knows about pmbus.
>
> thanks
> -- PMM

Queueing reads are an implementation detail I decided on, not part of
the spec. How the data gets discarded is up to the device
manufacturer.
There's a separate issue here though that I have a fix for.
SMBUS_DATA_MAX_LEN in QEMU is for an older SMBus version and it needs
to be increased.

I can set aside some time to bring the SMBus implementation up to
date, but for now increasing it to 255 will put QEMU in line SMBus
v3.3.
-Titus

On Tue, 9 Jun 2026 at 15:52, Titus Rwantare <titusr@google.com> wrote:
>
> On Tue, 9 Jun 2026 at 04:01, Peter Maydell <peter.maydell@linaro.org> wrote:
> > I had a look at this bug earlier, but was not sure that this is the
> > right thing. What happens is that the guest does something that
> > causes the device to queue up data X ready for the guest to
> > read, but then instead of reading it, the guest does another
> > "I would like data X please" action. My guess is that the way
> > the hardware handles this is probably not "add the second
> > copy of data X after the first one". Perhaps it is "drop the
> > data the guest didn't read, so the next guest read gets the
> > second lot of data, not the first". But maybe the spec really
> > does say "you can do things in the order 'ask for A, ask for
> > B, read data for A, read data for B".
> >
> > I couldn't conveniently find the pmbus spec to find out what
> > the hardware is supposed to do here. We need some input from
> > somebody who knows about pmbus.

> Queueing reads are an implementation detail I decided on, not part of
> the spec. How the data gets discarded is up to the device
> manufacturer.

So what *does* the spec say? Does it just say that if you don't
read all the data before doing something else then it's all
unpredictable? Is there a way for the guest to get the device
back into a sane state after it's broken it like that?

-- PMM

   On Tue, 9 Jun 2026 at 08:25, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 9 Jun 2026 at 15:52, Titus Rwantare <titusr@google.com> wrote:
> >
> > On Tue, 9 Jun 2026 at 04:01, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > I had a look at this bug earlier, but was not sure that this is the
> > > right thing. What happens is that the guest does something that
> > > causes the device to queue up data X ready for the guest to
> > > read, but then instead of reading it, the guest does another
> > > "I would like data X please" action. My guess is that the way
> > > the hardware handles this is probably not "add the second
> > > copy of data X after the first one". Perhaps it is "drop the
> > > data the guest didn't read, so the next guest read gets the
> > > second lot of data, not the first". But maybe the spec really
> > > does say "you can do things in the order 'ask for A, ask for
> > > B, read data for A, read data for B".
> > >
> > > I couldn't conveniently find the pmbus spec to find out what
> > > the hardware is supposed to do here. We need some input from
> > > somebody who knows about pmbus.
>
> > Queueing reads are an implementation detail I decided on, not part of
> > the spec. How the data gets discarded is up to the device
> > manufacturer.
>
> So what *does* the spec say? Does it just say that if you don't
> read all the data before doing something else then it's all
> unpredictable? Is there a way for the guest to get the device
> back into a sane state after it's broken it like that?
>
> -- PMM

I'm not sure this can happen on hardware. The device puts all the
bytes on the bus, and the controller can interrupt it with a Repeated
START

SMBus v3.3 6.5:
```
The data formats implemented by SMBus are:
• Controller-transmitter transmits to target-receiver: The transfer
direction in this case is not changed.
• Controller reads target immediately after the first byte: At the
moment of the first acknowledgment (provided by the target-receiver)
the controller-transmitter becomes a controller-receiver and the
target-receiver becomes a target-transmitter.
• Combined format: During a change of direction within a transfer, the
controller generates a REPEATED START condition and the target address
but with the R/W#
set to 1. In this case the controller receiver terminates the transfer
by generating a NACK on the last byte of the transfer and a STOP
condition.
```
Hmm, seems to me the data gets cleared with a change in direction.
-Titus

oops, the patch series that fixes this needs to be upstreamed.

-Titus