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

[Philippe Mathieu-Daudé]

On 10/12/24 11:20, Philippe Mathieu-Daudé 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?

I was looking for docstring in "exec/cpu-common.h" but now see
commit b7ecba0f6f6 ("docs/devel/loads-stores.rst: Document our
various load and store APIs"):

  ``cpu_physical_memory_*``
  ~~~~~~~~~~~~~~~~~~~~~~~~~

  For new code they are better avoided:

  ``cpu_physical_memory_read``

  ``cpu_physical_memory_write``

  ``cpu_physical_memory_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.
>
> Thanks,
>
> Phil.