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

["Philippe Mathieu-Daudé via" <qemu-devel@nongnu.org>]

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