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

[Paolo Bonzini]

On 18/06/20 15:12, Alex Bennée wrote:
>>
>> @@ -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);

Also, !ops is acceptable since you have a couple lines below:

    mr->ops = ops ? ops : &unassigned_mem_ops;


>> +    assert(ops->read);
>> +    assert(ops->write);
> 
> Do ROM devices need a ->write function?

Yes, ROMD regions are treated as I/O regions for writes.  However they
don't need a read function.

> 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?

This might actually be really close with the above assertions fixed.
For example, commit 08565552f7 ("cputlb: Move NOTDIRTY handling from I/O
path to TLB path", 2019-09-25) got rid of io_mem_notdirty.

The only cases I found with "git grep" are:

- tz_ppc_dummy_ops which is broken and should just use NULL ops

- hw/nvram/nrf51_nvm.c's flash_ops which is fixed if ROMD regions are
changed not to require a read callback.

- designware_pci_host_msi_ops which is broken and should have a dummy
read callback.

Needless to say, this is something that the submitter should have done,
not the reviewer.

Paolo