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

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

[Philippe Mathieu-Daudé]

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);
         } else if (words[0][5] == 'w') {
             uint16_t data = value;
             tswap16s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 2);
+            cpu_physical_memory_write(addr, &data, 2);
         } else if (words[0][5] == 'l') {
             uint32_t data = value;
             tswap32s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 4);
+            cpu_physical_memory_write(addr, &data, 4);
         } else if (words[0][5] == 'q') {
             uint64_t data = value;
             tswap64s(&data);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                &data, 8);
+            cpu_physical_memory_write(addr, &data, 8);
         }
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
@@ -548,22 +545,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
 
         if (words[0][4] == 'b') {
             uint8_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 1);
+            cpu_physical_memory_read(addr, &data, 1);
             value = data;
         } else if (words[0][4] == 'w') {
             uint16_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 2);
+            cpu_physical_memory_read(addr, &data, 2);
             value = tswap16(data);
         } else if (words[0][4] == 'l') {
             uint32_t data;
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &data, 4);
+            cpu_physical_memory_read(addr, &data, 4);
             value = tswap32(data);
         } else if (words[0][4] == 'q') {
-            address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                               &value, 8);
+            cpu_physical_memory_read(addr, &value, 8);
             tswap64s(&value);
         }
         qtest_send_prefix(chr);
@@ -583,9 +576,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(len);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           len);
-
+        cpu_physical_memory_read(addr, data, len);
         enc = qemu_hexdump_line(NULL, data, len, 0, 0);
 
         qtest_send_prefix(chr);
@@ -605,8 +596,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(ret == 0);
 
         data = g_malloc(len);
-        address_space_read(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                           len);
+        cpu_physical_memory_read(addr, data, len);
         b64_data = g_base64_encode(data, len);
         qtest_send_prefix(chr);
         qtest_sendf(chr, "OK %s\n", b64_data);
@@ -640,8 +630,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                 data[i] = 0;
             }
         }
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                            len);
+        cpu_physical_memory_write(addr, data, len);
         g_free(data);
 
         qtest_send_prefix(chr);
@@ -663,8 +652,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         if (len) {
             data = g_malloc(len);
             memset(data, pattern, len);
-            address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED,
-                                data, len);
+            cpu_physical_memory_write(addr, data, len);
             g_free(data);
         }
 
@@ -696,9 +684,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
                            len, out_len);
             out_len = MIN(out_len, len);
         }
-
-        address_space_write(first_cpu->as, addr, MEMTXATTRS_UNSPECIFIED, data,
-                            len);
+        cpu_physical_memory_write(addr, data, len);
 
         qtest_send_prefix(chr);
         qtest_send(chr, "OK\n");
-- 
2.45.2

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

[Richard Henderson]

On 11/27/24 13:19, Philippe Mathieu-Daudé 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);

This just calls address_space_write with &address_space_memory.
I assume we'll get rid of address_space_memory too, at some point.
But I suppose that's good enough for qtest for now.

r~

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

[Philippe Mathieu-Daudé]

On 27/11/24 23:35, Richard Henderson wrote:
> On 11/27/24 13:19, Philippe Mathieu-Daudé 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);
> 
> This just calls address_space_write with &address_space_memory.
> I assume we'll get rid of address_space_memory too, at some point.

Certainly, but one thing at a time ;) For the first prototype
I'm focusing on being able to instantiate vCPUs from different
target architecture in "any machine we have now" (even if it
doesn't make much sense). I'll then move to HW modelling, likely
starting by vCPU Clusters and then address spaces.

> But I suppose that's good enough for qtest for now.

Little win: less first_cpu accesses. What really bother me in
this file are the endianness mentions (and tswap calls).

> 
> r~
> 

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

[Fabiano Rosas]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

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

Queued, thanks!

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

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
>> 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>
>
> Queued, thanks!

Scratch that, I see some tests failing:

QTEST_QEMU_BINARY=../build/qemu-system-arm ./tests/qtest/sse-timer-test
# random seed: R02S7b4a25dac8a32a86ee016342d60e78ec
# starting QEMU: exec ../build/qemu-system-arm -qtest unix:/tmp/qtest-7386.sock -qtest-log /dev/fd/2 -chardev socket,path=/tmp/qtest-7386.qmp,id=char0 -mon chardev=char0,mode=control -display
 none -audio none -machine mps3-an547 -accel qtest
[I 0.000000] OPENED
[R +0.033524] endianness
[S +0.033533] OK little
{"QMP": {"version": {"qemu": {"micro": 93, "minor": 1, "major": 9}, "package": "v9.2.0-rc3-23-g3fe88295e8"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities"}

{"return": {}}
1..3
# Start of arm tests
# Start of sse-timer tests
[R +0.035240] writel 0x58100000 0x0
[S +0.035250] OK
[R +0.035263] writel 0x4800002c 0x0
[S +0.035265] OK
[R +0.035275] writel 0x4800004c 0x0
[S +0.035278] OK
[R +0.035287] writel 0x58100008 0x0
[S +0.035289] OK
[R +0.035296] writel 0x5810000c 0x0
[S +0.035299] OK
[R +0.035308] clock_step 3125
[S +0.035312] OK 3125
[R +0.035323] readl 0x58100008
[S +0.035326] OK 0x0000000000000000
[R +0.035334] readl 0x5810000c
[S +0.035337] OK 0x0000000000000000
[R +0.035344] writel 0x58100000 0x1
[S +0.035346] OK
[R +0.035354] clock_step 3125
[S +0.035357] OK 6250
[R +0.035364] readl 0x58100008
[S +0.035367] OK 0x0000000000000000
**
ERROR:../tests/qtest/sse-timer-test.c:91:test_counter: assertion failed (readl(COUNTER_BASE + CNTCV_LO) == 100): (0 == 100)
Bail out! ERROR:../tests/qtest/sse-timer-test.c:91:test_counter:
assertion failed (readl(COUNTER_BASE + CNTCV_LO) == 100): (0 == 100)

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

[Philippe Mathieu-Daudé]

On 9/12/24 21:34, Fabiano Rosas wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> 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>
>>
>> Queued, thanks!
> 
> Scratch that, I see some tests failing:
> 
> QTEST_QEMU_BINARY=../build/qemu-system-arm ./tests/qtest/sse-timer-test
> # random seed: R02S7b4a25dac8a32a86ee016342d60e78ec
> # starting QEMU: exec ../build/qemu-system-arm -qtest unix:/tmp/qtest-7386.sock -qtest-log /dev/fd/2 -chardev socket,path=/tmp/qtest-7386.qmp,id=char0 -mon chardev=char0,mode=control -display
>   none -audio none -machine mps3-an547 -accel qtest
> [I 0.000000] OPENED
> [R +0.033524] endianness
> [S +0.033533] OK little
> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 1, "major": 9}, "package": "v9.2.0-rc3-23-g3fe88295e8"}, "capabilities": ["oob"]}}
> {"execute": "qmp_capabilities"}
> 
> {"return": {}}
> 1..3
> # Start of arm tests
> # Start of sse-timer tests
> [R +0.035240] writel 0x58100000 0x0
> [S +0.035250] OK
> [R +0.035263] writel 0x4800002c 0x0
> [S +0.035265] OK
> [R +0.035275] writel 0x4800004c 0x0
> [S +0.035278] OK
> [R +0.035287] writel 0x58100008 0x0
> [S +0.035289] OK
> [R +0.035296] writel 0x5810000c 0x0
> [S +0.035299] OK
> [R +0.035308] clock_step 3125
> [S +0.035312] OK 3125
> [R +0.035323] readl 0x58100008
> [S +0.035326] OK 0x0000000000000000
> [R +0.035334] readl 0x5810000c
> [S +0.035337] OK 0x0000000000000000
> [R +0.035344] writel 0x58100000 0x1
> [S +0.035346] OK
> [R +0.035354] clock_step 3125
> [S +0.035357] OK 6250
> [R +0.035364] readl 0x58100008
> [S +0.035367] OK 0x0000000000000000
> **
> ERROR:../tests/qtest/sse-timer-test.c:91:test_counter: assertion failed (readl(COUNTER_BASE + CNTCV_LO) == 100): (0 == 100)
> Bail out! ERROR:../tests/qtest/sse-timer-test.c:91:test_counter:
> assertion failed (readl(COUNTER_BASE + CNTCV_LO) == 100): (0 == 100)

Odd, this should be a no-op. I'll have a look. Thanks for testing!

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

[Peter Maydell]

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.

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.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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?

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

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.

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