* [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
@ 2024-06-18 13:56 Zheyu Ma
2024-06-18 14:04 ` Philippe Mathieu-Daudé
2024-06-18 18:58 ` Paul Zimmerman
0 siblings, 2 replies; 6+ messages in thread
From: Zheyu Ma @ 2024-06-18 13:56 UTC (permalink / raw)
Cc: Zheyu Ma, qemu-devel
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>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
2024-06-18 13:56 [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions Zheyu Ma
@ 2024-06-18 14:04 ` Philippe Mathieu-Daudé
2024-06-18 18:58 ` Paul Zimmerman
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-18 14:04 UTC (permalink / raw)
To: Zheyu Ma, Paul Zimmerman; +Cc: qemu-devel
Cc'ing Paul.
On 18/6/24 15:56, Zheyu Ma 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>
> ---
> 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;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
2024-06-18 13:56 [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions Zheyu Ma
2024-06-18 14:04 ` Philippe Mathieu-Daudé
@ 2024-06-18 18:58 ` Paul Zimmerman
2024-06-18 20:37 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 6+ messages in thread
From: Paul Zimmerman @ 2024-06-18 18:58 UTC (permalink / raw)
To: Zheyu Ma; +Cc: qemu-devel, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]
On Tue, Jun 18, 2024 at 6:56 AM Zheyu Ma <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>
> ---
> 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>
[-- Attachment #2: Type: text/html, Size: 2801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
2024-06-18 18:58 ` Paul Zimmerman
@ 2024-06-18 20:37 ` Philippe Mathieu-Daudé
2024-06-18 21:32 ` Paul Zimmerman
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-06-18 20:37 UTC (permalink / raw)
To: Paul Zimmerman, Zheyu Ma; +Cc: qemu-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
2024-06-18 20:37 ` Philippe Mathieu-Daudé
@ 2024-06-18 21:32 ` Paul Zimmerman
2024-06-20 10:14 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Paul Zimmerman @ 2024-06-18 21:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Zheyu Ma, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions
2024-06-18 21:32 ` Paul Zimmerman
@ 2024-06-20 10:14 ` Peter Maydell
0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2024-06-20 10:14 UTC (permalink / raw)
To: Paul Zimmerman; +Cc: Philippe Mathieu-Daudé, Zheyu Ma, qemu-devel
On Tue, 18 Jun 2024 at 22:33, Paul Zimmerman <pauldzim@gmail.com> wrote:
>
> 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).
> 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?
Yes, assert is definitely the wrong thing here. RAZ/WI and log a
guest-error is what we typically do for devices where the spec doesn't
give a behaviour for accesses to register offsets that aren't documented
as having registers.
I've applied this to target-arm.next; thanks.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-06-20 10:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-18 13:56 [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions Zheyu Ma
2024-06-18 14:04 ` Philippe Mathieu-Daudé
2024-06-18 18:58 ` Paul Zimmerman
2024-06-18 20:37 ` Philippe Mathieu-Daudé
2024-06-18 21:32 ` Paul Zimmerman
2024-06-20 10:14 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).