Re: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully
On Mon, 1 Jul 2024 at 12:18, Philippe Mathieu-Daudé wrote:
>
> Hi Zheyu,
>
> On 30/6/24 17:14, Zheyu Ma wrote:
> > This commit handles invalid address accesses gracefully in both read and
> > write
> > functions. Instead of asserting and aborting, it logs an error message and
> > returns
> > a neutral value for read operations and does nothing for write operations.
> >
> > Error log:
> > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be
> > reached
> > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code
> > should not be reached
> > Aborted
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio
> > readw 0x3f212003
>
> Thanks for this very interesting bug report (and reproducer).
>
> > EOF
> >
> > Signed-off-by: Zheyu Ma
> > ---
> > hw/misc/bcm2835_thermal.c | 12
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c
> > index ee7816b8a5..5c2a429d58 100644
> > --- a/hw/misc/bcm2835_thermal.c
> > +++ b/hw/misc/bcm2835_thermal.c
> > @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque,
> > hwaddr addr, unsigned size)
> > val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true);
> > break;
> > default:
> > -/* MemoryRegionOps are aligned, so this can not happen. */
> > -g_assert_not_reached();
>
> Like Xingtao Yao mentioned, I believe the current code is correct
> and shouldn't be reached.
>
> Why is it reached? You might have uncovered a core memory bug.
I think we get here because the bcm2835_thermal_ops MemoryRegionOps
sets impl.max_access_size and valid.max_access_size to 4, but it leaves
impl.min_access_size and valid.min_access_size unset, which means
"default to 1". So the memory system is presented with an access
of size 2 at offset 3, and it tries to synthesize it as a pair
of byte accesses at offsets 3 and 4; but the offset 3 trips
our assert above. So I think the fix is:
@@ -80,8 +80,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr,
static const MemoryRegionOps bcm2835_thermal_ops = {
.read = bcm2835_thermal_read,
.write = bcm2835_thermal_write,
+.impl.min_access_size = 4,
.impl.max_access_size = 4,
.valid.min_access_size = 4,
+.valid.max_access_size = 4,
.endianness = DEVICE_NATIVE_ENDIAN,
};
The .valid.max_access_size change is because otherwise we
fall over in a different way: the memory system tries to
synthesize a read using a 4-byte access at offset 3 even
though the device doesn't say it permits unaligned accesses.
This part does seem like a bug in the access_with_adjusted_size
code. If we want to look at that we should probably start with
this patch series from Tomoyuki Hirose from earlier this year:
https://lore.kernel.org/qemu-devel/20240201081313.1339788-1-tomoyuki.hir...@igel.co.jp/
which attempted to improve that code.
thanks
-- PMM
Re: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully
Hi Zheyu, On 30/6/24 17:14, Zheyu Ma wrote: This commit handles invalid address accesses gracefully in both read and write functions. Instead of asserting and aborting, it logs an error message and returns a neutral value for read operations and does nothing for write operations. Error log: ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Aborted Reproducer: cat << EOF | qemu-system-aarch64 -display \ none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio readw 0x3f212003 Thanks for this very interesting bug report (and reproducer). EOF Signed-off-by: Zheyu Ma --- hw/misc/bcm2835_thermal.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c index ee7816b8a5..5c2a429d58 100644 --- a/hw/misc/bcm2835_thermal.c +++ b/hw/misc/bcm2835_thermal.c @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque, hwaddr addr, unsigned size) val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true); break; default: -/* MemoryRegionOps are aligned, so this can not happen. */ -g_assert_not_reached(); Like Xingtao Yao mentioned, I believe the current code is correct and shouldn't be reached. Why is it reached? You might have uncovered a core memory bug. Likely around access_with_adjusted_size() in system/memory.c. I'll keep investigating, but so far it reminds me a previous patch from Andrew, but it isn't the fix: https://patchwork.ozlabs.org/project/qemu-devel/patch/20170630030058.28943-1-and...@aj.id.au/ +qemu_log_mask(LOG_GUEST_ERROR, + "bcm2835_thermal_read: invalid address 0x%" + HWADDR_PRIx "\n", addr); +val = 0; } return val; }
RE: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully
Hi, zheyu > -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of Zheyu > Ma > Sent: Sunday, June 30, 2024 11:14 PM > To: Peter Maydell ; Philippe Mathieu-Daudé > > Cc: Zheyu Ma ; qemu-...@nongnu.org; > qemu-devel@nongnu.org > Subject: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses > gracefully > > This commit handles invalid address accesses gracefully in both read and write > functions. Instead of asserting and aborting, it logs an error message and > returns > a neutral value for read operations and does nothing for write operations. > > Error log: > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not > be reached > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code > should not be reached > Aborted > > Reproducer: > cat << EOF | qemu-system-aarch64 -display \ > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio > readw 0x3f212003 > EOF > > Signed-off-by: Zheyu Ma > --- > hw/misc/bcm2835_thermal.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c > index ee7816b8a5..5c2a429d58 100644 > --- a/hw/misc/bcm2835_thermal.c > +++ b/hw/misc/bcm2835_thermal.c > @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque, > hwaddr addr, unsigned size) > val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true); > break; > default: > -/* MemoryRegionOps are aligned, so this can not happen. */ > -g_assert_not_reached(); > +qemu_log_mask(LOG_GUEST_ERROR, > + "bcm2835_thermal_read: invalid address 0x%" > + HWADDR_PRIx "\n", addr); > +val = 0; > } > return val; > } > @@ -72,8 +74,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr > addr, > __func__, value, addr); > break; > default: > -/* MemoryRegionOps are aligned, so this can not happen. */ > -g_assert_not_reached(); > +qemu_log_mask(LOG_GUEST_ERROR, > + "bcm2835_thermal_write: invalid address 0x%" > + HWADDR_PRIx "\n", addr); > +break; > } > } the default branch will never be reached in normal access, so I think this modification is not needed. > > -- > 2.34.1 >
[PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully
This commit handles invalid address accesses gracefully in both read and write functions. Instead of asserting and aborting, it logs an error message and returns a neutral value for read operations and does nothing for write operations. Error log: ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not be reached Aborted Reproducer: cat << EOF | qemu-system-aarch64 -display \ none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio readw 0x3f212003 EOF Signed-off-by: Zheyu Ma --- hw/misc/bcm2835_thermal.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c index ee7816b8a5..5c2a429d58 100644 --- a/hw/misc/bcm2835_thermal.c +++ b/hw/misc/bcm2835_thermal.c @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque, hwaddr addr, unsigned size) val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true); break; default: -/* MemoryRegionOps are aligned, so this can not happen. */ -g_assert_not_reached(); +qemu_log_mask(LOG_GUEST_ERROR, + "bcm2835_thermal_read: invalid address 0x%" + HWADDR_PRIx "\n", addr); +val = 0; } return val; } @@ -72,8 +74,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr addr, __func__, value, addr); break; default: -/* MemoryRegionOps are aligned, so this can not happen. */ -g_assert_not_reached(); +qemu_log_mask(LOG_GUEST_ERROR, + "bcm2835_thermal_write: invalid address 0x%" + HWADDR_PRIx "\n", addr); +break; } } -- 2.34.1
