Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined

[Philippe Mathieu-Daudé]

On 6/18/20 3:12 PM, Alex Bennée wrote:
> 
> P J P <ppandit@redhat.com> writes:
> 
>> From: Prasad J Pandit <pjp@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.maydell@linaro.org>
>> Signed-off-by: Prasad J Pandit <pjp@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;
> 
>