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

[Paolo Bonzini]

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