On 5/16/20 5:33 PM, BALATON Zoltan wrote:
On Sat, 16 May 2020, Alexander Bulekov wrote:
On 200516 1513, BALATON Zoltan wrote:
According to docs bits 1 and 0 of MM_INDEX are hard coded to 0 so
unaligned access via this register should not be possible.
This also fixes problems reported in bug #1878134.
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
Hi Zoltan,
I applied this patch and confirmed that I cannot reproduce the crash
in #1878134
Thanks!
Acked-by: Alexander Bulekov <alx...@bu.edu>
Thanks, so that should be Tested-by I think but I don't care much about
tags so whatever works for me.
'Acked-by' means as a Fuzzer maintainer, Alexander checked your patch
and is happy that another maintainer (usually Gerd for hw/display/, as
ati.c doesn't have particular maintainer) takes this patch.
You are right, if Alexander tested your patch, he also should add:
Tested-by: Alexander Bulekov <alx...@bu.edu>
If a developer review your patch and agree the logic matches the
description and doesn't introduce new regressions, he might reply with a
'Reviewed-by' tag.
Note than tags are not trophies for the patch author, but are helpful
for distributions such Debian/Fedora/NetBSD/... when they backport
particular patches fixing bugs, before new QEMU (stable) version is
released.
Also they are useful in history in case a developer/maintainer goes MIA,
there is still others to contact.
Finally, there is a tag documented for bug fixes:
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message
If your patch addresses a bug in a public bug tracker, please add a line
with "Buglink: <URL-of-the-bug>" there, too.
Buglink: https://bugs.launchpad.net/qemu/+bug/1878134
Now, looking at your device implementation, it seems
1/ The device isn't supposed to have 64-bit accesses
So this might be a more generic fix to Alexander issue:
-- >8 --
@@ -879,6 +879,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
static const MemoryRegionOps ati_mm_ops = {
.read = ati_mm_read,
.write = ati_mm_write,
+ .valid.max_access_size = 4,
.endianness = DEVICE_LITTLE_ENDIAN,
};
---
2/ All the registers are 32-bit aligned
So you can simplify the implementation by letting
access_with_adjusted_size() handle the 8/16-bit accesses by using:
@@ -879,6 +879,8 @@ static void ati_mm_write(void *opaque, hwaddr addr,
static const MemoryRegionOps ati_mm_ops = {
.read = ati_mm_read,
.write = ati_mm_write,
+ .min.min_access_size = 4,
.endianness = DEVICE_LITTLE_ENDIAN,
};
Regards,
Phil.
Regards,
BALATON Zoltan
hw/display/ati.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/ati.c b/hw/display/ati.c
index f4c4542751..2ee23173b2 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -531,7 +531,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
}
switch (addr) {
case MM_INDEX:
- s->regs.mm_index = data;
+ s->regs.mm_index = data & ~3;
break;
case MM_DATA ... MM_DATA + 3:
/* indexed access to regs or memory */
--
2.21.3
