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

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

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

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

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.

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

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