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...@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-720cf2f47...@redhat.com/ > > I believe Paolo was referring to this series from Prasad: > > https://lore.kernel.org/qemu-devel/20200811114133.672647-10-ppan...@redhat.com/ > > We may need to solve that issue then maybe we can consider revive Prasad's > patchset? >