On 10/25/24 02:57, Jan Lübbe wrote:
Hi,
On Fri, 2024-10-25 at 08:55 +0200, Cédric Le Goater wrote:
On 10/24/24 19:59, Philippe Mathieu-Daudé wrote:
Cc'ing Jan.
On 22/10/24 12:04, Guenter Roeck wrote:
On 10/21/24 21:09, Philippe Mathieu-Daudé wrote:
Hi Guenter,
On 21/10/24 11:02, Guenter Roeck wrote:
Unrelated to this, but I found that the sd emulation in 9.1 is also broken
for loongarch and sifive_u, and partially for ast2600-evb (it has two
controllers, with one of them no longer working). That is too much for me
to track down quickly, so this is just a heads-up.
It would greatly help if we could merge some of your testsuite under QEMU.
Please Cc me with reproducer or assign Gitlab issues to me,
I'll have a look.
If it wasn't funny, it would be sad.
hw/sd/sd.c:sd_reset() calls sd_bootpart_offset() t determine the boot partition
offset. That function needs to have sd->ext_csd[EXT_CSD_PART_CONFIG] configured.
However, the value is only set later in sd_reset() with the call to
sc->set_csd().
One of the parameters of that function is the previously calculated size.
So in other words there is a circular dependency. The call to
sd_bootpart_offset()
returns 0 because sd->ext_csd[EXT_CSD_PART_CONFIG] isn't set, then
the call to sc->set_csd() sets it ... too late. Subsequent calls to
sd_bootpart_offset() will then return the expected offset.
oh. I didn't realize that :/ I guess we only tested our own scenario when
we were working on the ast2600 bringup.
Ah, I missed that early call to sd_bootpart_offset() as well.
As this function calculates the *runtime* offset which changes as the
guest switches between accessing the main user data area and the boot
partitions by writing to the EXT_CSD_PART_CONFIG_ACC_MASK bits, it
shouldn't be involved in the calculations in sd_reset() at all.
I think the change in the reset handler should be :
- size -= sd_bootpart_offset(sd);
+ if (sd_is_emmc(sd)) {
+ size -= sd->boot_part_size * 2;
+ }
Yes, that seems correct.
I have no idea how this is supposed to work, and I don't think it works
as implemented. I'll revert commit e32ac563b83 in my local copy of qemu.
Jan, can you have a look at this bug report please? Otherwise I'll
have a look during soft freeze.
Guenter, just to make sure: In your case [1], you had "boot-emmc" (see
aspeed_machine_ast2600_class_emmc_init) enabled, right? Otherwise the
I tried both enabled and disabled.
boot partition size would be 0, meaning that the eMMC has no boot
partitions.
That is what I would have expected, but it does not reflect reality.
In that configuration, your backing file needs to have space for the
boot partitions at the start (2*1MiB) and the rootfs starts at the 2
MiB offset.
boot-emmc doesn't make a difference, because
if (emmc) {
qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
qdev_prop_set_uint8(card, "boot-config",
boot_emmc ? 0x1 << 3 : 0x0);
}
in hw/arm/aspeed.c sets the boot partition size independently of the
boot-emmc flag. I suspect that the expectation is that I always provide
an emmc image with a 2 MB gap at the beginning, but in my opinion that is
really hyper-clumsy, and I simply won't do it, sorry. I can work around
the problem by changing the above code to only set boot-partition-size if
boot_emmc is set, or I can revert commit e32ac563b83. For now I chose the
latter.
Thanks,
Guenter
