On Fri, May 09, 2025 at 11:22:10AM -0400, Peter Xu wrote: > On Tue, May 06, 2025 at 02:21:08PM +0100, Peter Maydell wrote: > > On Mon, 5 May 2025 at 23:23, Joe Komlodi <koml...@google.com> wrote: > > > > > > address_space_write_rom_internal can take in a NULL pointer for ptr if > > > it's only doing cache flushes instead of populating the ROM. > > > > > > However, if building with --enable-ubsan, incrementing buf causes ubsan > > > to go off when doing cache flushes, since it will trigger on pointer > > > arithmetic on a NULL pointer, even if that NULL pointer doesn't get > > > dereferenced. > > > > > > To fix this, we can move the buf incrementing to only be done when > > > writing data to ROM, since that's the only point where it gets > > > dereferenced and should be non-NULL. > > > > > > Found by running: > > > qemu-system-aarch64 \ > > > -machine virt \ > > > -accel kvm > > > > > > When built with --enable-ubsan. > > > > > > Signed-off-by: Joe Komlodi <koml...@google.com> > > > --- > > > system/physmem.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/system/physmem.c b/system/physmem.c > > > index 16cf557d1a..ccd2b50da3 100644 > > > --- a/system/physmem.c > > > +++ b/system/physmem.c > > > @@ -3204,6 +3204,7 @@ static inline MemTxResult > > > address_space_write_rom_internal(AddressSpace *as, > > > case WRITE_DATA: > > > memcpy(ram_ptr, buf, l); > > > invalidate_and_set_dirty(mr, addr1, l); > > > + buf += l; > > > break; > > > > very minor, but I think the buf += l would be slightly better > > one line up, next to the memcpy(). That way we keep the > > "copy more data from buf" and "advance buf the corresponding > > amount" next to each other, rather than separating them by > > the set-dirty operation on the MR. > > > > Anyway > > Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> > > I'll adjust that when sending a PR. Thanks!
Unfortunately, this patch (with/without the touch up..) breaks ppc32 on the boot-serial test.. Reproducer: $ QTEST_QEMU_BINARY=./qemu-system-ppc ./tests/qtest/boot-serial-test -r /ppc/boot-serial/mac99 I think what happened is the ROM writer in that test can provide a "buf" pointer over some range where memory_region_supports_direct_access() can sometimes return true, but not always, in a WRITE_DATA request. In that case, "buf" needs to be properly incremented in all cases, because in that case only some part of "buf" is copied, and only copied if the guest mem is directly accessible. We'll need to increment "buf" properly even if it's looping over some range of indirect memory. I'll drop it as of now, please feel free to have a closer look.. Thanks, -- Peter Xu
