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

[Peter Maydell]

On 23 March 2015 at 14:39, Paolo Bonzini <address@hidden> wrote:
>
>
> 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.

The trouble is that since C doesn't do polymorphism you
then end up with awkward names for one or the other...

>  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?

So, for PCI you can have something which sets an abort bit
automatically, because the PCI spec mandates that kind of
register level exposure of transaction failures. But for AMBA
(and I guess many other buses), there's no such standardization.
The bus standard says "your transaction might fail", but what
the device actually does in that situation is up to the device
(which might ignore it, go into some lockup mode til the guest
resets it, make a note in a device-specific status register...)

For PCI, I thought the approach here was going to be that the default
background AddressSpace handlers set the abort bit and then returned
the "-1" or whatever result the spec says? In that case the
ldl functions would never return a failure result.

> 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.

OK, that's two voices against as_*. I'll leave it at
address_space_*...

>>    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.

Agreed.

>>    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.

I'm not sure there is a clean split here; I'd rather
just put them all in memory.h. We can split things up as
a separate thing (and keep the scope of this change smaller).

>>  * 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.

Hmm. Checking a few, I notice that for instance the kvm-all.c
cpu_physical_memory_rw() should probably be using cpu->as.
And the uses in the bitband read/write accessors in hw/arm/armv7m.c
should also be using a CPU address space. Most uses in devices
should really be taking a pointer to the address space to use
as a device property...

-- PMM