[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined |
Date: |
Thu, 18 Jun 2020 15:35:14 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
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
Re: [PATCH v1] memory: assert MemoryRegionOps callbacks are defined, no-reply, 2020/06/18