[PATCH v3 0/3] memory: memory_region_is_mapped() cleanups

[PATCH v3 0/3] memory: memory_region_is_mapped() cleanups

[David Hildenbrand]

Playing with memory_region_is_mapped(), I realized that memory regions
mapped via an alias behave a little bit "differently", as they don't have
their ->container set.
* memory_region_is_mapped() will never succeed for memory regions mapped
  via an alias
* memory_region_to_address_space(), memory_region_find(),
  memory_region_find_rcu(), memory_region_present() won't work, which seems
  okay, because we don't expect such memory regions getting passed to these
  functions.
* memory_region_to_absolute_addr() will result in a wrong address. As
  the result is only used for tracing, that is tolerable.

Let's cleanup/fix the code and documentation of memory_region_is_mapped()
and change one user that really should be checking something else.

v1 -> v2:
- "memory: Make memory_region_is_mapped() succeed when mapped via an
   alias"
-- Add an assertion
- Add RBs

Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>

David Hildenbrand (3):
  machine: Use host_memory_backend_is_mapped() in
    machine_consume_memdev()
  memory: Make memory_region_is_mapped() succeed when mapped via an
    alias
  memory: Update description of memory_region_is_mapped()

 hw/core/machine.c     |  2 +-
 include/exec/memory.h |  4 +++-
 softmmu/memory.c      | 13 ++++++++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] machine: Use host_memory_backend_is_mapped() in machine_consume_memdev()

[David Hildenbrand]

memory_region_is_mapped() is the wrong check, we actually want to check
whether the backend is already marked mapped.

For example, memory regions mapped via an alias, such as NVDIMMs,
currently don't make memory_region_is_mapped() return "true". As the
machine is initialized before any memory devices (and thereby before
NVDIMMs are initialized), this isn't a fix but merely a cleanup.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/core/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index e24e3e27db..57c82e9473 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1081,7 +1081,7 @@ MemoryRegion *machine_consume_memdev(MachineState *machine,
 {
     MemoryRegion *ret = host_memory_backend_get_memory(backend);
 
-    if (memory_region_is_mapped(ret)) {
+    if (host_memory_backend_is_mapped(backend)) {
         error_report("memory backend %s can't be used multiple times.",
                      object_get_canonical_path_component(OBJECT(backend)));
         exit(EXIT_FAILURE);
-- 
2.31.1

[PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[David Hildenbrand]

memory_region_is_mapped() currently does not return "true" when a memory
region is mapped via an alias.

Assuming we have:
    alias (A0) -> alias (A1) -> region (R0)
Mapping A0 would currently only make memory_region_is_mapped() succeed
on A0, but not on A1 and R0.

Let's fix that by adding a "mapped_via_alias" counter to memory regions and
updating it accordingly when an alias gets (un)mapped.

I am not aware of actual issues, this is rather a cleanup to make it
consistent.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h |  1 +
 softmmu/memory.c      | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 20f1b27377..fea1a493b9 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -738,6 +738,7 @@ struct MemoryRegion {
     const MemoryRegionOps *ops;
     void *opaque;
     MemoryRegion *container;
+    int mapped_via_alias; /* Mapped via an alias, container might be NULL */
     Int128 size;
     hwaddr addr;
     void (*destructor)(MemoryRegion *mr);
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7340e19ff5..b52b6a2f66 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2535,8 +2535,13 @@ static void memory_region_add_subregion_common(MemoryRegion *mr,
                                                hwaddr offset,
                                                MemoryRegion *subregion)
 {
+    MemoryRegion *alias;
+
     assert(!subregion->container);
     subregion->container = mr;
+    for (alias = subregion->alias; alias; alias = alias->alias) {
+        alias->mapped_via_alias++;
+    }
     subregion->addr = offset;
     memory_region_update_container_subregions(subregion);
 }
@@ -2561,9 +2566,15 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion)
 {
+    MemoryRegion *alias;
+
     memory_region_transaction_begin();
     assert(subregion->container == mr);
     subregion->container = NULL;
+    for (alias = subregion->alias; alias; alias = alias->alias) {
+        alias->mapped_via_alias--;
+        assert(alias->mapped_via_alias >= 0);
+    }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
     memory_region_unref(subregion);
     memory_region_update_pending |= mr->enabled && subregion->enabled;
@@ -2660,7 +2671,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr)
 
 bool memory_region_is_mapped(MemoryRegion *mr)
 {
-    return mr->container ? true : false;
+    return !!mr->container || mr->mapped_via_alias;
 }
 
 /* Same as memory_region_find, but it does not add a reference to the
-- 
2.31.1

[PATCH v3 3/3] memory: Update description of memory_region_is_mapped()

[David Hildenbrand]

Let's update the documentation, making it clearer what the semantics
of memory_region_is_mapped() actually are.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/exec/memory.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index fea1a493b9..63be794a06 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2297,7 +2297,8 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr);
 
 /**
  * memory_region_is_mapped: returns true if #MemoryRegion is mapped
- * into any address space.
+ * into another memory region, which does not necessarily imply that it is
+ * mapped into an address space.
  *
  * @mr: a #MemoryRegion which should be checked if it's mapped
  */
-- 
2.31.1

Re: [PATCH v3 0/3] memory: memory_region_is_mapped() cleanups

[David Hildenbrand]

On 02.11.21 17:43, David Hildenbrand wrote:
> Playing with memory_region_is_mapped(), I realized that memory regions
> mapped via an alias behave a little bit "differently", as they don't have
> their ->container set.
> * memory_region_is_mapped() will never succeed for memory regions mapped
>   via an alias
> * memory_region_to_address_space(), memory_region_find(),
>   memory_region_find_rcu(), memory_region_present() won't work, which seems
>   okay, because we don't expect such memory regions getting passed to these
>   functions.
> * memory_region_to_absolute_addr() will result in a wrong address. As
>   the result is only used for tracing, that is tolerable.
> 
> Let's cleanup/fix the code and documentation of memory_region_is_mapped()
> and change one user that really should be checking something else.
> 
> v1 -> v2:
> - "memory: Make memory_region_is_mapped() succeed when mapped via an
>    alias"
> -- Add an assertion
> - Add RBs
> 
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
> 
> David Hildenbrand (3):
>   machine: Use host_memory_backend_is_mapped() in
>     machine_consume_memdev()
>   memory: Make memory_region_is_mapped() succeed when mapped via an
>     alias
>   memory: Update description of memory_region_is_mapped()
> 
>  hw/core/machine.c     |  2 +-
>  include/exec/memory.h |  4 +++-
>  softmmu/memory.c      | 13 ++++++++++++-
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 

Gentle ping

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/3] memory: memory_region_is_mapped() cleanups

[Philippe Mathieu-Daudé via]

On 11/2/21 17:43, David Hildenbrand wrote:
> Playing with memory_region_is_mapped(), I realized that memory regions
> mapped via an alias behave a little bit "differently", as they don't have
> their ->container set.
> * memory_region_is_mapped() will never succeed for memory regions mapped
>   via an alias
> * memory_region_to_address_space(), memory_region_find(),
>   memory_region_find_rcu(), memory_region_present() won't work, which seems
>   okay, because we don't expect such memory regions getting passed to these
>   functions.
> * memory_region_to_absolute_addr() will result in a wrong address. As
>   the result is only used for tracing, that is tolerable.
> 
> Let's cleanup/fix the code and documentation of memory_region_is_mapped()
> and change one user that really should be checking something else.

> David Hildenbrand (3):
>   machine: Use host_memory_backend_is_mapped() in
>     machine_consume_memdev()
>   memory: Make memory_region_is_mapped() succeed when mapped via an
>     alias
>   memory: Update description of memory_region_is_mapped()

Thanks, queued via memory-api.

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 9723 bytes --]

Hi David,

While I realize my response is quite late, I wanted to report this error I
found when running the acceptance
tests for the orangepi-pc machine using avocado:

ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
--show=app,console run -t machine:orangepi-pc
tests/avocado/boot_linux_console.py
...
 (4/5)
tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08:
-console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
\console: DRAM:
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
reached\nOriginal status: ERROR\n{'name':
'4-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08',
'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49e...
(90.64 s)
 (5/5)
tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
/console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000)
console: DRAM:
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
reached\nOriginal status: ERROR\n{'name':
'5-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9',
'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49...
(90.64 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 2 |
CANCEL 0
JOB TIME   : 221.25 s

Basically the two tests freeze during the part where the U-Boot bootloader
needs to detect the amount of memory. We model this in the
hw/misc/allwinner-h3-dramc.c file.
And when running the machine manually it shows an assert on
'alias->mapped_via_alias >= 0'. When running manually via gdb, I was able
to collect this backtrace:

$ gdb ./build/qemu-system-arm
...
gdb) run -M orangepi-pc -nographic
./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
...
U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
DRAM:
qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion:
Assertion `alias->mapped_via_alias >= 0' failed.

Thread 4 "qemu-system-arm" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffff5f72700 (LWP 32866)]
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff7535859 in __GI_abort () at abort.c:79
#2  0x00007ffff7535729 in __assert_fail_base
    (fmt=0x7ffff76cb588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
file=0x55555642f8cd "../softmmu/memory.c", line=2588, function=<optimized
out>)
    at assert.c:92
#3  0x00007ffff7546f36 in __GI___assert_fail
    (assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
file=0x55555642f8cd "../softmmu/memory.c", line=2588,
function=0x555556430690 <__PRETTY_FUNCTION__.8>
"memory_region_del_subregion") at assert.c:101
#4  0x0000555555e587e0 in memory_region_del_subregion (mr=0x555556f0bc00,
subregion=0x7ffff5fa1090) at ../softmmu/memory.c:2588
#5  0x0000555555e589f3 in memory_region_readd_subregion (mr=0x7ffff5fa1090)
at ../softmmu/memory.c:2630
#6  0x0000555555e58a5f in memory_region_set_address (mr=0x7ffff5fa1090,
addr=1090519040) at ../softmmu/memory.c:2642
#7  0x0000555555ac352b in allwinner_h3_dramc_map_rows (s=0x7ffff5fa0c50,
row_bits=16 '\020', bank_bits=2 '\002', page_size=512) at
../hw/misc/allwinner-h3-dramc.c:92
#8  0x0000555555ac36c2 in allwinner_h3_dramcom_write
(opaque=0x7ffff5fa0c50, offset=0, val=4396785, size=4) at
../hw/misc/allwinner-h3-dramc.c:131
#9  0x0000555555e52561 in memory_region_write_accessor (mr=0x7ffff5fa11a0,
addr=0, value=0x7ffff5f710e8, size=4, shift=0, mask=4294967295, attrs=...)
at ../softmmu/memory.c:492
#10 0x0000555555e527ad in access_with_adjusted_size (addr=0,
value=0x7ffff5f710e8, size=4, access_size_min=4, access_size_max=4,
access_fn=
    0x555555e52467 <memory_region_write_accessor>, mr=0x7ffff5fa11a0,
attrs=...) at ../softmmu/memory.c:554
#11 0x0000555555e55935 in memory_region_dispatch_write (mr=0x7ffff5fa11a0,
addr=0, data=4396785, op=MO_32, attrs=...) at ../softmmu/memory.c:1514
#12 0x0000555555f891ae in io_writex (env=0x7ffff5f7ce30,
iotlbentry=0x7fffec0275f0, mmu_idx=7, val=4396785, addr=29761536,
retaddr=140734938367479, op=MO_32) at ../accel/tcg/cputlb.c:1420
#13 0x0000555555f8ba10 in store_helper (env=0x7ffff5f7ce30, addr=29761536,
val=4396785, oi=3623, retaddr=140734938367479, op=MO_32) at
../accel/tcg/cputlb.c:2355
#14 0x0000555555f8bdda in full_le_stl_mmu (env=0x7ffff5f7ce30,
addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
../accel/tcg/cputlb.c:2443
#15 0x0000555555f8be16 in helper_le_stl_mmu (env=0x7ffff5f7ce30,
addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
../accel/tcg/cputlb.c:2449
#16 0x00007fff680245f7 in code_gen_buffer ()
#17 0x0000555555f754cb in cpu_tb_exec (cpu=0x7ffff5f730a0,
itb=0x7fffa802b400, tb_exit=0x7ffff5f7182c) at ../accel/tcg/cpu-exec.c:357
#18 0x0000555555f76366 in cpu_loop_exec_tb (cpu=0x7ffff5f730a0,
tb=0x7fffa802b400, last_tb=0x7ffff5f71840, tb_exit=0x7ffff5f7182c) at
../accel/tcg/cpu-exec.c:842
#19 0x0000555555f76720 in cpu_exec (cpu=0x7ffff5f730a0) at
../accel/tcg/cpu-exec.c:1001
#20 0x0000555555f993dd in tcg_cpus_exec (cpu=0x7ffff5f730a0) at
../accel/tcg/tcg-accel-ops.c:67
#21 0x0000555555f9976d in mttcg_cpu_thread_fn (arg=0x7ffff5f730a0) at
../accel/tcg/tcg-accel-ops-mttcg.c:95
#22 0x000055555624bf4d in qemu_thread_start (args=0x5555572b6780) at
../util/qemu-thread-posix.c:556
#23 0x00007ffff770b609 in start_thread (arg=<optimized out>) at
pthread_create.c:477
#24 0x00007ffff7632293 in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call
memory_region_set_address, where internally we are calling
memory_region_del_subregion.
The allwinner-h3-dramc.c file does use memory_region_add_subregion_overlap
once in the realize function, but might use the memory_region_set_address
multiple times.
It looks to me this is the path where the assert comes in. If I revert this
patch on current master, the machine boots without the assertion.

Would you be able to help out how we can best resolve this? Ofcourse, if
there is anything needed to be changed on the allwinner-h3-dramc.c file, I
would be happy to prepare a patch for that.

Kind regards,
Niek Linnenbank

On Tue, Nov 2, 2021 at 5:46 PM David Hildenbrand <david@redhat.com> wrote:

> memory_region_is_mapped() currently does not return "true" when a memory
> region is mapped via an alias.
>
> Assuming we have:
>     alias (A0) -> alias (A1) -> region (R0)
> Mapping A0 would currently only make memory_region_is_mapped() succeed
> on A0, but not on A1 and R0.
>
> Let's fix that by adding a "mapped_via_alias" counter to memory regions and
> updating it accordingly when an alias gets (un)mapped.
>
> I am not aware of actual issues, this is rather a cleanup to make it
> consistent.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/exec/memory.h |  1 +
>  softmmu/memory.c      | 13 ++++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 20f1b27377..fea1a493b9 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -738,6 +738,7 @@ struct MemoryRegion {
>      const MemoryRegionOps *ops;
>      void *opaque;
>      MemoryRegion *container;
> +    int mapped_via_alias; /* Mapped via an alias, container might be NULL
> */
>      Int128 size;
>      hwaddr addr;
>      void (*destructor)(MemoryRegion *mr);
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7340e19ff5..b52b6a2f66 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2535,8 +2535,13 @@ static void
> memory_region_add_subregion_common(MemoryRegion *mr,
>                                                 hwaddr offset,
>                                                 MemoryRegion *subregion)
>  {
> +    MemoryRegion *alias;
> +
>      assert(!subregion->container);
>      subregion->container = mr;
> +    for (alias = subregion->alias; alias; alias = alias->alias) {
> +        alias->mapped_via_alias++;
> +    }
>      subregion->addr = offset;
>      memory_region_update_container_subregions(subregion);
>  }
> @@ -2561,9 +2566,15 @@ void
> memory_region_add_subregion_overlap(MemoryRegion *mr,
>  void memory_region_del_subregion(MemoryRegion *mr,
>                                   MemoryRegion *subregion)
>  {
> +    MemoryRegion *alias;
> +
>      memory_region_transaction_begin();
>      assert(subregion->container == mr);
>      subregion->container = NULL;
> +    for (alias = subregion->alias; alias; alias = alias->alias) {
> +        alias->mapped_via_alias--;
> +        assert(alias->mapped_via_alias >= 0);
> +    }
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
>      memory_region_unref(subregion);
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
> @@ -2660,7 +2671,7 @@ static FlatRange *flatview_lookup(FlatView *view,
> AddrRange addr)
>
>  bool memory_region_is_mapped(MemoryRegion *mr)
>  {
> -    return mr->container ? true : false;
> +    return !!mr->container || mr->mapped_via_alias;
>  }
>
>  /* Same as memory_region_find, but it does not add a reference to the
> --
> 2.31.1
>
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 11230 bytes --]

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Philippe Mathieu-Daudé via]

Hi Niek!

(+Mark FYI)

On 30/1/22 23:50, Niek Linnenbank wrote:
> Hi David,
> 
> While I realize my response is quite late, I wanted to report this error 
> I found when running the acceptance
> tests for the orangepi-pc machine using avocado:

Unfortunately I only run the full SD/MMC tests when I send a SD/MMC pull
request, so missed that here.

> Basically the two tests freeze during the part where the U-Boot 
> bootloader needs to detect the amount of memory. We model this in the 
> hw/misc/allwinner-h3-dramc.c file.
> And when running the machine manually it shows an assert on 
> 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was 
> able to collect this backtrace:
> 
> $ gdb ./build/qemu-system-arm
> ...
> gdb) run -M orangepi-pc -nographic 
> ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
> ...
> U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> DRAM:
> qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion: 
> Assertion `alias->mapped_via_alias >= 0' failed.
...

> So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call 
> memory_region_set_address, where internally we are calling 
> memory_region_del_subregion.
> The allwinner-h3-dramc.c file does use 
> memory_region_add_subregion_overlap once in the realize function, but 
> might use the memory_region_set_address multiple times.
> It looks to me this is the path where the assert comes in. If I revert 
> this patch on current master, the machine boots without the assertion.
> 
> Would you be able to help out how we can best resolve this? Ofcourse, if 
> there is anything needed to be changed on the allwinner-h3-dramc.c file, 
> I would be happy to prepare a patch for that.

David's patch LGTM and I think your model might be somehow abusing the
memory API, but I'd like to read on the DRAMCOM Control Register to
understand the allwinner_h3_dramc_map_rows() logic. I couldn't find a
reference looking at Allwinner_H3_Datasheet_V1.2.pdf.
I wonder if we could ignore implementing it.

Your use case is typically what I tried to solve with this model:
https://lore.kernel.org/qemu-devel/20210419094329.1402767-2-f4bug@amsat.org/

In your case, @span_size is your amount of DRAM, and @region_size is the
area u-boot is scanning (and @offset is zero).
Could that work, or is DRAMCOM doing much more?

Thanks,

Phil.

P.D. reference to documentation welcome :)

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[David Hildenbrand]

On 30.01.22 23:50, Niek Linnenbank wrote:
> Hi David,

Hi Niek,

thanks for the report.

> 
> While I realize my response is quite late, I wanted to report this error
> I found when running the acceptance
> tests for the orangepi-pc machine using avocado:
> 
> ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
> --show=app,console run -t machine:orangepi-pc
> tests/avocado/boot_linux_console.py
> ...
>  (4/5)
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08:
> -console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> \console: DRAM:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> reached\nOriginal status: ERROR\n{'name':
> '4-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08',
> 'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49e...
> (90.64 s)
>  (5/5)
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
> /console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000)
> console: DRAM:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> reached\nOriginal status: ERROR\n{'name':
> '5-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9',
> 'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49...
> (90.64 s)
> RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 2 |
> CANCEL 0
> JOB TIME   : 221.25 s
> 
> Basically the two tests freeze during the part where the U-Boot
> bootloader needs to detect the amount of memory. We model this in the
> hw/misc/allwinner-h3-dramc.c file.
> And when running the machine manually it shows an assert on
> 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was
> able to collect this backtrace:
> 
> $ gdb ./build/qemu-system-arm
> ...
> gdb) run -M orangepi-pc -nographic
> ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
> ...
> U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> DRAM:
> qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion:
> Assertion `alias->mapped_via_alias >= 0' failed.
> 
> Thread 4 "qemu-system-arm" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7ffff5f72700 (LWP 32866)]
> __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> (gdb) bt
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007ffff7535859 in __GI_abort () at abort.c:79
> #2  0x00007ffff7535729 in __assert_fail_base
>     (fmt=0x7ffff76cb588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
> assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
> file=0x55555642f8cd "../softmmu/memory.c", line=2588,
> function=<optimized out>)
>     at assert.c:92
> #3  0x00007ffff7546f36 in __GI___assert_fail
>     (assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
> file=0x55555642f8cd "../softmmu/memory.c", line=2588,
> function=0x555556430690 <__PRETTY_FUNCTION__.8>
> "memory_region_del_subregion") at assert.c:101
> #4  0x0000555555e587e0 in memory_region_del_subregion
> (mr=0x555556f0bc00, subregion=0x7ffff5fa1090) at ../softmmu/memory.c:2588
> #5  0x0000555555e589f3 in memory_region_readd_subregion
> (mr=0x7ffff5fa1090) at ../softmmu/memory.c:2630
> #6  0x0000555555e58a5f in memory_region_set_address (mr=0x7ffff5fa1090,
> addr=1090519040) at ../softmmu/memory.c:2642
> #7  0x0000555555ac352b in allwinner_h3_dramc_map_rows (s=0x7ffff5fa0c50,
> row_bits=16 '\020', bank_bits=2 '\002', page_size=512) at
> ../hw/misc/allwinner-h3-dramc.c:92
> #8  0x0000555555ac36c2 in allwinner_h3_dramcom_write
> (opaque=0x7ffff5fa0c50, offset=0, val=4396785, size=4) at
> ../hw/misc/allwinner-h3-dramc.c:131
> #9  0x0000555555e52561 in memory_region_write_accessor
> (mr=0x7ffff5fa11a0, addr=0, value=0x7ffff5f710e8, size=4, shift=0,
> mask=4294967295, attrs=...) at ../softmmu/memory.c:492
> #10 0x0000555555e527ad in access_with_adjusted_size (addr=0,
> value=0x7ffff5f710e8, size=4, access_size_min=4, access_size_max=4,
> access_fn=
>     0x555555e52467 <memory_region_write_accessor>, mr=0x7ffff5fa11a0,
> attrs=...) at ../softmmu/memory.c:554
> #11 0x0000555555e55935 in memory_region_dispatch_write
> (mr=0x7ffff5fa11a0, addr=0, data=4396785, op=MO_32, attrs=...) at
> ../softmmu/memory.c:1514
> #12 0x0000555555f891ae in io_writex (env=0x7ffff5f7ce30,
> iotlbentry=0x7fffec0275f0, mmu_idx=7, val=4396785, addr=29761536,
> retaddr=140734938367479, op=MO_32) at ../accel/tcg/cputlb.c:1420
> #13 0x0000555555f8ba10 in store_helper (env=0x7ffff5f7ce30,
> addr=29761536, val=4396785, oi=3623, retaddr=140734938367479, op=MO_32)
> at ../accel/tcg/cputlb.c:2355
> #14 0x0000555555f8bdda in full_le_stl_mmu (env=0x7ffff5f7ce30,
> addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
> ../accel/tcg/cputlb.c:2443
> #15 0x0000555555f8be16 in helper_le_stl_mmu (env=0x7ffff5f7ce30,
> addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
> ../accel/tcg/cputlb.c:2449
> #16 0x00007fff680245f7 in code_gen_buffer ()
> #17 0x0000555555f754cb in cpu_tb_exec (cpu=0x7ffff5f730a0,
> itb=0x7fffa802b400, tb_exit=0x7ffff5f7182c) at ../accel/tcg/cpu-exec.c:357
> #18 0x0000555555f76366 in cpu_loop_exec_tb (cpu=0x7ffff5f730a0,
> tb=0x7fffa802b400, last_tb=0x7ffff5f71840, tb_exit=0x7ffff5f7182c) at
> ../accel/tcg/cpu-exec.c:842
> #19 0x0000555555f76720 in cpu_exec (cpu=0x7ffff5f730a0) at
> ../accel/tcg/cpu-exec.c:1001
> #20 0x0000555555f993dd in tcg_cpus_exec (cpu=0x7ffff5f730a0) at
> ../accel/tcg/tcg-accel-ops.c:67
> #21 0x0000555555f9976d in mttcg_cpu_thread_fn (arg=0x7ffff5f730a0) at
> ../accel/tcg/tcg-accel-ops-mttcg.c:95
> #22 0x000055555624bf4d in qemu_thread_start (args=0x5555572b6780) at
> ../util/qemu-thread-posix.c:556
> #23 0x00007ffff770b609 in start_thread (arg=<optimized out>) at
> pthread_create.c:477
> #24 0x00007ffff7632293 in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> 
> So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call
> memory_region_set_address, where internally we are calling
> memory_region_del_subregion.

Okay, so we're using memory_region_set_address() on an alias after
marking it as enabled.

memory_region_readd_subregion() detect if the region is already added
via "mr->container" ... so in the old code, the

memory_region_del_subregion()
mr->container = container;
memory_region_update_container_subregions(mr);

I think the issue is that we want to do a "del+add" but we do a
"del+hack", not a proper add.

Would something like the following do the trick (untested)?:


diff --git a/softmmu/memory.c b/softmmu/memory.c
index 0d39cf3da6..7a1c8158c5 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2633,8 +2633,7 @@ static void
memory_region_readd_subregion(MemoryRegion *mr)
         memory_region_transaction_begin();
         memory_region_ref(mr);
         memory_region_del_subregion(container, mr);
-        mr->container = container;
-        memory_region_update_container_subregions(mr);
+        memory_region_add_subregion_common(container, mr->addr, mr);
         memory_region_unref(mr);
         memory_region_transaction_commit();
     }


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 8055 bytes --]

Hi David,

On Mon, Jan 31, 2022 at 9:11 AM David Hildenbrand <david@redhat.com> wrote:

> On 30.01.22 23:50, Niek Linnenbank wrote:
> > Hi David,
>
> Hi Niek,
>
> thanks for the report.
>
> >
> > While I realize my response is quite late, I wanted to report this error
> > I found when running the acceptance
> > tests for the orangepi-pc machine using avocado:
> >
> > ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
> > --show=app,console run -t machine:orangepi-pc
> > tests/avocado/boot_linux_console.py
> > ...
> >  (4/5)
> >
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08:
> > -console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> > \console: DRAM:
> > INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> > reached\nOriginal status: ERROR\n{'name':
> >
> '4-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08',
> > 'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49e...
> > (90.64 s)
> >  (5/5)
> >
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
> > /console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000)
> > console: DRAM:
> > INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> > reached\nOriginal status: ERROR\n{'name':
> >
> '5-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9',
> > 'logdir': '/home/fox/avocado/job-results/job-2022-01-30T23.09-af49...
> > (90.64 s)
> > RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 2 |
> > CANCEL 0
> > JOB TIME   : 221.25 s
> >
> > Basically the two tests freeze during the part where the U-Boot
> > bootloader needs to detect the amount of memory. We model this in the
> > hw/misc/allwinner-h3-dramc.c file.
> > And when running the machine manually it shows an assert on
> > 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was
> > able to collect this backtrace:
> >
> > $ gdb ./build/qemu-system-arm
> > ...
> > gdb) run -M orangepi-pc -nographic
> > ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
> > ...
> > U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> > DRAM:
> > qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion:
> > Assertion `alias->mapped_via_alias >= 0' failed.
> >
> > Thread 4 "qemu-system-arm" received signal SIGABRT, Aborted.
> > [Switching to Thread 0x7ffff5f72700 (LWP 32866)]
> > __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> > 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
> > (gdb) bt
> > #0  __GI_raise (sig=sig@entry=6) at
> ../sysdeps/unix/sysv/linux/raise.c:50
> > #1  0x00007ffff7535859 in __GI_abort () at abort.c:79
> > #2  0x00007ffff7535729 in __assert_fail_base
> >     (fmt=0x7ffff76cb588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
> > assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
> > file=0x55555642f8cd "../softmmu/memory.c", line=2588,
> > function=<optimized out>)
> >     at assert.c:92
> > #3  0x00007ffff7546f36 in __GI___assert_fail
> >     (assertion=0x55555642fd65 "alias->mapped_via_alias >= 0",
> > file=0x55555642f8cd "../softmmu/memory.c", line=2588,
> > function=0x555556430690 <__PRETTY_FUNCTION__.8>
> > "memory_region_del_subregion") at assert.c:101
> > #4  0x0000555555e587e0 in memory_region_del_subregion
> > (mr=0x555556f0bc00, subregion=0x7ffff5fa1090) at ../softmmu/memory.c:2588
> > #5  0x0000555555e589f3 in memory_region_readd_subregion
> > (mr=0x7ffff5fa1090) at ../softmmu/memory.c:2630
> > #6  0x0000555555e58a5f in memory_region_set_address (mr=0x7ffff5fa1090,
> > addr=1090519040) at ../softmmu/memory.c:2642
> > #7  0x0000555555ac352b in allwinner_h3_dramc_map_rows (s=0x7ffff5fa0c50,
> > row_bits=16 '\020', bank_bits=2 '\002', page_size=512) at
> > ../hw/misc/allwinner-h3-dramc.c:92
> > #8  0x0000555555ac36c2 in allwinner_h3_dramcom_write
> > (opaque=0x7ffff5fa0c50, offset=0, val=4396785, size=4) at
> > ../hw/misc/allwinner-h3-dramc.c:131
> > #9  0x0000555555e52561 in memory_region_write_accessor
> > (mr=0x7ffff5fa11a0, addr=0, value=0x7ffff5f710e8, size=4, shift=0,
> > mask=4294967295, attrs=...) at ../softmmu/memory.c:492
> > #10 0x0000555555e527ad in access_with_adjusted_size (addr=0,
> > value=0x7ffff5f710e8, size=4, access_size_min=4, access_size_max=4,
> > access_fn=
> >     0x555555e52467 <memory_region_write_accessor>, mr=0x7ffff5fa11a0,
> > attrs=...) at ../softmmu/memory.c:554
> > #11 0x0000555555e55935 in memory_region_dispatch_write
> > (mr=0x7ffff5fa11a0, addr=0, data=4396785, op=MO_32, attrs=...) at
> > ../softmmu/memory.c:1514
> > #12 0x0000555555f891ae in io_writex (env=0x7ffff5f7ce30,
> > iotlbentry=0x7fffec0275f0, mmu_idx=7, val=4396785, addr=29761536,
> > retaddr=140734938367479, op=MO_32) at ../accel/tcg/cputlb.c:1420
> > #13 0x0000555555f8ba10 in store_helper (env=0x7ffff5f7ce30,
> > addr=29761536, val=4396785, oi=3623, retaddr=140734938367479, op=MO_32)
> > at ../accel/tcg/cputlb.c:2355
> > #14 0x0000555555f8bdda in full_le_stl_mmu (env=0x7ffff5f7ce30,
> > addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
> > ../accel/tcg/cputlb.c:2443
> > #15 0x0000555555f8be16 in helper_le_stl_mmu (env=0x7ffff5f7ce30,
> > addr=29761536, val=4396785, oi=3623, retaddr=140734938367479) at
> > ../accel/tcg/cputlb.c:2449
> > #16 0x00007fff680245f7 in code_gen_buffer ()
> > #17 0x0000555555f754cb in cpu_tb_exec (cpu=0x7ffff5f730a0,
> > itb=0x7fffa802b400, tb_exit=0x7ffff5f7182c) at
> ../accel/tcg/cpu-exec.c:357
> > #18 0x0000555555f76366 in cpu_loop_exec_tb (cpu=0x7ffff5f730a0,
> > tb=0x7fffa802b400, last_tb=0x7ffff5f71840, tb_exit=0x7ffff5f7182c) at
> > ../accel/tcg/cpu-exec.c:842
> > #19 0x0000555555f76720 in cpu_exec (cpu=0x7ffff5f730a0) at
> > ../accel/tcg/cpu-exec.c:1001
> > #20 0x0000555555f993dd in tcg_cpus_exec (cpu=0x7ffff5f730a0) at
> > ../accel/tcg/tcg-accel-ops.c:67
> > #21 0x0000555555f9976d in mttcg_cpu_thread_fn (arg=0x7ffff5f730a0) at
> > ../accel/tcg/tcg-accel-ops-mttcg.c:95
> > #22 0x000055555624bf4d in qemu_thread_start (args=0x5555572b6780) at
> > ../util/qemu-thread-posix.c:556
> > #23 0x00007ffff770b609 in start_thread (arg=<optimized out>) at
> > pthread_create.c:477
> > #24 0x00007ffff7632293 in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> >
> > So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call
> > memory_region_set_address, where internally we are calling
> > memory_region_del_subregion.
>
> Okay, so we're using memory_region_set_address() on an alias after
> marking it as enabled.
>
> memory_region_readd_subregion() detect if the region is already added
> via "mr->container" ... so in the old code, the
>
> memory_region_del_subregion()
> mr->container = container;
> memory_region_update_container_subregions(mr);
>
> I think the issue is that we want to do a "del+add" but we do a
> "del+hack", not a proper add.
>

Okey, yeah that makes sense.


>
> Would something like the following do the trick (untested)?:
>
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 0d39cf3da6..7a1c8158c5 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2633,8 +2633,7 @@ static void
> memory_region_readd_subregion(MemoryRegion *mr)
>          memory_region_transaction_begin();
>          memory_region_ref(mr);
>          memory_region_del_subregion(container, mr);
> -        mr->container = container;
> -        memory_region_update_container_subregions(mr);
> +        memory_region_add_subregion_common(container, mr->addr, mr);
>          memory_region_unref(mr);
>          memory_region_transaction_commit();
>      }
>

Yes, this resolved the assertion problem indeed. I've re-run all acceptance
tests for the orangepi-pc machine with this change applied to
the current master at 95a6af2a00, and all tests are passing.

How do we proceed from here, can this become a new patch maybe?

Thanks for your help,
Niek

>
>
> --
> Thanks,
>
> David / dhildenb
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 10071 bytes --]

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 4065 bytes --]

Hi Philippe,

On Mon, Jan 31, 2022 at 12:29 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Niek!
>
> (+Mark FYI)
>
> On 30/1/22 23:50, Niek Linnenbank wrote:
> > Hi David,
> >
> > While I realize my response is quite late, I wanted to report this error
> > I found when running the acceptance
> > tests for the orangepi-pc machine using avocado:
>
> Unfortunately I only run the full SD/MMC tests when I send a SD/MMC pull
> request, so missed that here.
>

I understand. These tests are behind the AVOCADO_ALLOW_LARGE_STORAGE flag
in avocado, so I guess they
don't run on gitlab as well, but I'm not sure about that.


>
> > Basically the two tests freeze during the part where the U-Boot
> > bootloader needs to detect the amount of memory. We model this in the
> > hw/misc/allwinner-h3-dramc.c file.
> > And when running the machine manually it shows an assert on
> > 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was
> > able to collect this backtrace:
> >
> > $ gdb ./build/qemu-system-arm
> > ...
> > gdb) run -M orangepi-pc -nographic
> > ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
> > ...
> > U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> > DRAM:
> > qemu-system-arm: ../softmmu/memory.c:2588: memory_region_del_subregion:
> > Assertion `alias->mapped_via_alias >= 0' failed.
> ...
>
> > So it seems that the hw/misc/allwinner-h3-dramc.c file is using the call
> > memory_region_set_address, where internally we are calling
> > memory_region_del_subregion.
> > The allwinner-h3-dramc.c file does use
> > memory_region_add_subregion_overlap once in the realize function, but
> > might use the memory_region_set_address multiple times.
> > It looks to me this is the path where the assert comes in. If I revert
> > this patch on current master, the machine boots without the assertion.
> >
> > Would you be able to help out how we can best resolve this? Ofcourse, if
> > there is anything needed to be changed on the allwinner-h3-dramc.c file,
> > I would be happy to prepare a patch for that.
>
> David's patch LGTM and I think your model might be somehow abusing the
> memory API, but I'd like to read on the DRAMCOM Control Register to
> understand the allwinner_h3_dramc_map_rows() logic. I couldn't find a
> reference looking at Allwinner_H3_Datasheet_V1.2.pdf.
> I wonder if we could ignore implementing it.
>

Yes David's fix using memory_region_add_subregion_common inside
memory_region_readd_subregion resolves the issue indeed.
Well the allwinner-h3-dramc.c module works OK for now, but it can certainly
use improvements indeed.
And you're right, unfortunately the DRAMCOM device isn't documented in the
datasheet as far as I know.


>
> Your use case is typically what I tried to solve with this model:
>
> https://lore.kernel.org/qemu-devel/20210419094329.1402767-2-f4bug@amsat.org/
>
> In your case, @span_size is your amount of DRAM, and @region_size is the
> area u-boot is scanning (and @offset is zero).
> Could that work, or is DRAMCOM doing much more?
>

The current model in allwinner-h3-dramc.c is roughly based on the code that
is present in U-Boot in the file arm/arm/mach-sunxi/dram_sunxi_dw.c.
It implements the low-level initialization of the memory controller, and
when running using Qemu the most important thing it needs to do is
detect the amount of memory. If it cannot accomplish this task, the U-Boot
SPL won't boot properly or crash later. So what we have in
the allwinner-h3-dramc.c implementation comes from the information and code
in the dram_sunxi_dw.c file in U-Boot, not the datasheet.

The proposal you send with span_size/region_size looks interesting indeed.
It would be great if this could help
simplify the code in allwinner-h3-dramc.c. But it would require some effort
to figure out if it can indeed replace the current
behavior.

Kind regards,
Niek


>
> Thanks,
>
> Phil.
>
> P.D. reference to documentation welcome :)
>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 5412 bytes --]

Re: [PATCH v3 2/3] memory: Make memory_region_is_mapped() succeed when mapped via an alias

[Philippe Mathieu-Daudé via]

On 31/1/22 20:20, Niek Linnenbank wrote:
> Hi Philippe,
> 
> On Mon, Jan 31, 2022 at 12:29 AM Philippe Mathieu-Daudé <f4bug@amsat.org 
> <mailto:f4bug@amsat.org>> wrote:
> 
>     Hi Niek!
> 
>     (+Mark FYI)
> 
>     On 30/1/22 23:50, Niek Linnenbank wrote:
>      > Hi David,
>      >
>      > While I realize my response is quite late, I wanted to report
>     this error
>      > I found when running the acceptance
>      > tests for the orangepi-pc machine using avocado:
> 
>     Unfortunately I only run the full SD/MMC tests when I send a SD/MMC pull
>     request, so missed that here.
> 
> 
> I understand. These tests are behind the AVOCADO_ALLOW_LARGE_STORAGE 
> flag in avocado, so I guess they
> don't run on gitlab as well, but I'm not sure about that.

Indeed they don't run on GitLab due to that flag, but I run them locally
before sending a SD/MMC pull request (along with older images that are
not available anymore on the internet but are still in my local Avocado
cache).

>      > Basically the two tests freeze during the part where the U-Boot
>      > bootloader needs to detect the amount of memory. We model this in
>     the
>      > hw/misc/allwinner-h3-dramc.c file.
>      > And when running the machine manually it shows an assert on
>      > 'alias->mapped_via_alias >= 0'. When running manually via gdb, I was
>      > able to collect this backtrace:
>      >
>      > $ gdb ./build/qemu-system-arm
>      > ...
>      > gdb) run -M orangepi-pc -nographic
>      > ./Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img
>      > ...
>      > U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
>      > DRAM:
>      > qemu-system-arm: ../softmmu/memory.c:2588:
>     memory_region_del_subregion:
>      > Assertion `alias->mapped_via_alias >= 0' failed.
>     ...
> 
>      > So it seems that the hw/misc/allwinner-h3-dramc.c file is using
>     the call
>      > memory_region_set_address, where internally we are calling
>      > memory_region_del_subregion.
>      > The allwinner-h3-dramc.c file does use
>      > memory_region_add_subregion_overlap once in the realize function,
>     but
>      > might use the memory_region_set_address multiple times.
>      > It looks to me this is the path where the assert comes in. If I
>     revert
>      > this patch on current master, the machine boots without the
>     assertion.
>      >
>      > Would you be able to help out how we can best resolve this?
>     Ofcourse, if
>      > there is anything needed to be changed on the
>     allwinner-h3-dramc.c file,
>      > I would be happy to prepare a patch for that.
> 
>     David's patch LGTM and I think your model might be somehow abusing the
>     memory API, but I'd like to read on the DRAMCOM Control Register to
>     understand the allwinner_h3_dramc_map_rows() logic. I couldn't find a
>     reference looking at Allwinner_H3_Datasheet_V1.2.pdf.
>     I wonder if we could ignore implementing it.
> 
> 
> Yes David's fix using memory_region_add_subregion_common inside 
> memory_region_readd_subregion resolves the issue indeed.

Great.

> Well the allwinner-h3-dramc.c module works OK for now, but it can 
> certainly use improvements indeed.
> And you're right, unfortunately the DRAMCOM device isn't documented in 
> the datasheet as far as I know.

OK :/

>     Your use case is typically what I tried to solve with this model:
>     https://lore.kernel.org/qemu-devel/20210419094329.1402767-2-f4bug@amsat.org/
> 
>     In your case, @span_size is your amount of DRAM, and @region_size is the
>     area u-boot is scanning (and @offset is zero).
>     Could that work, or is DRAMCOM doing much more?
> 
> 
> The current model in allwinner-h3-dramc.c is roughly based on the code 
> that is present in U-Boot in the file arm/arm/mach-sunxi/dram_sunxi_dw.c.
> It implements the low-level initialization of the memory controller, and 
> when running using Qemu the most important thing it needs to do is
> detect the amount of memory. If it cannot accomplish this task, the 
> U-Boot SPL won't boot properly or crash later. So what we have in
> the allwinner-h3-dramc.c implementation comes from the information and 
> code in the dram_sunxi_dw.c file in U-Boot, not the datasheet.

OK, this is a good start point. I'll look at the memory accesses
(certainly not today, but that problem is of my interest).

> The proposal you send with span_size/region_size looks interesting 
> indeed. It would be great if this could help
> simplify the code in allwinner-h3-dramc.c. But it would require some 
> effort to figure out if it can indeed replace the current
> behavior.

Regards,

Phil.