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
