Re: [Qemu-devel] [PATCH] memory region: check the old.mmio.read status

[Laszlo Ersek]

On 09/12/18 16:28, Li Qiang wrote:
> Peter Maydell <address@hidden> 于2018年9月12日周三 下午8:55写道：
> 
>> On 12 September 2018 at 13:32, Li Qiang <address@hidden> wrote:
>>> To avoid NULL-deref for the devices without read callbacks
>>>
>>> Signed-off-by: Li Qiang <address@hidden>
>>> ---
>>>  memory.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 9b73892768..48d025426b 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -406,6 +406,10 @@ static MemTxResult
>> memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>>>  {
>>>      uint64_t tmp;
>>>
>>> +    if (!mr->ops->old_mmio.read[ctz32(size)]) {
>>> +        return MEMTX_DECODE_ERROR;
>>> +    }
>>> +
>>>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
>>>      if (mr->subpage) {
>>>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr,
>> tmp, size);
>>> --
>>> 2.11.0
>>>
>>
>> There's patches on-list which drop the old_mmio field from the MemoryRegion
>> struct entirely, so I think this patch as it stands is obsolete.
>>
>> Currently our semantics are "you must provide both read and write, even
>> if one of them just always returns 0 / does nothing / returns an error".
>> We could probably reasonably assert this at the point when the
>> MemoryRegionOps is registered.
>>
> 
> This patch is sent as the results of this thread:
> -->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01332.html
> 
> So I think  I should send a path set to add all the missing read function
> as Laszlo Ersek points
> in the above thread discussion, right?

Can we introduce a central utility function at least (for a no-op read
returning 0), and initialize the read callbacks in question with the
address of that function?

Thanks,
Laszlo