Re: [Qemu-devel] RFC: memory API changes

[Paolo Bonzini]

On 23/03/2015 13:24, Peter Maydell wrote:
> (This is part of the work I'm doing for transaction attributes.)
> 
> Currently we have several APIs used for making physical
> memory accesses:
> 
> 1. cpu_physical_memory_rw &c
> 
> 2. address_space_rw/read/write/map
> 
> 3. ld/st*_phys
> 
> These do more-or-less overlapping jobs and it's not
> obvious which should be used when. Also they need to be
> expanded to support transaction attributes and (in some
> cases) reporting of failed memory transactions. I propose:
> 
>  * ld/st*_phys to be renamed to as_ld*, eg
>     ldub_phys -> as_ldub
>     ldl_be_phys -> as_ldl_be
>     stq_phys -> as_stq
>     stl_le_phys -> as_ldl_le

I think shorthand functions with no extra arguments still have a place.
 I was thinking of having them only temporarily, until we add functions
(e.g. pci_dma_ld or amba_ld) that deal with the MemTxResult by setting
some bus-specific abort bit.  However, this API would complicate the
case when the same core code is used for both PCI and sysbus devices.
Perhaps AddressSpaces can grow a callback that transforms a "bad"
MemTxResult to a "good" one with some side effects?

I do not like the as_ prefix, mostly because it is an English word.  It
doesn't help that ", MEMTXATTRS_UNSPECIFIED, NULL", tacked on each
ld*_phys function invocation, is way more verbose than any savings you
get from shortening the name. :)

I could be persuaded about addrspace_ as the prefix.  Hence, adding new
functions addrspace_ld* and addrspace_st* with the two extra arguments
would be fine.  Still, rather than saving four characters in the prefix,
I'd rather move the maximum line length up from 80 to 90 characters, and
actually change that to a checkpatch error.

>    and to take two new arguments:
>     MemTxAttrs attrs, MemTxResult *result
>    (existing callsites can pass MEMTXATTRS_UNSPECIFIED, NULL
>    to get their current behaviour.)
>  * address_space_rw &c to be renamed:
>     address_space_rw -> as_rw_buf
>     address_space_read -> as_read_buf
>     address_space_write -> as_write_buf
>     address_space_map -> as_map_buf &c

address_space_map doesn't map into or out of a buffer, so the name is
fine as it is.

>    This is just so the names line up nicely and we have a
>    clear indication that this is a family of functions
>  * address_space_read/write/rw should return MemTxResult rather
>    than plain bool

Certainly okay.  Same for address_space_access_valid.

>  * we should put all the as_* function prototypes in one
>    header, probably memory.h, rather than some in cpu-common.h
>    and some in memory.h

I think separating "creator" and "user" functions in two headers could
be nice.  If we cannot come up with a name for the two headers (memory.h
and mem_ldst.h?), putting both in the same is okay too.

>  * cpu_physical_memory_rw are obsolete and should be replaced
>    with uses of the as_* functions -- we should at least note
>    this in the header file. (Can't do this as an automated change
>    really because the correct AS to use is callsite dependent.)

All users that should _not_ be using address_space_memory have been
already changed to address_space_rw, or should have, so it can be done
automatically.  Same for cpu_physical_memory_map/unmap, BTW.

> The point of indicating failure via MemTxResult is that at
> some point we need to correct the current broken handling of
> the CPUClass do_unassigned_access hook, because that should
> only be invoked if the CPU itself does an access to an unassigned
> address, not if some random DMA'ing device does!

100% agreement on this.

Paolo