From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1df39P-0001hN-H0 for qemu-devel@nongnu.org; Tue, 08 Aug 2017 07:58:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1df39K-0007xf-Ck for qemu-devel@nongnu.org; Tue, 08 Aug 2017 07:58:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52938) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1df39K-0007x4-32 for qemu-devel@nongnu.org; Tue, 08 Aug 2017 07:58:14 -0400 References: <1502189042-9676-1-git-send-email-peter.maydell@linaro.org> From: Paolo Bonzini Message-ID: Date: Tue, 8 Aug 2017 13:58:07 +0200 MIME-Version: 1.0 In-Reply-To: <1502189042-9676-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC] docs/devel/loads-stores.rst: Document our various load and store APIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: patches@linaro.org, Richard Henderson 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 transactio= ns. > +``cpu_physical_memory_*`` > +~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +These are convenience functions which are identical to > +``address_space_*`` but operate specifically on the system address spa= ce, > +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