* [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
@ 2021-09-06 12:20 Bin Meng
2021-09-06 12:25 ` David Hildenbrand
2021-09-06 13:01 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 6+ messages in thread
From: Bin Meng @ 2021-09-06 12:20 UTC (permalink / raw)
To: David Hildenbrand, Paolo Bonzini, Peter Xu
Cc: Peter Maydell, Philippe Mathieu-Daudé, qemu-devel
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.
In addition various constraints can be supplied to control how these
callbacks are called:
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
2021-09-06 12:20 [PATCH] docs/devel: memory: Document MemoryRegionOps requirement Bin Meng
@ 2021-09-06 12:25 ` David Hildenbrand
2021-09-06 13:01 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2021-09-06 12:25 UTC (permalink / raw)
To: Bin Meng, Paolo Bonzini, Peter Xu
Cc: Peter Maydell, Philippe Mathieu-Daudé, qemu-devel
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>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
2021-09-06 12:20 [PATCH] docs/devel: memory: Document MemoryRegionOps requirement Bin Meng
2021-09-06 12:25 ` David Hildenbrand
@ 2021-09-06 13:01 ` Philippe Mathieu-Daudé
2021-09-08 18:50 ` Peter Xu
1 sibling, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-06 13:01 UTC (permalink / raw)
To: Bin Meng, Peter Maydell, David Hildenbrand, Paolo Bonzini, Peter Xu
Cc: qemu-devel
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;
---
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
2021-09-06 13:01 ` Philippe Mathieu-Daudé
@ 2021-09-08 18:50 ` Peter Xu
2021-09-08 20:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Peter Xu @ 2021-09-08 18:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Bin Meng, qemu-devel, Paolo Bonzini, David Hildenbrand
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
2021-09-08 18:50 ` Peter Xu
@ 2021-09-08 20:17 ` Philippe Mathieu-Daudé
2021-10-02 14:37 ` Bin Meng
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-08 20:17 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, Bin Meng, qemu-devel, Paolo Bonzini, David Hildenbrand
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?
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] docs/devel: memory: Document MemoryRegionOps requirement
2021-09-08 20:17 ` Philippe Mathieu-Daudé
@ 2021-10-02 14:37 ` Bin Meng
0 siblings, 0 replies; 6+ messages in thread
From: Bin Meng @ 2021-10-02 14:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Paolo Bonzini, qemu-devel@nongnu.org Developers,
Peter Xu, David Hildenbrand
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-02 14:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-06 12:20 [PATCH] docs/devel: memory: Document MemoryRegionOps requirement Bin Meng
2021-09-06 12:25 ` David Hildenbrand
2021-09-06 13:01 ` Philippe Mathieu-Daudé
2021-09-08 18:50 ` Peter Xu
2021-09-08 20:17 ` Philippe Mathieu-Daudé
2021-10-02 14:37 ` Bin Meng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.