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

[Peter Xu <peterx@redhat.com>]

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?

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?

-- 
Peter Xu