On 6/18/20 3:12 PM, Alex Bennée wrote: > > P J P <ppan...@redhat.com> writes: > >> From: Prasad J Pandit <p...@fedoraproject.org> >> >> When registering a MemoryRegionOps object, assert that its >> read/write callback methods are defined. This avoids potential >> guest crash via a NULL pointer dereference. >> >> Suggested-by: Peter Maydell <peter.mayd...@linaro.org> >> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> >> --- >> memory.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> Update v1: add assert while registering MemoryRegionOps >> -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05187.html >> >> diff --git a/memory.c b/memory.c >> index 91ceaf9fcf..6e94fd5958 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -1495,6 +1495,9 @@ void memory_region_init_io(MemoryRegion *mr, >> const char *name, >> uint64_t size) >> { >> + assert(ops); >> + assert(ops->read); >> + assert(ops->write); > > If you look at memory_region_dispatch_write you can see that > mr->ops->write being empty is acceptable because it implies > mr->ops->write_with_attrs is set instead. I think the same is true for > read so I think you need something more like: > > assert(ops->read || ops->read_with_attrs); > assert(ops->write || ops->write_with_attrs); > > >> memory_region_init(mr, owner, name, size); >> mr->ops = ops ? ops : &unassigned_mem_ops; >> mr->opaque = opaque; >> @@ -1674,6 +1677,8 @@ void >> memory_region_init_rom_device_nomigrate(MemoryRegion *mr, >> { >> Error *err = NULL; >> assert(ops); >> + assert(ops->read); >> + assert(ops->write); > > Do ROM devices need a ->write function?
This is how you put the device in I/O mode, isn't it? > > Also doesn't this break a load of running stuff without fixes for all > the various missing bits? How far does make check-acceptance get? > >> memory_region_init(mr, owner, name, size); >> mr->ops = ops; >> mr->opaque = opaque; > >
