Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Bin Meng <bmeng.cn@gmail.com>]

On Mon, Sep 6, 2021 at 12:54 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 5 Sept 2021 at 17:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Mon, Sep 6, 2021 at 12:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Sun, 5 Sept 2021 at 16:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > {read,write}_with_attrs might be missing, and the codes currently do
> > > > not validate them before calling, which will cause segment fault.
> > > >
> > > > Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> > >
> > > This 'fixes' tag doesn't seem quite right. Before that
> > > commit 62a0db942dec, we still required that a MemoryRegionOps
> > > provided some form of both read and write.
> >
> > Did you mean before commit 62a0db942dec leaving MemoryRegionOps NULL
> > was not allowed, but things changed so that now it's possible to have
> > NULL MemoryRegionOps? If yes, then the commit id of "Fixes" should
> > probably go to the changes that allowed NULL memory ops. If not, I
> > don't think "Fixes" tag is wrong.
>
> I mean that before commit 62a0db942dec leaving the pointers all
> NULL was not allowed, and after that commit leaving the pointers all
> NULL was still not allowed. It's been a requirement that you
> provide at least one function pointer for read and one for
> write ever since the MemoryRegion APIs were introduced in 2012.
> You're proposing an API change; it might be a good one, but it
> isn't a 'Fixes' to anything.

Where is the requirement documented? I don't see anything mentioned in
docs/devel/memory.rst

If it's a requirement since 2012, then I agree "Fixes" can be dropped.
But a doc fix should be made to document the "requirement".

Regards,
Bin