Re: [PATCH 0/2] arm: Add collie and sx functional tests

["Jan Lübbe" <jlu@pengutronix.de>]

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
boot partition size would be 0, meaning that the eMMC has no boot
partitions.

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.

Jan

[1]https://lore.kernel.org/qemu-devel/8f3536a0-2685-4aa4-b26d-f460e738d387@roeck-us.net/#t
-- 
Pengutronix e.K.                        |                             |
Steuerwalder Str. 21                    | https://www.pengutronix.de/ |
31137 Hildesheim, Germany               | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686        | Fax:   +49-5121-206917-5555 |