Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement

[Philippe Mathieu-Daudé]

On 9/8/21 8:50 PM, Peter Xu wrote:
> On Mon, Sep 06, 2021 at 03:01:54PM +0200, Philippe Mathieu-Daudé wrote:
>> On 9/6/21 2:20 PM, Bin Meng wrote:
>>> It's been a requirement that at least one function pointer for read
>>> and one for write are provided ever since the MemoryRegion APIs were
>>> introduced in 2012.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  docs/devel/memory.rst | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
>>> index 5dc8a12682..7b589b21d2 100644
>>> --- a/docs/devel/memory.rst
>>> +++ b/docs/devel/memory.rst
>>> @@ -344,6 +344,11 @@ based on the attributes used for the memory 
>>> transaction, or need
>>>  to be able to respond that the access should provoke a bus error
>>>  rather than completing successfully; those devices can use the
>>>  ->read_with_attrs() and ->write_with_attrs() callbacks instead.
>>> +The requirement for a device's MemoryRegionOps is that at least
>>> +one callback for read and one for write are provided. If both
>>> +->read() and ->read_with_attrs() are provided, the plain ->read()
>>> +version takes precedence over the with_attrs() version. So does
>>> +the write callback.
>>
>> What about also adding a runtime check?
>>
>> -- >8 --
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index bfedaf9c4df..8ab602d3379 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1516,6 +1516,17 @@ MemTxResult
>> memory_region_dispatch_write(MemoryRegion *mr,
>>      }
>>  }
>>
>> +static void memory_region_set_ops(MemoryRegion *mr, const
>> MemoryRegionOps *ops)
>> +{
>> +    if (ops) {
>> +        assert(ops->valid.accepts || (ops->read || ops->read_with_attrs));
>> +        assert(ops->valid.accepts || (ops->write ||
>> ops->write_with_attrs));
> 
> Curious why accepts() matters.. Say, if there's only accepts() provided and it
> returned true, then I think we still can't avoid the coredump when read/write?

Good point :(

> I'm also curious what's the issue that Paolo mentioned here:
> 
> 8da074de-7dff-6505-5180-720cf2f47c70@redhat.com/">https://lore.kernel.org/qemu-devel/8da074de-7dff-6505-5180-720cf2f47c70@redhat.com/
> 
> I believe Paolo was referring to this series from Prasad:
> 
> https://lore.kernel.org/qemu-devel/20200811114133.672647-10-ppandit@redhat.com/
> 
> We may need to solve that issue then maybe we can consider revive Prasad's
> patchset?
>