Re: [Qemu-devel] [PATCH 07/14] exec.c: Add new address_space_ld*/st* functions

[Peter Maydell]

On 9 April 2015 at 13:00, Paolo Bonzini <address@hidden> wrote:
>
>
> On 09/04/2015 13:49, Peter Maydell wrote:
>>> > I think we do not want to expose these at all (or at least, all users
>>> > should really be CPUs and hence use *_phys functions).
>>> >
>>> > S390 is always big-endian, and watch_mem_read/write can use the same
>>> > buffer trick as subpages (and in fact should probably use memattrs as 
>>> > well).
>>> >
>>> > So, please at least add a comment that these functions are deprecated,
>>> > and check if watch_mem_read/write should be handled like subpages.
>> I looked at the subpages code, and it seems to me that it's the
>> other way around -- the subpages code should use these new functions.
>> At the moment the subpage handlers use address_space_read/write
>> to pull the data into a buffer, and then use the ldl_p/stl_p functions
>> to do "read data from target-CPU order buffer into host variable".
>> It would be better for them to just directly be able to say "do
>> a ld/st in target-CPU order into this host variable", which is
>> the purpose of these new functions. Indirecting via a buffer seems
>> like an ugly workaround for not having the direct operation.
>
> Using them in subpage code is fine, but then the subpage code is in
> exec.c and can use the _internal version directly (and pass
> DEVICE_NATIVE_ENDIAN).  Still, usage of these outside exec.c is probably
> suspicious.

I use them (later in the series) in target-arm/ as well, when I
want a CPU-target-endian access which lets me specify memory
attributes.

> It's at least worth pulling these in cpu-all.h; the whole
> contents of cpu-common.h look like a sundry of functions that either are
> deprecated or should be declared elsewhere.

They need to be in memory.h because cpu-all.h doesn't have
all the typedefs. Also, memory.h seems to me clearly the
best place, since it's where we declare the MemoryRegion
and AddressSpace related functions.

It's certainly true that using target-cpu-endian functions
in a device is somewhat suspicious, but that's generally
true of all those functions, not just these new
address_space_* ones.

So I think I'm happy to add a comment that the functions
are deprecated for use outside target-*/, but I think they
are still necessary.

-- PMM