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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.