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

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.