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

[Laszlo Ersek]

On 09/12/18 14:54, Peter Maydell wrote:
> 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".

That's new to me. Has this always been the case? There are several
static MemoryRegionOps structures that don't conform. (See the end of my
other email:
<http://mid.mail-archive.com/address@hidden>.)
Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").

Are all of those ops guest-triggerable QEMU crashers?

> We could probably reasonably assert this at the point when the
> MemoryRegionOps is registered.

Apparently, we should have...

Thanks,
Laszlo