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

[Philippe Mathieu-Daudé]

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).

Thanks,

Phil.