[Qemu-devel] [RFC] docs/devel/loads-stores.rst: Document our various load and store APIs

Peter Maydell posted 1 patch 6 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1502189042-9676-1-git-send-email-peter.maydell@linaro.org
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
docs/devel/loads-stores.rst | 365 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 365 insertions(+)
create mode 100644 docs/devel/loads-stores.rst
[Qemu-devel] [RFC] docs/devel/loads-stores.rst: Document our various load and store APIs
Posted by Peter Maydell 6 years, 8 months ago
QEMU has a wide selection of different functions for doing
loads and stores; provide some overview documentation of
what they do and how to pick which one to use.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is currently a work in progress, with many TODO points.
Some of the formatting is a bit inconsistent from section to
section. I post it to see if people think it's a useful
document to have and to try to get some answers to the TODO
points which are "I don't know why this function exists" and
"when should we use this function rather than that other one"...

 docs/devel/loads-stores.rst | 365 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 365 insertions(+)
 create mode 100644 docs/devel/loads-stores.rst

diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
new file mode 100644
index 0000000..73ae4c8
--- /dev/null
+++ b/docs/devel/loads-stores.rst
@@ -0,0 +1,365 @@
+..
+   Copyright (c) 2017 Linaro Limited
+   Written by Peter Maydell
+   TODO: do we have a standard doc license? bitmaps.rst
+   uses the FreeBSD documentation license, I notice.
+
+===================
+Load and Store APIs
+===================
+
+QEMU internally has multiple families of functions for performing
+loads and stores. This document attempts to enumerate them all
+and indicate when to use them. It does not provide detailed
+documentation of each API -- for that you should look at the
+documentation comments in the relevant header files.
+
+
+``ld*_p and st_*p``
+~~~~~~~~~~~~~~~~~~~
+
+These functions operate on a host pointer, and should be used
+when you already have a pointer into host memory (corresponding
+to guest ram or a local buffer). They deal with doing accesses
+with the desired endianness and with correctly handling
+potentially unaligned pointer values.
+
+Function names follow the pattern:
+
+load: ``ld{type}{sign}{size}_{endian}_p(ptr)``
+
+store: ``st{type}{size}_{endian}_p(ptr, val)``
+
+``type``
+ - (empty) : integer access
+ - ``f`` : float access
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes (including floats and doubles)
+ - ``u`` : unsigned
+ - ``s`` : signed
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``endian``
+ - ``he`` : host endian
+ - ``be`` : big endian
+ - ``le`` : little endian
+
+(except for byte accesses, which have no endian infix).
+
+The ``_{endian}`` infix is omitted for target-endian accesses.
+
+The target endian accessors are only available to source
+files which are built per-target.
+
+Regexes for git grep
+ - ``\<ldf\?[us]\?[bwlq]\(_[hbl]e\)\?_p\>``
+ - ``\<stf\?[bwlq]\(_[hbl]e\)\?_p\>``
+
+``cpu_{ld,st}_*``
+~~~~~~~~~~~~~~~~~
+
+These functions operate on a guest virtual address. Be aware
+that these functions may cause a guest CPU exception to be
+taken (eg for an alignment fault or MMU fault) which will
+result in guest CPU state being updated and control longjumping
+out of the function call. They should therefore only be used
+in code that is implementing emulation of the target CPU.
+
+Function names follow the pattern:
+
+load: ``cpu_ld{sign}{size}_{mmusuffix}(env, ptr)``
+
+store: ``cpu_st{size}_{mmusuffix}(env, ptr, val)``
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes
+ - ``u`` : unsigned
+ - ``s`` : signed
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``mmusuffix`` is one of the generic suffixes ``data`` or ``code``, or
+(for softmmu configs) a target-specific MMU mode suffix as defined
+in the target's ``cpu.h``.
+
+Regexes for git grep
+ - ``\<cpu_ld[us]\?[bwlq]_[a-zA-Z0-9]\+\>``
+ - ``\<cpu_st[bwlq]_[a-zA-Z0-9]\+\>``
+
+``cpu_{ld,st}_*_ra``
+~~~~~~~~~~~~~~~~~~~~
+
+Thes functions work like the ``cpu_{ld,st}_*`` functions except
+that they also take a ``retaddr`` argument.
+
+**TODO**: when should these be used and what should ``retaddr`` be?
+
+Function names follow the pattern:
+
+load: ``cpu_ld{sign}{size}_{mmusuffix}_ra(env, ptr, retaddr)``
+
+store: ``cpu_st{sign}{size}_{mmusuffix}_ra(env, ptr, val, retaddr)``
+
+Regexes for git grep
+ - ``\<cpu_ld[us]\?[bwlq]_[a-zA-Z0-9]\+_ra\>``
+ - ``\<cpu_st[bwlq]_[a-zA-Z0-9]\+_ra\>``
+
+``helper_*_{ld,st}*mmu``
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+These functions are intended primarily to be called by the code
+generated by the TCG backend. They may also be called by target
+CPU helper function code. (XXX why, and when this vs cpu_{ld,st}_* ?)
+
+**TODO** tcg.h claims the target-endian helper_ret* are temporary and
+will go away?
+
+**TODO** why is "target endian" "ret" anyway?
+
+load: ``helper_{endian}_ld{sign}{size}_mmu(env, addr, opindex, retaddr)``
+
+load (code): ``helper_{endian}_ld{sign}{size}_cmmu(env, addr, opindex, retaddr)``
+
+store: ``helper_{endian}_st{size}_mmu(env, addr, val, opindex, retaddr)``
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes
+ - ``u`` : unsigned
+ - ``s`` : signed
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``endian``
+ - ``le`` : little endian
+ - ``be`` : big endian
+ - ``ret`` : target endianness
+
+Regexes for git grep
+ - ``\<helper_\(le\|be\|ret\)_ld[us]\?[bwlq]_c\?mmu\>``
+ - ``\<helper_\(le\|be\|ret\)_st[bwlq]_mmu\>``
+
+``address_space_*``
+~~~~~~~~~~~~~~~~~~~
+
+These functions are the primary ones to use when emulating CPU
+or device memory accesses. They take an AddressSpace, which is the
+way QEMU defines the view of memory that a device or CPU has.
+(They generally correspond to being the "master" end of a hardware bus
+or bus fabric.)
+
+Each CPU has an AddressSpace. Some kinds of CPU have more than
+one AddressSpace (for instance ARM guest CPUs have an AddressSpace
+for the Secure world and one for NonSecure if they implement TrustZone).
+Devices which can do DMA-type operations should generally have an
+AddressSpace. There is also a "system address space" which typically
+has all the devices and memory that all CPUs can see. (Some older
+device models use the "system address space" rather than properly
+modelling that they have an AddressSpace of their own.)
+
+Functions are provided for doing byte-buffer reads and writes,
+and also for doing one-data-item loads and stores.
+
+In all cases the caller provides a MemTxAttrs to specify bus
+transaction attributes, and can check whether the memory transaction
+succeeded using a MemTxResult return code.
+
+``address_space_read(address_space, addr, attrs, buf, len)``
+
+``address_space_write(address_space, addr, attrs, buf, len)``
+
+``address_space_rw(address_space, addr, attrs, buf, len, is_write)``
+
+``address_space_ld{sign}{size}_{endian}(address_space, addr, attrs, txresult)``
+
+``address_space_st{size}_{endian}(address_space, addr, val, attrs, txresult)``
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes
+ - ``u`` : unsigned
+
+(No signed load operations are provided.)
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``endian``
+ - ``le`` : little endian
+ - ``be`` : big endian
+
+The ``_{endian}`` suffix is omitted for byte accesses.
+
+Regexes for git grep
+ - ``\<address_space_\(read\|write\|rw\)\>``
+ - ``\<address_space_ldu\?[bwql]\(_[lb]e\)\?\>``
+ - ``\<address_space_st[bwql]\(_[lb]e\)\?\>``
+
+``{ld,st}*_phys``
+~~~~~~~~~~~~~~~~~
+
+These are functions which are identical to
+``address_space_{ld,st}*``, except that they always pass
+``MEMTXATTRS_UNSPECIFIED`` for the transaction attributes, and ignore
+whether the transaction succeeded or failed.
+
+The fact that they ignore whether the transaction succeeded means
+they should not be used in new code.
+
+``load: ld{sign}{size}_{endian}_phys``
+
+``store: st{size}_{endian}_phys``
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes
+ - ``u`` : unsigned
+
+(No signed load operations are provided.)
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``endian``
+ - ``le`` : little endian
+ - ``be`` : big endian
+
+The ``_{endian}_`` infix is omitted for byte accesses.
+
+Regexes for git grep
+ - ``\<ldu\?[bwlq]\(_[bl]e\)\?_phys\>``
+ - ``\<st[bwlq]\(_[bl]e\)\?_phys\>``
+
+``cpu_physical_memory_*``
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+These are convenience functions which are identical to
+``address_space_*`` but operate specifically on the system address space,
+always pass a ``MEMTXATTRS_UNSPECIFIED`` set of memory attributes and
+ignore whether the memory transaction succeeded or failed.
+For new code they are better avoided:
+
+* there is likely to be behaviour you need to model correctly for a
+  failed read or write operation
+* a device should usually perform operations on its own AddressSpace
+  rather than using the system address space
+
+``cpu_physical_memory_read``
+
+``cpu_physical_memory_write``
+
+``cpu_physical_memory_rw``
+
+Regexes for git grep
+ - ``\<cpu_physical_memory_\(read\|write\|rw\)\>``
+
+``cpu_physical_memory_write_rom``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+This function performs a write by physical address like
+``address_space_write``, except that if the write is to a ROM then
+the ROM contents will be modified, even though a write by the guest
+CPU to the ROM would be ignored.
+
+Note that unlike ``cpu_physical_memory_write()`` this function takes
+an AddressSpace argument, but unlike ``address_space_write()`` this
+function does not take a ``MemTxAttrs`` or return a ``MemTxResult``.
+
+**TODO**: we should probably clean up this inconsistency and
+turn the function into ``address_space_write_rom`` with an API
+matching ``address_space_write``.
+
+``cpu_physical_memory_write_rom``
+
+
+``cpu_memory_rw_debug``
+~~~~~~~~~~~~~~~~~~~~~~~
+
+Access CPU memory by virtual address for debug purposes.
+
+This function is intended for use by the GDB stub and similar code.
+It takes a virtual address, converts it to a physical address via
+an MMU lookup using the current settings of the specified CPU,
+and then performs the access (using ``address_space_rw`` for
+reads or ``cpu_physical_memory_write_rom`` for writes).
+This means that if the access is a write to a ROM then this
+function will modify the contents (whereas a normal guest CPU access
+would ignore the write attempt).
+
+``cpu_memory_rw_debug``
+
+``dma_memory_*``
+~~~~~~~~~~~~~~~~
+
+These behave like ``address_space_*``, except that they perform a DMA
+barrier operation first.
+
+**TODO**: I don't understand when you need to use these, and when
+you can just use the address_space functions.
+
+``dma_memory_read``
+
+``dma_memory_write``
+
+``dma_memory_rw``
+
+Regexes for git grep
+ - ``\<dma_memory_\(read\|write\|rw\)\>``
+
+``pci_dma_*`` and ``{ld,st}*_pci_dma``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+These functions are specifically for PCI device models which need to
+perform accesses where the PCI device is a bus master. You pass them a
+``PCIDevice *`` and they will do ``dma_memory_*`` operations on the
+correct address space for that device.
+
+``pci_dma_read``
+
+``pci_dma_write``
+
+``pci_dma_rw``
+
+``load: ld{sign}{size}_{endian}_pci_dma``
+
+``store: st{size}_{endian}_pci_dma``
+
+``sign``
+ - (empty) : for 32 or 64 bit sizes
+ - ``u`` : unsigned
+
+(No signed load operations are provided.)
+
+``size``
+ - ``b`` : 8 bits
+ - ``w`` : 16 bits
+ - ``l`` : 32 bits
+ - ``q`` : 64 bits
+
+``endian``
+ - ``le`` : little endian
+ - ``be`` : big endian
+
+The ``_{endian}_`` infix is omitted for byte accesses.
+
+Regexes for git grep
+ - ``\<pci_dma_\(read\|write\|rw\)\>``
+ - ``\<ldu\?[bwlq]\(_[bl]e\)\?_pci_dma\>``
+ - ``\<st[bwlq]\(_[bl]e\)\?_pci_dma\>``
-- 
2.7.4


Re: [Qemu-devel] [RFC] docs/devel/loads-stores.rst: Document our various load and store APIs
Posted by Richard Henderson 6 years, 8 months ago
On 08/08/2017 03:44 AM, Peter Maydell wrote:
> QEMU has a wide selection of different functions for doing
> loads and stores; provide some overview documentation of
> what they do and how to pick which one to use.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is currently a work in progress, with many TODO points.
> Some of the formatting is a bit inconsistent from section to
> section. I post it to see if people think it's a useful
> document to have and to try to get some answers to the TODO
> points which are "I don't know why this function exists" and
> "when should we use this function rather than that other one"...
> 
>  docs/devel/loads-stores.rst | 365 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 365 insertions(+)
>  create mode 100644 docs/devel/loads-stores.rst
> 
> diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst
> new file mode 100644
> index 0000000..73ae4c8
> --- /dev/null
> +++ b/docs/devel/loads-stores.rst
> @@ -0,0 +1,365 @@
> +..
> +   Copyright (c) 2017 Linaro Limited
> +   Written by Peter Maydell
> +   TODO: do we have a standard doc license? bitmaps.rst
> +   uses the FreeBSD documentation license, I notice.
> +
> +===================
> +Load and Store APIs
> +===================
> +
> +QEMU internally has multiple families of functions for performing
> +loads and stores. This document attempts to enumerate them all
> +and indicate when to use them. It does not provide detailed
> +documentation of each API -- for that you should look at the
> +documentation comments in the relevant header files.
> +
> +
> +``ld*_p and st_*p``
> +~~~~~~~~~~~~~~~~~~~
> +
> +These functions operate on a host pointer, and should be used
> +when you already have a pointer into host memory (corresponding
> +to guest ram or a local buffer). They deal with doing accesses
> +with the desired endianness and with correctly handling
> +potentially unaligned pointer values.
> +
> +Function names follow the pattern:
> +
> +load: ``ld{type}{sign}{size}_{endian}_p(ptr)``
> +
> +store: ``st{type}{size}_{endian}_p(ptr, val)``
> +
> +``type``
> + - (empty) : integer access
> + - ``f`` : float access
> +
> +``sign``
> + - (empty) : for 32 or 64 bit sizes (including floats and doubles)
> + - ``u`` : unsigned
> + - ``s`` : signed
> +
> +``size``
> + - ``b`` : 8 bits
> + - ``w`` : 16 bits
> + - ``l`` : 32 bits
> + - ``q`` : 64 bits
> +
> +``endian``
> + - ``he`` : host endian
> + - ``be`` : big endian
> + - ``le`` : little endian
> +
> +(except for byte accesses, which have no endian infix).
> +
> +The ``_{endian}`` infix is omitted for target-endian accesses.
> +
> +The target endian accessors are only available to source
> +files which are built per-target.
> +
> +Regexes for git grep
> + - ``\<ldf\?[us]\?[bwlq]\(_[hbl]e\)\?_p\>``
> + - ``\<stf\?[bwlq]\(_[hbl]e\)\?_p\>``
> +
> +``cpu_{ld,st}_*``
> +~~~~~~~~~~~~~~~~~
> +
> +These functions operate on a guest virtual address. Be aware
> +that these functions may cause a guest CPU exception to be
> +taken (eg for an alignment fault or MMU fault) which will
> +result in guest CPU state being updated and control longjumping
> +out of the function call. They should therefore only be used
> +in code that is implementing emulation of the target CPU.
> +
> +Function names follow the pattern:
> +
> +load: ``cpu_ld{sign}{size}_{mmusuffix}(env, ptr)``
> +
> +store: ``cpu_st{size}_{mmusuffix}(env, ptr, val)``
> +
> +``sign``
> + - (empty) : for 32 or 64 bit sizes
> + - ``u`` : unsigned
> + - ``s`` : signed
> +
> +``size``
> + - ``b`` : 8 bits
> + - ``w`` : 16 bits
> + - ``l`` : 32 bits
> + - ``q`` : 64 bits
> +
> +``mmusuffix`` is one of the generic suffixes ``data`` or ``code``, or
> +(for softmmu configs) a target-specific MMU mode suffix as defined
> +in the target's ``cpu.h``.

On memory faults, these functions will not do any unwinding.  So the cpu state
most have been saved by the translator before calling a helper that uses these.


> +
> +Regexes for git grep
> + - ``\<cpu_ld[us]\?[bwlq]_[a-zA-Z0-9]\+\>``
> + - ``\<cpu_st[bwlq]_[a-zA-Z0-9]\+\>``
> +
> +``cpu_{ld,st}_*_ra``
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Thes functions work like the ``cpu_{ld,st}_*`` functions except
> +that they also take a ``retaddr`` argument.
> +
> +**TODO**: when should these be used and what should ``retaddr`` be?

retaddr should be GETPC() called directly from a HELPER(foo), so that it
computes the return address into code_gen_buffer.  retaddr will be used to
unwind the cpu state, and so the translator need not have done any extra saving
before calling the helper.

I would say that these functions should preferentially be used with all new code.


> +
> +Function names follow the pattern:
> +
> +load: ``cpu_ld{sign}{size}_{mmusuffix}_ra(env, ptr, retaddr)``
> +
> +store: ``cpu_st{sign}{size}_{mmusuffix}_ra(env, ptr, val, retaddr)``
> +
> +Regexes for git grep
> + - ``\<cpu_ld[us]\?[bwlq]_[a-zA-Z0-9]\+_ra\>``
> + - ``\<cpu_st[bwlq]_[a-zA-Z0-9]\+_ra\>``
> +
> +``helper_*_{ld,st}*mmu``
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +These functions are intended primarily to be called by the code
> +generated by the TCG backend. They may also be called by target
> +CPU helper function code. (XXX why, and when this vs cpu_{ld,st}_* ?)

Because cpu_{ld,st}_* infer a mmu_idx from the name + cpu_mmu_index, whereas
these functions take it as a parameter.

> +
> +**TODO** tcg.h claims the target-endian helper_ret* are temporary and
> +will go away?

Heh.  Yes.  Except that the usage within the target front-end helpers got in
the way.  The tcg backends were all converted to explicit endianness, but the
front ends did want a convenient target-endian name.  I suppose we could rename
"ret" to "te" or something, or just remove the comment...

> +
> +**TODO** why is "target endian" "ret" anyway?

"ret" was added simply to indicate that these functions take a "retaddr", as
opposed to the oldest iteration of these functions that did not.  It was the
third iteration that added "le"/"be".


r~

Re: [Qemu-devel] [RFC] docs/devel/loads-stores.rst: Document our various load and store APIs
Posted by Paolo Bonzini 6 years, 8 months ago
On 08/08/2017 12:44, Peter Maydell wrote:
> +``cpu_{ld,st}_*``
> +~~~~~~~~~~~~~~~~~
> +
> +These functions operate on a guest virtual address. Be aware
> +that these functions may cause a guest CPU exception to be
> +taken (eg for an alignment fault or MMU fault) which will
> +result in guest CPU state being updated and control longjumping
> +out of the function call. They should therefore only be used
> +in code that is implementing emulation of the target CPU.
>
> +``cpu_{ld,st}_*_ra``
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Thes functions work like the ``cpu_{ld,st}_*`` functions except
> +that they also take a ``retaddr`` argument.
> +
> +**TODO**: when should these be used and what should ``retaddr`` be?

They should always be used instead of the non-ra version in helpers (or
code that is called from helpers).  retaddr is GETPC() in the helper_*
functions and in tlb_fill, and must be propagated down from there.

This means that the non-ra versions should only be called from
translation, if I understand correctly.  You can use them after
cpu_restore_state, but it seems simpler to me to just say "don't use
them at all" and leave cpu_restore_state for special cases.

> +``helper_*_{ld,st}*mmu``
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +These functions are intended primarily to be called by the code
> +generated by the TCG backend. They may also be called by target
> +CPU helper function code. (XXX why, and when this vs cpu_{ld,st}_* ?)
> +
> +**TODO** tcg.h claims the target-endian helper_ret* are temporary and
> +will go away?

It seems indeed that we have:

- helper_ret_ldub_mmu/helper_ret_ldsb_mmu that can be changed to remove
the "ret_" since there's no endianness there

- some uses in target/mips of helper_ret_{lduw,ldul,ldq,stw,stl,stq}_mmu

> +**TODO** why is "target endian" "ret" anyway?

That "_ret" is like the "_ra" at the end of cpu functions, so perhaps
these should be renamed to helper_{le,be}_{ld,st}*mmu_ra?

They should only be used if the MMU index is not a constant and is not
the active one.

> +``{ld,st}*_phys``
> +~~~~~~~~~~~~~~~~~
> +
> +These are functions which are identical to
> +``address_space_{ld,st}*``, except that they always pass
> +``MEMTXATTRS_UNSPECIFIED`` for the transaction attributes, and ignore
> +whether the transaction succeeded or failed.
> +
> +The fact that they ignore whether the transaction succeeded means
> +they should not be used in new code.

It depends---not all processors have a way of reporting failed transactions.

> +``cpu_physical_memory_*``
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +These are convenience functions which are identical to
> +``address_space_*`` but operate specifically on the system address space,
> +always pass a ``MEMTXATTRS_UNSPECIFIED`` set of memory attributes and
> +ignore whether the memory transaction succeeded or failed.
> +For new code they are better avoided:
> +
> +* there is likely to be behaviour you need to model correctly for a
> +  failed read or write operation
> +* a device should usually perform operations on its own AddressSpace
> +  rather than using the system address space

Agreed.  Maybe we should just transform them with Coccinelle.

> +``cpu_physical_memory_write_rom``
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +This function performs a write by physical address like
> +``address_space_write``, except that if the write is to a ROM then
> +the ROM contents will be modified, even though a write by the guest
> +CPU to the ROM would be ignored.
> +
> +Note that unlike ``cpu_physical_memory_write()`` this function takes
> +an AddressSpace argument, but unlike ``address_space_write()`` this
> +function does not take a ``MemTxAttrs`` or return a ``MemTxResult``.
> +
> +**TODO**: we should probably clean up this inconsistency and
> +turn the function into ``address_space_write_rom`` with an API
> +matching ``address_space_write``.

Agreed.

> +``dma_memory_*``
> +~~~~~~~~~~~~~~~~
> +
> +These behave like ``address_space_*``, except that they perform a DMA
> +barrier operation first.
> +
> +**TODO**: I don't understand when you need to use these, and when
> +you can just use the address_space functions.

I guess it depends on the memory ordering semantics of the bus you're
implementing.  It's probably better/safer to just use these instead of
address_space_*.

Paolo