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

[Bin Meng]

On Thu, Sep 9, 2021 at 4:17 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> 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:
> >
> > 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?

It looks this patch is not applied. Given it's a doc improvement for
current implementation, I think we should apply this, and future
enhancement should be done in separate series?

Regards,
Bin