| Message ID | 20210906122020.5793-1-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | docs/devel: memory: Document MemoryRegionOps requirement | expand |
On 06.09.21 14:20, 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. Reviewed-by: David Hildenbrand <david@redhat.com>
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)); + mr->ops = ops; + } else { + mr->ops = &unassigned_mem_ops; + } +} + void memory_region_init_io(MemoryRegion *mr, Object *owner, const MemoryRegionOps *ops, @@ -1524,7 +1535,7 @@ void memory_region_init_io(MemoryRegion *mr, uint64_t size) { memory_region_init(mr, owner, name, size); - mr->ops = ops ? ops : &unassigned_mem_ops; + memory_region_set_ops(mr, ops); mr->opaque = opaque; mr->terminates = true; } @@ -1701,7 +1712,7 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, Error *err = NULL; assert(ops); memory_region_init(mr, owner, name, size); - mr->ops = ops; + memory_region_set_ops(mr, ops); mr->opaque = opaque; mr->terminates = true; mr->rom_device = true; ---
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?
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? >
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
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. In addition various constraints can be supplied to control how these callbacks are called:
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(+)