Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions

[Paul Zimmerman <pauldzim@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

On Tue, Jun 18, 2024 at 1:37 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi Paul,
>
> On 18/6/24 20:58, Paul Zimmerman wrote:
> > On Tue, Jun 18, 2024 at 6:56 AM Zheyu Ma <zheyuma97@gmail.com
> > <mailto:zheyuma97@gmail.com>> wrote:
> >  >
> >  > This commit modifies the dwc2_hsotg_read() and dwc2_hsotg_write()
> > functions
> >  > to handle invalid address access gracefully. Instead of using
> >  > g_assert_not_reached(), which causes the program to abort, the
> functions
> >  > now log an error message and return a default value for reads or do
> >  > nothing for writes.
> >  >
> >  > This change prevents the program from aborting and provides clear log
> >  > messages indicating when an invalid memory address is accessed.
> >  >
> >  > Reproducer:
> >  > cat << EOF | qemu-system-aarch64 -display none \
> >  > -machine accel=qtest, -m 512M -machine raspi2b -m 1G -nodefaults \
> >  > -usb -drive file=null-co://,if=none,format=raw,id=disk0 -device \
> >  > usb-storage,port=1,drive=disk0 -qtest stdio
> >  > readl 0x3f980dfb
> >  > EOF
> >  >
> >  > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com
> > <mailto:zheyuma97@gmail.com>>
> >  > ---
> >  >  hw/usb/hcd-dwc2.c | 9 +++++++--
> >  >  1 file changed, 7 insertions(+), 2 deletions(-)
> >  >
> >  > diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
> >  > index 8cac9c0a06..b4f0652c7d 100644
> >  > --- a/hw/usb/hcd-dwc2.c
> >  > +++ b/hw/usb/hcd-dwc2.c
> >  > @@ -1128,7 +1128,10 @@ static uint64_t dwc2_hsotg_read(void *ptr,
> hwaddr addr, unsigned size)
> >  >          val = dwc2_pcgreg_read(ptr, addr, (addr - HSOTG_REG(0xe00))
> >> 2, size);
> >  >          break;
> >  >      default:
> >  > -        g_assert_not_reached();
> >  > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset
> 0x%"HWADDR_PRIx"\n",
> >  > +                      __func__, addr);
> >  > +        val = 0;
> >  > +        break;
> >  >      }
> >  >
> >  >      return val;
> >  > @@ -1160,7 +1163,9 @@ static void dwc2_hsotg_write(void *ptr, hwaddr
> addr, uint64_t val,
> >  >          dwc2_pcgreg_write(ptr, addr, (addr - HSOTG_REG(0xe00)) >> 2,
> val, size);
> >  >          break;
> >  >      default:
> >  > -        g_assert_not_reached();
> >  > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset
> 0x%"HWADDR_PRIx"\n",
> >  > +                      __func__, addr);
> >  > +        break;
> >  >      }
> >  >  }
> >  >
> >  > --
> >  > 2.34.1
> >
> > Looks good to me.
> >
> > Reviewed-by: Paul Zimmerman <pauldzim@gmail.com <mailto:
> pauldzim@gmail.com>>
> >
>
> Does that mean on real HW the access to unassigned registers are
> silently ignored as RAZ/WI like this patch? (I don't have access
> to the specs -- IIRC you don't neither, but you might have real
> HW to test).


Hi Phil,

I have an old raspi around somewhere I could probably dig up and
test with, but I'm not familiar with qtest, so I don't know how I
would reproduce the failure on real hw.

Besides, isn't it always better to fail and log an error than just crash?

Regards,
Paul

[-- Attachment #2: Type: text/html, Size: 4542 bytes --]