Re: [PATCH-for-10.0] system/qtest: Access memory using cpu_physical_memory_rw() API

[Peter Maydell]

On Tue, 10 Dec 2024 at 10:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 10/12/24 11:03, Peter Maydell wrote:
> > On Wed, 27 Nov 2024 at 19:20, Philippe Mathieu-Daudé <philmd@linaro.org> 
> > wrote:
> >>
> >> There is no vCPU within the QTest accelerator (well, they
> >> are stubs doing nothing, see dummy_cpu_thread_fn).
> >> Directly use the cpu_physical_memory_rw() API -- which
> >> amusingly prefixed 'cpu_' does not use vCPU -- to access
> >> memory. This reduces accesses to the global 'first_cpu'.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   system/qtest.c | 42 ++++++++++++++----------------------------
> >>   1 file changed, 14 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/system/qtest.c b/system/qtest.c
> >> index 12703a20455..a2de9a7d5a4 100644
> >> --- a/system/qtest.c
> >> +++ b/system/qtest.c
> >> @@ -18,6 +18,7 @@
> >>   #include "chardev/char-fe.h"
> >>   #include "exec/ioport.h"
> >>   #include "exec/memory.h"
> >> +#include "exec/cpu-common.h"
> >>   #include "exec/tswap.h"
> >>   #include "hw/qdev-core.h"
> >>   #include "hw/irq.h"
> >> @@ -514,23 +515,19 @@ static void qtest_process_command(CharBackend *chr, 
> >> gchar **words)
> >>
> >>           if (words[0][5] == 'b') {
> >>               uint8_t data = value;
> >> -            address_space_write(first_cpu->as, addr, 
> >> MEMTXATTRS_UNSPECIFIED,
> >> -                                &data, 1);
> >> +            cpu_physical_memory_write(addr, &data, 1);
> >
> > I'm not a huge fan of this, because cpu_physical_memory_write()
> > is one of those old APIs that it would be nice to see less
> > use of, not more. Ideally anything issuing memory transactions
> > should know what it's issuing them to, i.e. should be using
> > address_space_* functions and passing an AddressSpace.
>
> I totally agree with you. I'm chasing one problem at a time
> starting by first_cpu, and you are already seeing ahead :)
>
> Do you mind posting a documentation patch clarifying the
> cpu_physical_memory_*() methods we want to deprecate?

docs/devel/loads-stores.rst already says
"For new code they are better avoided" about all of
\<cpu_physical_memory_\(read\|write\|rw\)\>  :-)

> > If you don't want to use first_cpu, then you could use
> > address_space_write(address_space_memory, ...), which is
> > what cpu_physical_memory_write() is doing under the hood.
> > The qtest protocol assumes a single address space anyway.
>
> Correct, good idea.
>
> Next problem I have here is to understand what 'endianness'
> means for QTest framework. Use case: heterogeneous ZynqMP
> with ARM and MicroBlaze cores.

qtest itself doesn't care about endianness -- if the test
asks it to write a 32-bit value it writes a 32-bit value.
It does have an (undocumented!) "endianness" command to
which it will return either "big" or "little". On the test
side this is what determines the return value for
qtest_big_endian(). We currently use this only in the
virtio tests, where what we're trying to determine is
"are the values in the in-guest-memory virtio data
structures little endian or big endian?", so that we can
use the same test to test a virtio device in a big-endian
machine or a little-endian machine.

thanks
-- PMM