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

[Niek Linnenbank <nieklinnenbank@gmail.com>]

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