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

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

[Peter Maydell]

This patchset adds new functional tests for the collie and sx1
boards, which are the only remaining ones that survived the
culling of the OMAP/PXA2xx/strongarm machines.

For these tests I'm indebted to Guenter Roeck, who has kindly
built and made available the kernel images, rootfs, etc and
documented the commands needed to boot them. All I've done
here is wrap those up into test cases in our testcase
framework by cribbing from some of our existing test code.

Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
 -- the sd card test for the sx1 board will not pass without
    that bugfix

thanks
-- PMM

Peter Maydell (2):
  tests/functional: Add a functional test for the collie board
  tests/functional: Add a functional test for the sx1 board

 MAINTAINERS                         |  1 +
 tests/functional/meson.build        |  3 ++
 tests/functional/test_arm_collie.py | 31 +++++++++++++
 tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+)
 create mode 100755 tests/functional/test_arm_collie.py
 create mode 100755 tests/functional/test_arm_sx1.py

-- 
2.34.1

[PATCH 1/2] tests/functional: Add a functional test for the collie board

[Peter Maydell]

Add a functional test for the collie board that uses the kernel and
rootfs provided by Guenter Roeck in the linux-test-downloads repo:
 https://github.com/groeck/linux-test-downloads/

This just boots Linux with a userspace that immediately reboots
the board, so we wait for the reboot log line.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 MAINTAINERS                         |  1 +
 tests/functional/meson.build        |  1 +
 tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)
 create mode 100755 tests/functional/test_arm_collie.py

diff --git a/MAINTAINERS b/MAINTAINERS
index c21d6a2f9e1..10b3360ed40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -929,6 +929,7 @@ F: hw/arm/strongarm*
 F: hw/gpio/zaurus.c
 F: include/hw/arm/sharpsl.h
 F: docs/system/arm/collie.rst
+F: tests/functional/test_arm_collie.py
 
 Stellaris
 M: Peter Maydell <peter.maydell@linaro.org>
diff --git a/tests/functional/meson.build b/tests/functional/meson.build
index c90c02517a0..0450805a9c4 100644
--- a/tests/functional/meson.build
+++ b/tests/functional/meson.build
@@ -51,6 +51,7 @@ tests_alpha_system_thorough = [
 
 tests_arm_system_thorough = [
   'arm_canona1100',
+  'arm_collie',
   'arm_integratorcp',
   'arm_raspi2',
   'arm_vexpress',
diff --git a/tests/functional/test_arm_collie.py b/tests/functional/test_arm_collie.py
new file mode 100755
index 00000000000..7e144a0a8fb
--- /dev/null
+++ b/tests/functional/test_arm_collie.py
@@ -0,0 +1,31 @@
+#!/usr/bin/env python3
+#
+# Functional test that boots a Linux kernel on a collie machine
+# and checks the console
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+from qemu_test import LinuxKernelTest, Asset
+from qemu_test.utils import archive_extract
+
+class CollieTest(LinuxKernelTest):
+
+    ASSET_ZIMAGE = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
+        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
+
+    ASSET_ROOTFS = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
+        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')
+
+    def test_arm_collie(self):
+        self.set_machine('collie')
+        zimage_path = self.ASSET_ZIMAGE.fetch()
+        rootfs_path = self.ASSET_ROOTFS.fetch()
+        self.vm.add_args('-append', 'rdinit=/sbin/init console=ttySA1')
+        self.launch_kernel(zimage_path,
+                           initrd=rootfs_path,
+                           wait_for='reboot: Restarting system')
+
+if __name__ == '__main__':
+    LinuxKernelTest.main()
-- 
2.34.1

Re: [PATCH 1/2] tests/functional: Add a functional test for the collie board

[Thomas Huth]

On 17/10/2024 18.32, Peter Maydell wrote:
> Add a functional test for the collie board that uses the kernel and
> rootfs provided by Guenter Roeck in the linux-test-downloads repo:
>   https://github.com/groeck/linux-test-downloads/
> 
> This just boots Linux with a userspace that immediately reboots
> the board, so we wait for the reboot log line.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   MAINTAINERS                         |  1 +
>   tests/functional/meson.build        |  1 +
>   tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
>   3 files changed, 33 insertions(+)
>   create mode 100755 tests/functional/test_arm_collie.py
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c21d6a2f9e1..10b3360ed40 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -929,6 +929,7 @@ F: hw/arm/strongarm*
>   F: hw/gpio/zaurus.c
>   F: include/hw/arm/sharpsl.h
>   F: docs/system/arm/collie.rst
> +F: tests/functional/test_arm_collie.py
>   
>   Stellaris
>   M: Peter Maydell <peter.maydell@linaro.org>
> diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> index c90c02517a0..0450805a9c4 100644
> --- a/tests/functional/meson.build
> +++ b/tests/functional/meson.build
> @@ -51,6 +51,7 @@ tests_alpha_system_thorough = [
>   
>   tests_arm_system_thorough = [
>     'arm_canona1100',
> +  'arm_collie',
>     'arm_integratorcp',
>     'arm_raspi2',
>     'arm_vexpress',
> diff --git a/tests/functional/test_arm_collie.py b/tests/functional/test_arm_collie.py
> new file mode 100755
> index 00000000000..7e144a0a8fb
> --- /dev/null
> +++ b/tests/functional/test_arm_collie.py
> @@ -0,0 +1,31 @@
> +#!/usr/bin/env python3
> +#
> +# Functional test that boots a Linux kernel on a collie machine
> +# and checks the console
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +from qemu_test import LinuxKernelTest, Asset
> +from qemu_test.utils import archive_extract
> +
> +class CollieTest(LinuxKernelTest):
> +
> +    ASSET_ZIMAGE = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
> +        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
> +
> +    ASSET_ROOTFS = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
> +        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')
> +
> +    def test_arm_collie(self):
> +        self.set_machine('collie')
> +        zimage_path = self.ASSET_ZIMAGE.fetch()
> +        rootfs_path = self.ASSET_ROOTFS.fetch()
> +        self.vm.add_args('-append', 'rdinit=/sbin/init console=ttySA1')
> +        self.launch_kernel(zimage_path,
> +                           initrd=rootfs_path,
> +                           wait_for='reboot: Restarting system')
> +
> +if __name__ == '__main__':
> +    LinuxKernelTest.main()

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/2] tests/functional: Add a functional test for the collie board

[Philippe Mathieu-Daudé]

On 17/10/24 13:32, Peter Maydell wrote:
> Add a functional test for the collie board that uses the kernel and
> rootfs provided by Guenter Roeck in the linux-test-downloads repo:
>   https://github.com/groeck/linux-test-downloads/
> 
> This just boots Linux with a userspace that immediately reboots
> the board, so we wait for the reboot log line.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   MAINTAINERS                         |  1 +
>   tests/functional/meson.build        |  1 +
>   tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
>   3 files changed, 33 insertions(+)
>   create mode 100755 tests/functional/test_arm_collie.py


> +class CollieTest(LinuxKernelTest):
> +
> +    ASSET_ZIMAGE = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
> +        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
> +
> +    ASSET_ROOTFS = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
> +        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')

Should we wrap these lines to comply with checkpatch.pl?

Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] tests/functional: Add a functional test for the collie board

[Daniel P. Berrangé]

On Tue, Oct 22, 2024 at 01:11:31AM -0300, Philippe Mathieu-Daudé wrote:
> On 17/10/24 13:32, Peter Maydell wrote:
> > Add a functional test for the collie board that uses the kernel and
> > rootfs provided by Guenter Roeck in the linux-test-downloads repo:
> >   https://github.com/groeck/linux-test-downloads/
> > 
> > This just boots Linux with a userspace that immediately reboots
> > the board, so we wait for the reboot log line.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   MAINTAINERS                         |  1 +
> >   tests/functional/meson.build        |  1 +
> >   tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
> >   3 files changed, 33 insertions(+)
> >   create mode 100755 tests/functional/test_arm_collie.py
> 
> 
> > +class CollieTest(LinuxKernelTest):
> > +
> > +    ASSET_ZIMAGE = Asset(
> > +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
> > +        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
> > +
> > +    ASSET_ROOTFS = Asset(
> > +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
> > +        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')
> 
> Should we wrap these lines to comply with checkpatch.pl?

While we have some line wrapped URLs in the tests already, IMHO they are
pretty awful to look at. I think we should make URLs be an exception to
line wrapping rules, provided the URL is the /only/ thing on the line.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/2] tests/functional: Add a functional test for the collie board

[Peter Maydell]

On Tue, 22 Oct 2024 at 08:09, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Oct 22, 2024 at 01:11:31AM -0300, Philippe Mathieu-Daudé wrote:
> > On 17/10/24 13:32, Peter Maydell wrote:
> > > Add a functional test for the collie board that uses the kernel and
> > > rootfs provided by Guenter Roeck in the linux-test-downloads repo:
> > >   https://github.com/groeck/linux-test-downloads/
> > >
> > > This just boots Linux with a userspace that immediately reboots
> > > the board, so we wait for the reboot log line.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >   MAINTAINERS                         |  1 +
> > >   tests/functional/meson.build        |  1 +
> > >   tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
> > >   3 files changed, 33 insertions(+)
> > >   create mode 100755 tests/functional/test_arm_collie.py
> >
> >
> > > +class CollieTest(LinuxKernelTest):
> > > +
> > > +    ASSET_ZIMAGE = Asset(
> > > +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
> > > +        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
> > > +
> > > +    ASSET_ROOTFS = Asset(
> > > +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
> > > +        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')
> >
> > Should we wrap these lines to comply with checkpatch.pl?
>
> While we have some line wrapped URLs in the tests already, IMHO they are
> pretty awful to look at. I think we should make URLs be an exception to
> line wrapping rules, provided the URL is the /only/ thing on the line.

Our style guide already says
"If wrapping the line at 80 columns is obviously less readable and more
 awkward, prefer not to wrap it"
and that's the principle under which I chose not to wrap these.

(I think of this as analogous to George Orwell's "Break any
of these rules sooner than say anything outright barbarous"
rule for writing English...)

-- PMM

Re: [PATCH 1/2] tests/functional: Add a functional test for the collie board

[Philippe Mathieu-Daudé]

On 22/10/24 06:02, Peter Maydell wrote:
> On Tue, 22 Oct 2024 at 08:09, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Tue, Oct 22, 2024 at 01:11:31AM -0300, Philippe Mathieu-Daudé wrote:
>>> On 17/10/24 13:32, Peter Maydell wrote:
>>>> Add a functional test for the collie board that uses the kernel and
>>>> rootfs provided by Guenter Roeck in the linux-test-downloads repo:
>>>>    https://github.com/groeck/linux-test-downloads/
>>>>
>>>> This just boots Linux with a userspace that immediately reboots
>>>> the board, so we wait for the reboot log line.
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>>    MAINTAINERS                         |  1 +
>>>>    tests/functional/meson.build        |  1 +
>>>>    tests/functional/test_arm_collie.py | 31 +++++++++++++++++++++++++++++
>>>>    3 files changed, 33 insertions(+)
>>>>    create mode 100755 tests/functional/test_arm_collie.py
>>>
>>>
>>>> +class CollieTest(LinuxKernelTest):
>>>> +
>>>> +    ASSET_ZIMAGE = Asset(
>>>> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/zImage',
>>>> +        '10ace8abf9e0875ef8a83b8829cc3b5b50bc6d7bc3ca29f19f49f5673a43c13b')
>>>> +
>>>> +    ASSET_ROOTFS = Asset(
>>>> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/collie/rootfs-sa110.cpio',
>>>> +        '89ccaaa5c6b33331887047e1618ffe81b0f55909173944347d5d2426f3bcc1f2')
>>>
>>> Should we wrap these lines to comply with checkpatch.pl?
>>
>> While we have some line wrapped URLs in the tests already, IMHO they are
>> pretty awful to look at. I think we should make URLs be an exception to
>> line wrapping rules, provided the URL is the /only/ thing on the line.

Clever, fine by me.

> Our style guide already says
> "If wrapping the line at 80 columns is obviously less readable and more
>   awkward, prefer not to wrap it"
> and that's the principle under which I chose not to wrap these.
> 
> (I think of this as analogous to George Orwell's "Break any
> of these rules sooner than say anything outright barbarous"
> rule for writing English...)
> 
> -- PMM

[PATCH 2/2] tests/functional: Add a functional test for the sx1 board

[Peter Maydell]

Add a functional test for the sx1 board that uses the kernel and
rootfs provided by Guenter Roeck in the linux-test-downloads repo:
 https://github.com/groeck/linux-test-downloads/

We have three variants of the test for this board:
  * just boot initrd
  * boot with filesystem on SD card
  * boot from flash

In all cases these images have a userspace that is configured to
immediately reboot the system on successful boot, and the board
itself supports telling QEMU to do the reboot, so we only need to
wait for QEMU to exit (via -no-reboot).

Since there are three subtests, the test as a whole takes about
80s on my local machine. That's about the same as the aarch64_virt
test, so give it the same overall test timeout as that one.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The sdcard test requires the 'hw/sd/omap_mmc: Don't use sd_cmd_type_t' bugfix.
---
 tests/functional/meson.build     |  2 +
 tests/functional/test_arm_sx1.py | 72 ++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100755 tests/functional/test_arm_sx1.py

diff --git a/tests/functional/meson.build b/tests/functional/meson.build
index 0450805a9c4..ec72bfaf63e 100644
--- a/tests/functional/meson.build
+++ b/tests/functional/meson.build
@@ -16,6 +16,7 @@ test_timeouts = {
   'aarch64_virt' : 360,
   'acpi_bits' : 240,
   'arm_raspi2' : 120,
+  'arm_sx1' : 360,
   'mips_malta' : 120,
   'netdev_ethtool' : 180,
   'ppc_40p' : 240,
@@ -54,6 +55,7 @@ tests_arm_system_thorough = [
   'arm_collie',
   'arm_integratorcp',
   'arm_raspi2',
+  'arm_sx1',
   'arm_vexpress',
 ]
 
diff --git a/tests/functional/test_arm_sx1.py b/tests/functional/test_arm_sx1.py
new file mode 100755
index 00000000000..2d86405831e
--- /dev/null
+++ b/tests/functional/test_arm_sx1.py
@@ -0,0 +1,72 @@
+#!/usr/bin/env python3
+#
+# Copyright (c) 2024 Linaro Ltd.
+#
+# Functional test that boots a Linux kernel on an sx1 machine
+# and checks the console. We have three variants:
+#  * just boot initrd
+#  * boot with filesystem on SD card
+#  * boot from flash
+# In all cases these images have a userspace that is configured
+# to immediately reboot the system on successful boot, so we
+# only need to wait for QEMU to exit (via -no-reboot).
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+from qemu_test import LinuxKernelTest, Asset
+from qemu_test.utils import archive_extract
+
+class SX1Test(LinuxKernelTest):
+
+    ASSET_ZIMAGE = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/zImage',
+        'a0271899a8dc2165f9e0adb2d0a57fc839ae3a469722ffc56c77e108a8887615')
+
+    ASSET_INITRD = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/rootfs-armv4.cpio',
+        '35b0721249821aa544cd85b85d3cb8901db4c6d128eed86ab261e5d9e37d58f8')
+
+    ASSET_SD_FS = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/rootfs-armv4.ext2',
+        'c1db7f43ef92469ebc8605013728c8950e7608439f01d13678994f0ce101c3a8')
+
+    ASSET_FLASH = Asset(
+        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/flash',
+        '17e6a2758fa38efd2666be0879d4751fd37d194f25168a8deede420df519b676')
+
+    CONSOLE_ARGS = 'console=ttyS0,115200 earlycon=uart8250,mmio32,0xfffb0000,115200n8'
+
+    def test_arm_sx1_initrd(self):
+        self.set_machine('sx1')
+        zimage_path = self.ASSET_ZIMAGE.fetch()
+        initrd_path = self.ASSET_INITRD.fetch()
+        self.vm.add_args('-append', f'kunit.enable=0 rdinit=/sbin/init {self.CONSOLE_ARGS}')
+        self.vm.add_args('-no-reboot')
+        self.launch_kernel(zimage_path,
+                           initrd=initrd_path)
+        self.vm.wait()
+
+    def test_arm_sx1_sd(self):
+        self.set_machine('sx1')
+        zimage_path = self.ASSET_ZIMAGE.fetch()
+        sd_fs_path = self.ASSET_SD_FS.fetch()
+        self.vm.add_args('-append', f'kunit.enable=0 root=/dev/mmcblk0 rootwait {self.CONSOLE_ARGS}')
+        self.vm.add_args('-no-reboot')
+        self.vm.add_args('-snapshot')
+        self.vm.add_args('-drive', f'format=raw,if=sd,file={sd_fs_path}')
+        self.launch_kernel(zimage_path)
+        self.vm.wait()
+
+    def test_arm_sx1_flash(self):
+        self.set_machine('sx1')
+        zimage_path = self.ASSET_ZIMAGE.fetch()
+        flash_path = self.ASSET_FLASH.fetch()
+        self.vm.add_args('-append', f'kunit.enable=0 root=/dev/mtdblock3 rootwait {self.CONSOLE_ARGS}')
+        self.vm.add_args('-no-reboot')
+        self.vm.add_args('-snapshot')
+        self.vm.add_args('-drive', f'format=raw,if=pflash,file={flash_path}')
+        self.launch_kernel(zimage_path)
+        self.vm.wait()
+
+if __name__ == '__main__':
+    LinuxKernelTest.main()
-- 
2.34.1

Re: [PATCH 2/2] tests/functional: Add a functional test for the sx1 board

[Thomas Huth]

On 17/10/2024 18.32, Peter Maydell wrote:
> Add a functional test for the sx1 board that uses the kernel and
> rootfs provided by Guenter Roeck in the linux-test-downloads repo:
>   https://github.com/groeck/linux-test-downloads/
> 
> We have three variants of the test for this board:
>    * just boot initrd
>    * boot with filesystem on SD card
>    * boot from flash
> 
> In all cases these images have a userspace that is configured to
> immediately reboot the system on successful boot, and the board
> itself supports telling QEMU to do the reboot, so we only need to
> wait for QEMU to exit (via -no-reboot).
> 
> Since there are three subtests, the test as a whole takes about
> 80s on my local machine. That's about the same as the aarch64_virt
> test, so give it the same overall test timeout as that one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The sdcard test requires the 'hw/sd/omap_mmc: Don't use sd_cmd_type_t' bugfix.
> ---
>   tests/functional/meson.build     |  2 +
>   tests/functional/test_arm_sx1.py | 72 ++++++++++++++++++++++++++++++++
>   2 files changed, 74 insertions(+)
>   create mode 100755 tests/functional/test_arm_sx1.py
> 
> diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> index 0450805a9c4..ec72bfaf63e 100644
> --- a/tests/functional/meson.build
> +++ b/tests/functional/meson.build
> @@ -16,6 +16,7 @@ test_timeouts = {
>     'aarch64_virt' : 360,
>     'acpi_bits' : 240,
>     'arm_raspi2' : 120,
> +  'arm_sx1' : 360,
>     'mips_malta' : 120,
>     'netdev_ethtool' : 180,
>     'ppc_40p' : 240,
> @@ -54,6 +55,7 @@ tests_arm_system_thorough = [
>     'arm_collie',
>     'arm_integratorcp',
>     'arm_raspi2',
> +  'arm_sx1',
>     'arm_vexpress',
>   ]
>   
> diff --git a/tests/functional/test_arm_sx1.py b/tests/functional/test_arm_sx1.py
> new file mode 100755
> index 00000000000..2d86405831e
> --- /dev/null
> +++ b/tests/functional/test_arm_sx1.py
> @@ -0,0 +1,72 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (c) 2024 Linaro Ltd.
> +#
> +# Functional test that boots a Linux kernel on an sx1 machine
> +# and checks the console. We have three variants:
> +#  * just boot initrd
> +#  * boot with filesystem on SD card
> +#  * boot from flash
> +# In all cases these images have a userspace that is configured
> +# to immediately reboot the system on successful boot, so we
> +# only need to wait for QEMU to exit (via -no-reboot).
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +from qemu_test import LinuxKernelTest, Asset
> +from qemu_test.utils import archive_extract
> +
> +class SX1Test(LinuxKernelTest):
> +
> +    ASSET_ZIMAGE = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/zImage',
> +        'a0271899a8dc2165f9e0adb2d0a57fc839ae3a469722ffc56c77e108a8887615')
> +
> +    ASSET_INITRD = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/rootfs-armv4.cpio',
> +        '35b0721249821aa544cd85b85d3cb8901db4c6d128eed86ab261e5d9e37d58f8')
> +
> +    ASSET_SD_FS = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/rootfs-armv4.ext2',
> +        'c1db7f43ef92469ebc8605013728c8950e7608439f01d13678994f0ce101c3a8')
> +
> +    ASSET_FLASH = Asset(
> +        'https://github.com/groeck/linux-test-downloads/raw/225223f2ad7d637b34426810bf6c3b727b76a718/sx1/flash',
> +        '17e6a2758fa38efd2666be0879d4751fd37d194f25168a8deede420df519b676')
> +
> +    CONSOLE_ARGS = 'console=ttyS0,115200 earlycon=uart8250,mmio32,0xfffb0000,115200n8'
> +
> +    def test_arm_sx1_initrd(self):
> +        self.set_machine('sx1')
> +        zimage_path = self.ASSET_ZIMAGE.fetch()
> +        initrd_path = self.ASSET_INITRD.fetch()
> +        self.vm.add_args('-append', f'kunit.enable=0 rdinit=/sbin/init {self.CONSOLE_ARGS}')
> +        self.vm.add_args('-no-reboot')
> +        self.launch_kernel(zimage_path,
> +                           initrd=initrd_path)
> +        self.vm.wait()
> +
> +    def test_arm_sx1_sd(self):
> +        self.set_machine('sx1')
> +        zimage_path = self.ASSET_ZIMAGE.fetch()
> +        sd_fs_path = self.ASSET_SD_FS.fetch()
> +        self.vm.add_args('-append', f'kunit.enable=0 root=/dev/mmcblk0 rootwait {self.CONSOLE_ARGS}')
> +        self.vm.add_args('-no-reboot')
> +        self.vm.add_args('-snapshot')
> +        self.vm.add_args('-drive', f'format=raw,if=sd,file={sd_fs_path}')
> +        self.launch_kernel(zimage_path)
> +        self.vm.wait()
> +
> +    def test_arm_sx1_flash(self):
> +        self.set_machine('sx1')
> +        zimage_path = self.ASSET_ZIMAGE.fetch()
> +        flash_path = self.ASSET_FLASH.fetch()
> +        self.vm.add_args('-append', f'kunit.enable=0 root=/dev/mtdblock3 rootwait {self.CONSOLE_ARGS}')
> +        self.vm.add_args('-no-reboot')
> +        self.vm.add_args('-snapshot')
> +        self.vm.add_args('-drive', f'format=raw,if=pflash,file={flash_path}')
> +        self.launch_kernel(zimage_path)
> +        self.vm.wait()
> +
> +if __name__ == '__main__':
> +    LinuxKernelTest.main()

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 17/10/2024 18.32, Peter Maydell wrote:
> This patchset adds new functional tests for the collie and sx1
> boards, which are the only remaining ones that survived the
> culling of the OMAP/PXA2xx/strongarm machines.
> 
> For these tests I'm indebted to Guenter Roeck, who has kindly
> built and made available the kernel images, rootfs, etc and
> documented the commands needed to boot them. All I've done
> here is wrap those up into test cases in our testcase
> framework by cribbing from some of our existing test code.
> 
> Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
> ("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
>   -- the sd card test for the sx1 board will not pass without
>      that bugfix
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    tests/functional: Add a functional test for the collie board
>    tests/functional: Add a functional test for the sx1 board
> 
>   MAINTAINERS                         |  1 +
>   tests/functional/meson.build        |  3 ++
>   tests/functional/test_arm_collie.py | 31 +++++++++++++
>   tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
>   4 files changed, 107 insertions(+)
>   create mode 100755 tests/functional/test_arm_collie.py
>   create mode 100755 tests/functional/test_arm_sx1.py

Thanks, I'll queue the patches with this little fix squashed into the second 
patch:

diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1948,6 +1948,7 @@ S: Maintained
  F: hw/*/omap*
  F: include/hw/arm/omap.h
  F: docs/system/arm/sx1.rst
+F: tests/functional/test_arm_sx1.py

  IPack
  M: Alberto Garcia <berto@igalia.com>

  Thomas

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

[Thomas Huth]

On 21/10/2024 09.15, Thomas Huth wrote:
> On 17/10/2024 18.32, Peter Maydell wrote:
>> This patchset adds new functional tests for the collie and sx1
>> boards, which are the only remaining ones that survived the
>> culling of the OMAP/PXA2xx/strongarm machines.
>>
>> For these tests I'm indebted to Guenter Roeck, who has kindly
>> built and made available the kernel images, rootfs, etc and
>> documented the commands needed to boot them. All I've done
>> here is wrap those up into test cases in our testcase
>> framework by cribbing from some of our existing test code.
>>
>> Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
>> ("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
>>   -- the sd card test for the sx1 board will not pass without
>>      that bugfix
>>
>> thanks
>> -- PMM
>>
>> Peter Maydell (2):
>>    tests/functional: Add a functional test for the collie board
>>    tests/functional: Add a functional test for the sx1 board
>>
>>   MAINTAINERS                         |  1 +
>>   tests/functional/meson.build        |  3 ++
>>   tests/functional/test_arm_collie.py | 31 +++++++++++++
>>   tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
>>   4 files changed, 107 insertions(+)
>>   create mode 100755 tests/functional/test_arm_collie.py
>>   create mode 100755 tests/functional/test_arm_sx1.py
> 
> Thanks, I'll queue the patches [...]

Unfortunately, the test_arm_sx1.SX1Test.test_arm_sx1_sd test is not working 
for me, it seems to fail the shutdown of the QEMU binary:

ok 1 test_arm_sx1.SX1Test.test_arm_sx1_flash
ok 2 test_arm_sx1.SX1Test.test_arm_sx1_initrd
qemu received signal 9; command: "/home/thuth/tmp/qemu-build/qemu-system-arm 
-display none -vga none -chardev socket,id=mon,fd=5 -mon 
chardev=mon,mode=control -machine sx1 -chardev socket,id=console,fd=10 
-serial chardev:console -append kunit.enable=0 root=/dev/mmcblk0 rootwait 
console=ttyS0,115200 earlycon=uart8250,mmio32,0xfffb0000,115200n8 -no-reboot 
-snapshot -drive 
format=raw,if=sd,file=/home/thuth/.cache/qemu/download/c1db7f43ef92469ebc8605013728c8950e7608439f01d13678994f0ce101c3a8 
-kernel 
/home/thuth/.cache/qemu/download/a0271899a8dc2165f9e0adb2d0a57fc839ae3a469722ffc56c77e108a8887615"
Traceback (most recent call last):
   File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 611, 
in _do_shutdown
     self._soft_shutdown(timeout)
   File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 596, 
in _soft_shutdown
     self._subp.wait(timeout=timeout)
   File "/usr/lib64/python3.12/subprocess.py", line 1264, in wait
     return self._wait(timeout=timeout)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "/usr/lib64/python3.12/subprocess.py", line 2045, in _wait
     raise TimeoutExpired(self.args, timeout)
subprocess.TimeoutExpired: Command 
'('/home/thuth/tmp/qemu-build/qemu-system-arm', '-display', 'none', '-vga', 
'none', '-chardev', 'socket,id=mon,fd=5', '-mon', 
'chardev=mon,mode=control', '-machine', 'sx1', '-chardev', 
'socket,id=console,fd=10', '-serial', 'chardev:console', '-append', 
'kunit.enable=0 root=/dev/mmcblk0 rootwait console=ttyS0,115200 
earlycon=uart8250,mmio32,0xfffb0000,115200n8', '-no-reboot', '-snapshot', 
'-drive', 
'format=raw,if=sd,file=/home/thuth/.cache/qemu/download/c1db7f43ef92469ebc8605013728c8950e7608439f01d13678994f0ce101c3a8', 
'-kernel', 
'/home/thuth/.cache/qemu/download/a0271899a8dc2165f9e0adb2d0a57fc839ae3a469722ffc56c77e108a8887615')' 
timed out after 30 seconds

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
   File "/home/thuth/devel/qemu/tests/functional/test_arm_sx1.py", line 58, 
in test_arm_sx1_sd
     self.vm.wait()
   File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 666, 
in wait
     self.shutdown(timeout=timeout)
   File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 648, 
in shutdown
     self._do_shutdown(timeout)
   File "/home/thuth/devel/qemu/python/qemu/machine/machine.py", line 618, 
in _do_shutdown
     raise AbnormalShutdown("Could not perform graceful shutdown") \
qemu.machine.machine.AbnormalShutdown: Could not perform graceful shutdown

not ok 3 test_arm_sx1.SX1Test.test_arm_sx1_sd

Could you please have a look?

  Thomas

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

[Peter Maydell]

On Mon, 21 Oct 2024 at 09:01, Thomas Huth <thuth@redhat.com> wrote:
>
> On 21/10/2024 09.15, Thomas Huth wrote:
> > On 17/10/2024 18.32, Peter Maydell wrote:
> >> This patchset adds new functional tests for the collie and sx1
> >> boards, which are the only remaining ones that survived the
> >> culling of the OMAP/PXA2xx/strongarm machines.
> >>
> >> For these tests I'm indebted to Guenter Roeck, who has kindly
> >> built and made available the kernel images, rootfs, etc and
> >> documented the commands needed to boot them. All I've done
> >> here is wrap those up into test cases in our testcase
> >> framework by cribbing from some of our existing test code.
> >>
> >> Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
> >> ("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
> >>   -- the sd card test for the sx1 board will not pass without
> >>      that bugfix
> >>
> >> thanks
> >> -- PMM
> >>
> >> Peter Maydell (2):
> >>    tests/functional: Add a functional test for the collie board
> >>    tests/functional: Add a functional test for the sx1 board
> >>
> >>   MAINTAINERS                         |  1 +
> >>   tests/functional/meson.build        |  3 ++
> >>   tests/functional/test_arm_collie.py | 31 +++++++++++++
> >>   tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
> >>   4 files changed, 107 insertions(+)
> >>   create mode 100755 tests/functional/test_arm_collie.py
> >>   create mode 100755 tests/functional/test_arm_sx1.py
> >
> > Thanks, I'll queue the patches [...]
>
> Unfortunately, the test_arm_sx1.SX1Test.test_arm_sx1_sd test is not working
> for me, it seems to fail the shutdown of the QEMU binary:

The interesting information will be in the guest console
log, which seems to not be in this log. But the first
question is "did you include the omap_mmc fix that the
cover letter and patch description says is a prerequisite"? :-)

thanks
-- PMM

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

[Thomas Huth]

On 21/10/2024 11.17, Peter Maydell wrote:
> On Mon, 21 Oct 2024 at 09:01, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 21/10/2024 09.15, Thomas Huth wrote:
>>> On 17/10/2024 18.32, Peter Maydell wrote:
>>>> This patchset adds new functional tests for the collie and sx1
>>>> boards, which are the only remaining ones that survived the
>>>> culling of the OMAP/PXA2xx/strongarm machines.
>>>>
>>>> For these tests I'm indebted to Guenter Roeck, who has kindly
>>>> built and made available the kernel images, rootfs, etc and
>>>> documented the commands needed to boot them. All I've done
>>>> here is wrap those up into test cases in our testcase
>>>> framework by cribbing from some of our existing test code.
>>>>
>>>> Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
>>>> ("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
>>>>    -- the sd card test for the sx1 board will not pass without
>>>>       that bugfix
>>>>
>>>> thanks
>>>> -- PMM
>>>>
>>>> Peter Maydell (2):
>>>>     tests/functional: Add a functional test for the collie board
>>>>     tests/functional: Add a functional test for the sx1 board
>>>>
>>>>    MAINTAINERS                         |  1 +
>>>>    tests/functional/meson.build        |  3 ++
>>>>    tests/functional/test_arm_collie.py | 31 +++++++++++++
>>>>    tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
>>>>    4 files changed, 107 insertions(+)
>>>>    create mode 100755 tests/functional/test_arm_collie.py
>>>>    create mode 100755 tests/functional/test_arm_sx1.py
>>>
>>> Thanks, I'll queue the patches [...]
>>
>> Unfortunately, the test_arm_sx1.SX1Test.test_arm_sx1_sd test is not working
>> for me, it seems to fail the shutdown of the QEMU binary:
> 
> The interesting information will be in the guest console
> log, which seems to not be in this log. But the first
> question is "did you include the omap_mmc fix that the
> cover letter and patch description says is a prerequisite"? :-)

Of course I did not. It's Monday morning, apparently I'm still too tired to 
read properly ;-) - sorry for that!

/me goes to the kitchen to grab another coffee

Ok, I'll just ignore the patches for my upcoming "test/functional" pull 
request, maybe best if you simply merge them via your arm tree instead.

  Thomas

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

[Guenter Roeck]

On 10/21/24 02:17, Peter Maydell wrote:
> On Mon, 21 Oct 2024 at 09:01, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 21/10/2024 09.15, Thomas Huth wrote:
>>> On 17/10/2024 18.32, Peter Maydell wrote:
>>>> This patchset adds new functional tests for the collie and sx1
>>>> boards, which are the only remaining ones that survived the
>>>> culling of the OMAP/PXA2xx/strongarm machines.
>>>>
>>>> For these tests I'm indebted to Guenter Roeck, who has kindly
>>>> built and made available the kernel images, rootfs, etc and
>>>> documented the commands needed to boot them. All I've done
>>>> here is wrap those up into test cases in our testcase
>>>> framework by cribbing from some of our existing test code.
>>>>
>>>> Based-on: 20241017162755.710698-1-peter.maydell@linaro.org
>>>> ("hw/sd/omap_mmc: Don't use sd_cmd_type_t")
>>>>    -- the sd card test for the sx1 board will not pass without
>>>>       that bugfix
>>>>
>>>> thanks
>>>> -- PMM
>>>>
>>>> Peter Maydell (2):
>>>>     tests/functional: Add a functional test for the collie board
>>>>     tests/functional: Add a functional test for the sx1 board
>>>>
>>>>    MAINTAINERS                         |  1 +
>>>>    tests/functional/meson.build        |  3 ++
>>>>    tests/functional/test_arm_collie.py | 31 +++++++++++++
>>>>    tests/functional/test_arm_sx1.py    | 72 +++++++++++++++++++++++++++++
>>>>    4 files changed, 107 insertions(+)
>>>>    create mode 100755 tests/functional/test_arm_collie.py
>>>>    create mode 100755 tests/functional/test_arm_sx1.py
>>>
>>> Thanks, I'll queue the patches [...]
>>
>> Unfortunately, the test_arm_sx1.SX1Test.test_arm_sx1_sd test is not working
>> for me, it seems to fail the shutdown of the QEMU binary:
> 
> The interesting information will be in the guest console
> log, which seems to not be in this log. But the first
> question is "did you include the omap_mmc fix that the
> cover letter and patch description says is a prerequisite"? :-)
> 

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.

Guenter

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

[Philippe Mathieu-Daudé]

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.

Please Cc me with reproducer or assign Gitlab issues to me,
I'll have a look.

Regards,

Phil.

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

[Guenter Roeck]

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.
> 
> Please Cc me with reproducer or assign Gitlab issues to me,
> I'll have a look.
> 

The problem is caused by commit e32ac563b8 ("hw/sd/sdcard: Fix handling of disabled
boot partitions"). Reverting it fixes the problem.

The problem only affects the sd card with index 2, or in other words it is only seen
with "-drive file=<rootfs>,format=raw,if=sd,index=2".

The Linux boot log says

[   11.201043] List of all partitions:
[   11.201364] b300           14336 mmcblk0
[   11.201414]  driver: mmcblk
[   11.201649] b308            1024 mmcblk0boot0
[   11.201678]  (driver?)
[   11.202100] b310            1024 mmcblk0boot1
[   11.202129]  (driver?)

and I am trying to boot from b300 (i.e., I have "root=b300" in the Linux command line).

This happens with both ast2600-evb and fuji-bmc machines.

Given that the patch is supposed to fix a problem, I am not sure what I would have
to change to get it back to working. All I can say is that it used to work but doesn't
anymore.

Guenter

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

[Guenter Roeck]

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

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.

Guenter

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

[Philippe Mathieu-Daudé]

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

Regards,

Phil.

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

[Cédric Le Goater]

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.

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;
+    }


Thanks,

C.



>> 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.
> 
> Regards,
> 
> Phil.
> 

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

[Jan Lübbe]

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 |

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

[Guenter Roeck]

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

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

[Jan Lübbe]

On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
> On 10/25/24 02:57, Jan Lübbe wrote:
> > 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:
> > > > 
> > > > > 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. 

Ah, yes. :/

So you can have SD, eMMC with boot from boot partition *disabled* or
eMMC with boot from boot partition *enabled*.

> 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 was surprised that the boot partitions' contents are stored in the
same backing file as the main area (instead of being separate files).
But I've not researched why it was designed this way.

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

With e32ac563b83 reverted, your backing file layout will change *at
runtime* depending on whether booting from the boot partition is
enabled via EXT CSD registers. You should be able to try that using
'mmc bootpart enable <boot_part> 0 /dev/mmcblk0' (with boot_part=0 for
disabled and boot_part=1/2 for enabled).


I'm not sure what the best way forward is... perhaps make the boot-
partition-size zero if boot_emmc is false? e.g.

@@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
         }
         card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
         if (emmc) {
-            qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
+            /*
+             * Avoid the requirement for a padded disk image if booting from
+             * eMMC boot partitions is not needed.
+             */
+            qdev_prop_set_uint64(card, "boot-partition-size",
+                                boot_emmc ? 1 * MiB : 0);
             qdev_prop_set_uint8(card, "boot-config",
                                 boot_emmc ? 0x1 << 3 : 0x0);
         }


Then you'd have the choice between:
- an eMMC, but without boot partitions (and simple backing file layout)
- an eMMC with boot partitions and need a backing file with 
  the boot partitions at the beginning

That might be the lesser evil. :)

Jan

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

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

[Guenter Roeck]

On 10/25/24 08:25, Jan Lübbe wrote:
> On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
>> On 10/25/24 02:57, Jan Lübbe wrote:
>>> 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:
>>>>>
>>>>>> 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.
> 
> Ah, yes. :/
> 
> So you can have SD, eMMC with boot from boot partition *disabled* or
> eMMC with boot from boot partition *enabled*.
> 
>> 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 was surprised that the boot partitions' contents are stored in the
> same backing file as the main area (instead of being separate files).
> But I've not researched why it was designed this way.
> 
>> 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.
> 
> With e32ac563b83 reverted, your backing file layout will change *at
> runtime* depending on whether booting from the boot partition is
> enabled via EXT CSD registers. You should be able to try that using
> 'mmc bootpart enable <boot_part> 0 /dev/mmcblk0' (with boot_part=0 for
> disabled and boot_part=1/2 for enabled).
> 

That does not matter for me since I am only testing if the emulation can boot
Linux from emmc. Booing Linux from the boot partition isn't possible anyway
because it is too small.

> 
> I'm not sure what the best way forward is... perhaps make the boot-
> partition-size zero if boot_emmc is false? e.g.
> 
> @@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>           }
>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>           if (emmc) {
> -            qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
> +            /*
> +             * Avoid the requirement for a padded disk image if booting from
> +             * eMMC boot partitions is not needed.
> +             */
> +            qdev_prop_set_uint64(card, "boot-partition-size",
> +                                boot_emmc ? 1 * MiB : 0);
>               qdev_prop_set_uint8(card, "boot-config",
>                                   boot_emmc ? 0x1 << 3 : 0x0);
>           }
> 
> 
> Then you'd have the choice between:
> - an eMMC, but without boot partitions (and simple backing file layout)
> - an eMMC with boot partitions and need a backing file with
>    the boot partitions at the beginning
> 
> That might be the lesser evil. :)
> 

I would agree, but ...

I don't know how boot partitions are used in the field. Having its size fixed to 1 MiB
seems odd. Maybe a second parameter in addition to boot-emmc would be useful, so one
could say something like "boot-emmc=on,boot-partition-size=16M".

Thanks,
Guenter

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

[Philippe Mathieu-Daudé]

On 25/10/24 12:25, Jan Lübbe wrote:
> On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
>> On 10/25/24 02:57, Jan Lübbe wrote:
>>> 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:
>>>>>
>>>>>> 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.
> 
> Ah, yes. :/
> 
> So you can have SD, eMMC with boot from boot partition *disabled* or
> eMMC with boot from boot partition *enabled*.
> 
>> 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 was surprised that the boot partitions' contents are stored in the
> same backing file as the main area (instead of being separate files).
> But I've not researched why it was designed this way.

Yeah I'd have preferred separate files too, but when it seemed
to be simpler for the single user case:
https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1bab53@kaod.org/

> 
>> 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.
> 
> With e32ac563b83 reverted, your backing file layout will change *at
> runtime* depending on whether booting from the boot partition is
> enabled via EXT CSD registers. You should be able to try that using
> 'mmc bootpart enable <boot_part> 0 /dev/mmcblk0' (with boot_part=0 for
> disabled and boot_part=1/2 for enabled).
> 
> 
> I'm not sure what the best way forward is... perhaps make the boot-
> partition-size zero if boot_emmc is false? e.g.
> 
> @@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>           }
>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>           if (emmc) {
> -            qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
> +            /*
> +             * Avoid the requirement for a padded disk image if booting from
> +             * eMMC boot partitions is not needed.
> +             */
> +            qdev_prop_set_uint64(card, "boot-partition-size",
> +                                boot_emmc ? 1 * MiB : 0);
>               qdev_prop_set_uint8(card, "boot-config",
>                                   boot_emmc ? 0x1 << 3 : 0x0);
>           }
> 
> 
> Then you'd have the choice between:
> - an eMMC, but without boot partitions (and simple backing file layout)
> - an eMMC with boot partitions and need a backing file with
>    the boot partitions at the beginning
> 
> That might be the lesser evil. :)
> 
> Jan
> 

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

[Guenter Roeck]

On 10/25/24 21:47, Philippe Mathieu-Daudé wrote:
> On 25/10/24 12:25, Jan Lübbe wrote:
>> On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
>>> On 10/25/24 02:57, Jan Lübbe wrote:
>>>> 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:
>>>>>>
>>>>>>> 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.
>>
>> Ah, yes. :/
>>
>> So you can have SD, eMMC with boot from boot partition *disabled* or
>> eMMC with boot from boot partition *enabled*.
>>
>>> 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 was surprised that the boot partitions' contents are stored in the
>> same backing file as the main area (instead of being separate files).
>> But I've not researched why it was designed this way.
> 
> Yeah I'd have preferred separate files too, but when it seemed
> to be simpler for the single user case:
> https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1bab53@kaod.org/
> 

I don't mind a single file. What bothers me is that the partitioning is made
mandatory for ast2600 even if not used.

Guenter

>>
>>> 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.
>>
>> With e32ac563b83 reverted, your backing file layout will change *at
>> runtime* depending on whether booting from the boot partition is
>> enabled via EXT CSD registers. You should be able to try that using
>> 'mmc bootpart enable <boot_part> 0 /dev/mmcblk0' (with boot_part=0 for
>> disabled and boot_part=1/2 for enabled).
>>
>>
>> I'm not sure what the best way forward is... perhaps make the boot-
>> partition-size zero if boot_emmc is false? e.g.
>>
>> @@ -339,7 +339,12 @@ static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo, bool emmc,
>>           }
>>           card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>           if (emmc) {
>> -            qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>> +            /*
>> +             * Avoid the requirement for a padded disk image if booting from
>> +             * eMMC boot partitions is not needed.
>> +             */
>> +            qdev_prop_set_uint64(card, "boot-partition-size",
>> +                                boot_emmc ? 1 * MiB : 0);
>>               qdev_prop_set_uint8(card, "boot-config",
>>                                   boot_emmc ? 0x1 << 3 : 0x0);
>>           }
>>
>>
>> Then you'd have the choice between:
>> - an eMMC, but without boot partitions (and simple backing file layout)
>> - an eMMC with boot partitions and need a backing file with
>>    the boot partitions at the beginning
>>
>> That might be the lesser evil. :)
>>
>> Jan
>>
> 

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

[Cédric Le Goater]

On 10/26/24 07:54, Guenter Roeck wrote:
> On 10/25/24 21:47, Philippe Mathieu-Daudé wrote:
>> On 25/10/24 12:25, Jan Lübbe wrote:
>>> On Fri, 2024-10-25 at 06:59 -0700, Guenter Roeck wrote:
>>>> On 10/25/24 02:57, Jan Lübbe wrote:
>>>>> 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:
>>>>>>>
>>>>>>>> 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.
>>>
>>> Ah, yes. :/
>>>
>>> So you can have SD, eMMC with boot from boot partition *disabled* or
>>> eMMC with boot from boot partition *enabled*.
>>>
>>>> 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 was surprised that the boot partitions' contents are stored in the
>>> same backing file as the main area (instead of being separate files).
>>> But I've not researched why it was designed this way.
>>
>> Yeah I'd have preferred separate files too, but when it seemed
>> to be simpler for the single user case:
>> https://lore.kernel.org/qemu-devel/d48b6357-c839-4971-aa28-bdbd5b1bab53@kaod.org/
>>
> 
> I don't mind a single file. What bothers me is that the partitioning is made
> mandatory for ast2600 even if not used.

Our only use case, in 2019, was to boot QEMU ast2600 machines from an
eMMC device using an OpenBMC FW image like the ones we find on IBM
Power10 Rainier systems. I agree we only focused on this scenario.
Most of the support should be there for other use cases, and it's now
a question of finding the right tunables for the user.

I did a quick experiment using 2 patches,

one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
area in emmc image")

     @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
              sect = 0;
          }
          size = sect << HWBLOCK_SHIFT;
     -    size -= sd_bootpart_offset(sd);
     +    if (sd_is_emmc(sd)) {
     +        size -= sd->boot_part_size * 2;
     +    }
      
          sect = sd_addr_to_wpnum(size) + 1;

and another on hw/arm/aspeed.c to remove the setting of the eMMC
device properties when it is not bootable :
   
     @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
                  return;
              }
              card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
     -        if (emmc) {
     +        if (emmc && boot_emmc) {
                  qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
                  qdev_prop_set_uint8(card, "boot-config",
                                      boot_emmc ? 0x1 << 3 : 0x0);
   
(I am not saying this is correct)

I then generated an eMMC image from a raw disk image, and without the
extra 2M for the boot areas (for U-Boot binaries) :

     $ fdisk -l   obmc-phosphor-image-rainier.wic
     Disk obmc-phosphor-image-rainier.wic: 14.13 GiB, 15167689728 bytes, 29624394 sectors
     Units: sectors of 1 * 512 = 512 bytes
     Sector size (logical/physical): 512 bytes / 512 bytes
     I/O size (minimum/optimal): 512 bytes / 512 bytes
     Disklabel type: gpt
     Disk identifier: DB9A8B4B-4BB8-4D5E-83A3-67A3D4F64614
     
     Device                              Start      End  Sectors Size Type
     obmc-phosphor-image-rainier.wic1       40     2087     2048   1M Microsoft basic data
     obmc-phosphor-image-rainier.wic2     2088   133159   131072  64M Linux filesystem
     obmc-phosphor-image-rainier.wic3   133160   264231   131072  64M Linux filesystem
     obmc-phosphor-image-rainier.wic4   264232  2361383  2097152   1G Linux filesystem
     obmc-phosphor-image-rainier.wic5  2361384  4458535  2097152   1G Linux filesystem
     obmc-phosphor-image-rainier.wic6  4458536 19138599 14680064   7G Linux filesystem
     obmc-phosphor-image-rainier.wic7 19138600 29624359 10485760   5G Linux filesystem

and used that to boot an ast2600-evb from flash with an emmc device.
   
     /path/to/qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev id=net0,... \
       -drive file=./flash-ast2600-evb,format=raw,if=mtd \
       -drive file=./ast2600-evb.pnor,format=raw,if=mtd \
       -drive file=mmc-ast2600-evb.qcow2,format=qcow2,if=sd,id=sd2,index=2 \
     -nographic -snapshot -serial mon:stdio

See the run below.

Thanks,

C.



U-Boot 2019.04-00234-g2ac7fde7b0-dirty (Oct 23 2024 - 17:43:49 +0200)

SOC: AST2600-A3
eSPI Mode: SIO:Enable : SuperIO-2e
Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
Model: AST2600A1 EVB
DRAM:  1008 MiB (capacity:1024 MiB, VGA:64 MiB, ECC:off)
RC Bridge phy@1e6ed200 : Link up
MMC:

sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
Loading Environment from SPI Flash... SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB, total 64 MiB
OK
In:    serial@1e784000
Out:   serial@1e784000
Err:   serial@1e784000
Model: AST2600A1 EVB
Net:   eth0: ftgmac@1e660000
Warning: ftgmac@1e680000 (eth1) using random MAC address - 0e:95:58:4b:71:2b
, eth1: ftgmac@1e680000
Warning: ftgmac@1e670000 (eth2) using random MAC address - 5e:60:24:2d:46:b6
, eth2: ftgmac@1e670000
Warning: ftgmac@1e690000 (eth3) using random MAC address - 4e:32:35:b8:f2:62
, eth3: ftgmac@1e690000
Hit any key to stop autoboot:  0
ast# printenv
baudrate=115200
bootargs=console=ttyS4,115200n8 root=/dev/ram rw dyndbg="file drivers/spi/* +p" loglevel=10
bootcmd=bootm 20100000
bootdelay=2
ethaddr=c0:ff:ee:00:00:04
fdtcontroladdr=bcf6c258
stderr=serial@1e784000
stdin=serial@1e784000
stdout=serial@1e784000
verify=yes

Environment size: 273/65532 bytes
ast# setenv bootargs "console=ttyS4,115200n8 root=/dev/ram rw"
ast# boot
## Loading kernel from FIT Image at 20100000 ...
    Using 'conf-1' configuration
    Trying 'kernel-1' kernel subimage
      Description:  Linux kernel
      Type:         Kernel Image
      Compression:  uncompressed
      Data Start:   0x201000f0
      Data Size:    4174872 Bytes = 4 MiB
      Architecture: ARM
      OS:           Linux
      Load Address: 0x80001000
      Entry Point:  0x80001000
      Hash algo:    sha256
      Hash value:   347b1c70a8c23e643b32a0806697992106cc75fc8769ff4f53512c18506f55ad
    Verifying Hash Integrity ... sha256+ OK
## Loading ramdisk from FIT Image at 20100000 ...
    Using 'conf-1' configuration
    Trying 'ramdisk-1' ramdisk subimage
      Description:  obmc-phosphor-initramfs
      Type:         RAMDisk Image
      Compression:  uncompressed
      Data Start:   0x205068a4
      Data Size:    2063220 Bytes = 2 MiB
      Architecture: ARM
      OS:           Linux
      Load Address: unavailable
      Entry Point:  unavailable
      Hash algo:    sha256
      Hash value:   41b330cff12982e4f39907cdb8b35c6849a1f5d1e96e7e5427dab00a5720fe6b
    Verifying Hash Integrity ... sha256+ OK
## Loading fdt from FIT Image at 20100000 ...
    Using 'conf-1' configuration
    Trying 'fdt-1' fdt subimage
      Description:  Flattened Device Tree blob
      Type:         Flat Device Tree
      Compression:  uncompressed
      Data Start:   0x204fb608
      Data Size:    45519 Bytes = 44.5 KiB
      Architecture: ARM
      Hash algo:    sha256
      Hash value:   82eabb92e1ca0c2cd7540088990ecba1c82b1960518c2963b441ce56954c2a04
    Verifying Hash Integrity ... sha256+ OK
    Booting using the fdt blob at 0x204fb608
    Loading Kernel Image ... OK
    Loading Ramdisk to 8fe08000, end 8ffffb74 ... OK
    Loading Device Tree to 8fdf9000, end 8fe071ce ... OK

Starting kernel ...

[    0.000000] Booting Linux on physical CPU 0xf00
[    0.000000] Linux version 6.6.57-00318-g7b7fd0ac7dc1-dirty (legoater@alder) (arm-none-eabi-gcc (Fedora 12.4.0-1.el9) 12.4.0, GNU ld (Fedora 2.43-1.el9) 2.43) #800 SMP Wed Oct 23 11:43:23 CEST 2024
[    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=10c5387d
[    0.000000] CPU: div instructions available: patching division code
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] OF: fdt: Machine model: AST2600 EVB
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] Reserved memory: created CMA memory pool at 0xbb000000, size 64 MiB
[    0.000000] OF: reserved mem: initialized node video, compatible id shared-dma-pool
[    0.000000] OF: reserved mem: 0xbb000000..0xbeffffff (65536 KiB) map reusable video
[    0.000000] Reserved memory: created CMA memory pool at 0xba000000, size 16 MiB
[    0.000000] OF: reserved mem: initialized node framebuffer, compatible id shared-dma-pool
[    0.000000] OF: reserved mem: 0xba000000..0xbaffffff (16384 KiB) map reusable framebuffer
[    0.000000] Reserved memory: created DMA memory pool at 0x9e770000, size 0 MiB
[    0.000000] OF: reserved mem: initialized node pci_msi_memory, compatible id shared-dma-pool
[    0.000000] OF: reserved mem: 0x9e770000..0x9e7700ff (0 KiB) nomap non-reusable pci_msi_memory
[    0.000000] cma: Reserved 16 MiB at 0xb9000000 on node -1
[    0.000000] Zone ranges:
[    0.000000]   Normal   [mem 0x0000000080000000-0x00000000beffffff]
[    0.000000]   HighMem  empty
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000080000000-0x000000009e76ffff]
[    0.000000]   node   0: [mem 0x000000009e771000-0x00000000beffffff]
[    0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x00000000beffffff]
[    0.000000] On node 0, zone Normal: 1 pages in unavailable ranges
[    0.000000] percpu: Embedded 13 pages/cpu s21204 r8192 d23852 u53248
[    0.000000] Kernel command line: console=ttyS4,115200n8 root=/dev/ram rw
[    0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes, linear)
[    0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes, linear)
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 256031
[    0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[    0.000000] stackdepot: allocating hash table via alloc_large_system_hash
[    0.000000] stackdepot hash table entries: 65536 (order: 6, 262144 bytes, linear)
[    0.000000] Memory: 906472K/1032188K available (9216K kernel code, 555K rwdata, 2284K rodata, 1024K init, 1889K bss, 27412K reserved, 98304K cma-reserved, 0K highmem)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.000000] rcu: Hierarchical RCU implementation.
[    0.000000] rcu: 	RCU event tracing is enabled.
[    0.000000] 	Tracing variant of Tasks RCU enabled.
[    0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
[    0.000000] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16
[    0.000000] rcu: srcu_init: Setting srcu_struct sizes based on contention.
[    0.000000] arch_timer: cp15 timer(s) running at 1200.00MHz (phys).
[    0.000000] clocksource: arch_sys_counter: mask: 0x1fffffffffffffff max_cycles: 0x114c1bbbeec, max_idle_ns: 440795229572 ns
[    0.000122] sched_clock: 61 bits at 1200MHz, resolution 0ns, wraps every 2199023255551ns
[    0.000269] Switching to timer-based delay loop, resolution 0ns
[    0.004541] Calibrating delay loop (skipped), value calculated using timer frequency.. 2400.00 BogoMIPS (lpj=12000000)
[    0.004655] CPU: Testing write buffer coherency: ok
[    0.005791] pid_max: default: 32768 minimum: 301
[    0.015945] Mount-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
[    0.016005] Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes, linear)
[    0.048544] RCU Tasks Trace: Setting shift to 1 and lim to 1 rcu_task_cb_adjust=1.
[    0.049148] Setting up static identity map for 0x80100000 - 0x80100078
[    0.050194] rcu: Hierarchical SRCU implementation.
[    0.050223] rcu: 	Max phase no-delay instances is 1000.
[    0.055498] ASPEED AST2600 rev A3 (05030303)
[    0.056887] smp: Bringing up secondary CPUs ...
[    0.063247] smp: Brought up 1 node, 2 CPUs
[    0.063318] SMP: Total of 2 processors activated (4800.00 BogoMIPS).
[    0.063361] CPU: All CPU(s) started in SVC mode.
[    0.076322] devtmpfs: initialized
[    0.121627] VFP support v0.3: implementor 41 architecture 2 part 30 variant 7 rev 5
[    0.139892] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
[    0.140492] futex hash table entries: 512 (order: 3, 32768 bytes, linear)
[    0.146864] pinctrl core: initialized pinctrl subsystem
[    0.158505] NET: Registered PF_NETLINK/PF_ROUTE protocol family
[    0.170489] DMA: preallocated 256 KiB pool for atomic coherent allocations
[    0.177969] hw-breakpoint: found 5 (+1 reserved) breakpoint and 4 watchpoint registers.
[    0.178056] hw-breakpoint: maximum watchpoint size is 8 bytes.
[    0.253146] AST2600 secure boot disabled
[    0.254852] usbcore: registered new interface driver usbfs
[    0.255167] usbcore: registered new interface driver hub
[    0.255369] usbcore: registered new device driver usb
[    0.256011] mc: Linux media interface: v0.10
[    0.256227] videodev: Linux video capture interface: v2.00
[    0.256475] EDAC MC: Ver: 3.0.0
[    0.267571] mctp: management component transport protocol core
[    0.267602] NET: Registered PF_MCTP protocol family
[    0.272183] vgaarb: loaded
[    0.278743] clocksource: Switched to clocksource arch_sys_counter
[    0.295844] NET: Registered PF_INET protocol family
[    0.297068] IP idents hash table entries: 16384 (order: 5, 131072 bytes, linear)
[    0.301957] tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear)
[    0.302110] Table-perturb hash table entries: 65536 (order: 6, 262144 bytes, linear)
[    0.302173] TCP established hash table entries: 8192 (order: 3, 32768 bytes, linear)
[    0.302333] TCP bind hash table entries: 8192 (order: 5, 131072 bytes, linear)
[    0.302502] TCP: Hash tables configured (established 8192 bind 8192)
[    0.303413] UDP hash table entries: 512 (order: 2, 16384 bytes, linear)
[    0.303578] UDP-Lite hash table entries: 512 (order: 2, 16384 bytes, linear)
[    0.304874] NET: Registered PF_UNIX/PF_LOCAL protocol family
[    0.305268] PCI: CLS 0 bytes, default 64
[    0.312589] Unpacking initramfs...
[    0.318913] workingset: timestamp_bits=30 max_order=18 bucket_order=0
[    0.329112] squashfs: version 4.0 (2009/01/31) Phillip Lougher
[    0.331161] jffs2: version 2.2. (SUMMARY)  © 2001-2006 Red Hat, Inc.
[    0.333508] NET: Registered PF_ALG protocol family
[    0.357212] aspeed-pciecfg 1e770000.pciecfg: can't get pcie reset: -517
[    0.360696] aspeed-pcie 1e7700c0.pcie: host bridge /ahb/apb@1e6e0000/pcie@1e7700C0 ranges:
[    0.361398] aspeed-pcie 1e7700c0.pcie:       IO 0x0000018000..0x000001ffff -> 0x0000018000
[    0.361841] aspeed-pcie 1e7700c0.pcie:      MEM 0x0070000000..0x007fffffff -> 0x0070000000
[    0.365013] aspeed-pcie 1e7700c0.pcie: Parsing DT failed
[    0.377054] Serial: 8250/16550 driver, 6 ports, IRQ sharing enabled
[    0.390043] printk: console [ttyS4] disabled
[    0.415131] 1e784000.serial: ttyS4 at MMIO 0x1e784000 (irq = 33, base_baud = 1500000) is a 16550A
[    0.417346] printk: console [ttyS4] enabled
[    0.445888] timeriomem_rng 1e6e2524.hwrng: 32bits from 0x(ptrval) @ 1us
[    0.464386] random: crng init done
[    0.466579] loop: module loaded
[    0.531428] spi-nor spi0.0: mx25u51245g (65536 Kbytes)
[    0.563190] spi-aspeed-smc 1e620000.spi: CE0 read buswidth:4 [0x406c0641]
[    1.052071] Freeing initrd memory: 2016K
[    1.127789] 5 fixed-partitions partitions found on MTD device bmc
[    1.128074] Creating 5 MTD partitions on "bmc":
[    1.128530] 0x000000000000-0x0000000e0000 : "u-boot"
[    1.132732] 0x0000000e0000-0x000000100000 : "u-boot-env"
[    1.135435] 0x000000100000-0x000000a00000 : "kernel"
[    1.137924] 0x000000a00000-0x000002a00000 : "rofs"
[    1.140689] 0x000002a00000-0x000004000000 : "rwfs"
[    1.143835] spi-nor spi0.1: unrecognized JEDEC id bytes: 00 00 00 00 00 00
[    1.150726] spi-nor spi1.0: mx25u51245g (65536 Kbytes)
[    1.209269] spi-aspeed-smc 1e630000.spi: CE0 read buswidth:4 [0x406c0741]
[    1.227015] spi-nor spi2.0: unrecognized JEDEC id bytes: 00 00 00 00 00 00
[    1.277325] ftgmac100 1e660000.ethernet: Read MAC address c0:ff:ee:00:00:04 from chip
[    1.279250] RTL8211E Gigabit Ethernet 1e650000.mdio-1:00: attached PHY driver (mii_bus:phy_addr=1e650000.mdio-1:00, irq=POLL)
[    1.281528] ftgmac100 1e660000.ethernet eth0: irq 34, mapped at 79751e15
[    1.282860] ftgmac100 1e680000.ethernet: Read MAC address 0e:95:58:4b:71:2b from chip
[    1.283363] RTL8211E Gigabit Ethernet 1e650008.mdio-1:00: attached PHY driver (mii_bus:phy_addr=1e650008.mdio-1:00, irq=POLL)
[    1.284870] ftgmac100 1e680000.ethernet eth1: irq 35, mapped at 3dc7362d
[    1.285697] ftgmac100 1e670000.ethernet: Read MAC address 5e:60:24:2d:46:b6 from chip
[    1.286179] RTL8211E Gigabit Ethernet 1e650010.mdio-1:00: attached PHY driver (mii_bus:phy_addr=1e650010.mdio-1:00, irq=POLL)
[    1.287413] ftgmac100 1e670000.ethernet eth2: irq 36, mapped at da2d312c
[    1.288442] ftgmac100 1e690000.ethernet: Read MAC address 4e:32:35:b8:f2:62 from chip
[    1.288946] RTL8211E Gigabit Ethernet 1e650018.mdio-1:00: attached PHY driver (mii_bus:phy_addr=1e650018.mdio-1:00, irq=POLL)
[    1.290191] ftgmac100 1e690000.ethernet eth3: irq 37, mapped at 045a874a
[    1.290646] e1000e: Intel(R) PRO/1000 Network Driver
[    1.290767] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[    1.291109] usbcore: registered new interface driver cdc_ether
[    1.291329] usbcore: registered new interface driver cdc_eem
[    1.291551] usbcore: registered new interface driver rndis_host
[    1.291772] usbcore: registered new interface driver cdc_subset
[    1.291998] usbcore: registered new interface driver r8153_ecm
[    1.293432] platform-uhci 1e6b0000.usb: Detected 2 ports from device-tree
[    1.293643] platform-uhci 1e6b0000.usb: Enabled Aspeed implementation workarounds
[    1.304773] platform-uhci 1e6b0000.usb: Generic UHCI Host Controller
[    1.305371] platform-uhci 1e6b0000.usb: new USB bus registered, assigned bus number 1
[    1.306869] platform-uhci 1e6b0000.usb: irq 38, io mem 0x1e6b0000
[    1.319480] ehci-platform 1e6a3000.usb: EHCI Host Controller
[    1.319796] ehci-platform 1e6a3000.usb: new USB bus registered, assigned bus number 2
[    1.325447] ehci-platform 1e6a3000.usb: irq 39, io mem 0x1e6a3000
[    1.327363] usb usb1: New USB device found, idVendor=1d6b, idProduct=0001, bcdDevice= 6.06
[    1.327680] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    1.327944] usb usb1: Product: Generic UHCI Host Controller
[    1.328284] usb usb1: Manufacturer: Linux 6.6.57-00318-g7b7fd0ac7dc1-dirty uhci_hcd
[    1.328585] usb usb1: SerialNumber: 1e6b0000.usb
[    1.333949] hub 1-0:1.0: USB hub found
[    1.334617] hub 1-0:1.0: 2 ports detected
[    1.352324] ehci-platform 1e6a3000.usb: USB 2.0 started, EHCI 1.00
[    1.353520] usb usb2: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.06
[    1.353739] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
[    1.353919] usb usb2: Product: EHCI Host Controller
[    1.354048] usb usb2: Manufacturer: Linux 6.6.57-00318-g7b7fd0ac7dc1-dirty ehci_hcd
[    1.354228] usb usb2: SerialNumber: 1e6a3000.usb
[    1.355837] hub 2-0:1.0: USB hub found
[    1.356145] hub 2-0:1.0: 6 ports detected
[    1.361440] aspeed_vhub 1e6a0000.usb-vhub: Initialized virtual hub in USB2 mode
[    1.364590] g_ether gadget.0: HOST MAC 92:2c:b1:d8:ef:d6
[    1.364864] g_ether gadget.0: MAC 26:cd:b2:66:3d:c6
[    1.370159] g_ether gadget.0: Ethernet Gadget, version: Memorial Day 2008
[    1.370573] g_ether gadget.0: g_ether ready
[    1.371708] gadgetfs: USB Gadget filesystem, version 24 Aug 2004
[    1.372347] file system registered
[    1.373323] Mass Storage Function, version: 2009/09/11
[    1.373725] LUN: removable file: (no medium)
[    1.374475] no file given for LUN0
[    1.375010] udc 1e6a0000.usb-vhub:p2: failed to start g_mass_storage: -22
[    1.375216] g_mass_storage: probe of gadget.1 failed with error -22
[    1.375497] Mass Storage Function, version: 2009/09/11
[    1.375633] LUN: removable file: (no medium)
[    1.375774] no file given for LUN0
[    1.375893] udc 1e6a0000.usb-vhub:p3: failed to start g_mass_storage: -22
[    1.376055] g_mass_storage: probe of gadget.2 failed with error -22
[    1.376307] Mass Storage Function, version: 2009/09/11
[    1.376438] LUN: removable file: (no medium)
[    1.376572] no file given for LUN0
[    1.376684] udc 1e6a0000.usb-vhub:p4: failed to start g_mass_storage: -22
[    1.376846] g_mass_storage: probe of gadget.3 failed with error -22
[    1.377073] Mass Storage Function, version: 2009/09/11
[    1.377209] LUN: removable file: (no medium)
[    1.377346] no file given for LUN0
[    1.377459] udc 1e6a0000.usb-vhub:p5: failed to start g_mass_storage: -22
[    1.377625] g_mass_storage: probe of gadget.4 failed with error -22
[    1.377861] Mass Storage Function, version: 2009/09/11
[    1.377996] LUN: removable file: (no medium)
[    1.378387] no file given for LUN0
[    1.378545] udc 1e6a0000.usb-vhub:p6: failed to start g_mass_storage: -22
[    1.378712] g_mass_storage: probe of gadget.5 failed with error -22
[    1.378999] Mass Storage Function, version: 2009/09/11
[    1.379129] LUN: removable file: (no medium)
[    1.379264] no file given for LUN0
[    1.379373] udc 1e6a0000.usb-vhub:p7: failed to start g_mass_storage: -22
[    1.379532] g_mass_storage: probe of gadget.6 failed with error -22
[    1.379780] UDC core: g_mass_storage: couldn't find an available UDC
[    1.382150] aspeed-rtc 1e781000.rtc: registered as rtc0
[    1.382371] aspeed-rtc 1e781000.rtc: hctosys: unable to read the hardware clock
[    1.383125] i2c_dev: i2c /dev entries driver
[    1.388271] aspeed-i2c-bus 1e78a080.i2c: i2c bus 0 registered, irq 41
[    1.390585] aspeed-i2c-bus 1e78a100.i2c: i2c bus 1 registered, irq 42
[    1.392585] aspeed-i2c-bus 1e78a180.i2c: i2c bus 2 registered, irq 43
[    1.394340] aspeed-i2c-bus 1e78a200.i2c: i2c bus 3 registered, irq 44
[    1.396073] aspeed-i2c-bus 1e78a280.i2c: i2c bus 4 registered, irq 45
[    1.397777] aspeed-i2c-bus 1e78a300.i2c: i2c bus 5 registered, irq 46
[    1.401396] aspeed-i2c-bus 1e78a380.i2c: i2c bus 6 registered, irq 47
[    1.409287] at24 7-0050: 1024 byte 24c08 EEPROM, writable, 16 bytes/write
[    1.409732] aspeed-i2c-bus 1e78a400.i2c: i2c bus 7 registered, irq 48
[    1.411983] aspeed-i2c-bus 1e78a480.i2c: i2c bus 8 registered, irq 49
[    1.416524] aspeed-i2c-bus 1e78a500.i2c: i2c bus 9 registered, irq 50
[    1.418837] aspeed-i2c-bus 1e78a680.i2c: i2c bus 12 registered, irq 51
[    1.420850] aspeed-i2c-bus 1e78a700.i2c: i2c bus 13 registered, irq 52
[    1.422600] aspeed-i2c-bus 1e78a780.i2c: i2c bus 14 registered, irq 53
[    1.424342] aspeed-i2c-bus 1e78a800.i2c: i2c bus 15 registered, irq 54
[    1.456554] aspeed-video 1e700000.video: irq 57
[    1.457116] aspeed-video 1e700000.video: assigned reserved memory node video
[    1.459470] aspeed-video 1e700000.video: alloc mem size(24576) at 0xbb000000 for jpeg header
[    1.463628] Driver for 1-wire Dallas network protocol.
[    1.465558] adt7475 7-002e: Error configuring attenuator bypass
[    1.467012] adt7475 7-002e: ADT7490 device, revision 2
[    1.467206] adt7475 7-002e: Optional features: in4 pwm2
[    1.467367] adt7475 7-002e: Bypassing attenuators on: in1 in3 in4
[    1.470797] lm75 8-004d: hwmon0: sensor 'lm75'
[    1.473632] device-mapper: ioctl: 4.48.0-ioctl (2023-03-01) initialised: dm-devel@redhat.com
[    1.475222] sdhci: Secure Digital Host Controller Interface driver
[    1.475383] sdhci: Copyright(c) Pierre Ossman
[    1.475546] sdhci-pltfm: SDHCI platform and OF driver helper
[    1.477140] usbcore: registered new interface driver usbhid
[    1.477340] usbhid: USB HID core driver
[    1.487275] sdhci-aspeed 1e750100.sdhci: Configured for slot 0
[    1.493430] fsi-master-aspeed 1e79b000.fsi: hub version e0050101 (1 links)
[    1.493886] fsi-master-aspeed 1e79b000.fsi: mmode set to 70040110 (divisor 1)
[    1.523565] NET: Registered PF_INET6 protocol family
[    1.531816] Segment Routing with IPv6
[    1.532210] In-situ OAM (IOAM) with IPv6
[    1.533314] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[    1.536435] NET: Registered PF_PACKET protocol family
[    1.536666] 8021q: 802.1Q VLAN Support v1.8
[    1.537326] Registering SWP/SWPB emulation handler
[    1.551759] mmc0: SDHCI controller on 1e750100.sdhci [1e750100.sdhci] using ADMA
[    1.580102] kmemleak: Kernel memory leak detector initialized (mem pool available: 15783)
[    1.580182] kmemleak: Automatic memory scanning thread started
[    1.593456] aspeed-pciecfg: probe of 1e770000.pciecfg failed with error 1
[    1.594472] aspeed-pcie 1e7700c0.pcie: host bridge /ahb/apb@1e6e0000/pcie@1e7700C0 ranges:
[    1.594823] aspeed-pcie 1e7700c0.pcie:       IO 0x0000018000..0x000001ffff -> 0x0000018000
[    1.595114] aspeed-pcie 1e7700c0.pcie:      MEM 0x0070000000..0x007fffffff -> 0x0070000000
[    2.096859] aspeed-pcie 1e7700c0.pcie: PCIE- Link up
[    2.099914] aspeed-pcie 1e7700c0.pcie: PCI host bridge to bus 0001:80
[    2.100337] pci_bus 0001:80: root bus resource [bus 80-ff]
[    2.100789] pci_bus 0001:80: root bus resource [io  0x0000-0x7fff] (bus address [0x18000-0x1ffff])
[    2.101071] pci_bus 0001:80: root bus resource [mem 0x70000000-0x7fffffff]
[    2.104803] PCI: bus128: Fast back to back transfers enabled
[    2.109460] printk: console [netcon0] enabled
[    2.109696] netconsole: network logging started
[    2.110876] clk: Disabling unused clocks
[    2.147407] Freeing unused kernel image (initmem) memory: 1024K
[    2.176818] Checked W+X mappings: passed, no W+X pages found
[    2.177212] Run /init as init process
[    2.181933] mmc0: new high speed MMC card at address 0001
[    2.185019] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
[    2.208621] GPT:Primary header thinks Alt. header is not at the end of the disk.
[    2.208884] GPT:29624393 != 33554431
[    2.209017] GPT:Alternate GPT header not at the end of the disk.
[    2.209192] GPT:29624393 != 33554431
[    2.209303] GPT: Use GNU Parted to correct GPT errors.
[    2.210018]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
Saving 256 bits of creditable seed for next boot
Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Starting network: [    2.856576] ftgmac100 1e660000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
udhcpc: started, v1.36.1
udhcpc: broadcasting discover
udhcpc: broadcasting select for 192.168.199.4, server 192.168.199.1
udhcpc: lease of 192.168.199.4 obtained from 192.168.199.1, lease time 3600
deleting routers
adding dns 192.168.199.1
OK
Starting crond: OK
Starting dropbear sshd: OK

Aspeed AST2600 EVB
ast2600-evb login: [   12.652131] sdhci-aspeed 1e740100.sdhci: Configured for slot 0
[   12.655905] sdhci-aspeed 1e740200.sdhci: Configured for slot 1
[   12.735134] mmc1: SDHCI controller on 1e740100.sdhci [1e740100.sdhci] using ADMA
[   12.785076] mmc2: SDHCI controller on 1e740200.sdhci [1e740200.sdhci] using ADMA

Aspeed AST2600 EVB
ast2600-evb login: root
Password:
# fdisk -l /dev/mmcblk0
Found valid GPT with protective MBR; using GPT

Disk /dev/mmcblk0: 33554432 sectors,     0
Logical sector size: 512
Disk identifier (GUID): 00000000-0000-0000-0000-00004d9b9ef0
Partition table holds up to 128 entries
First usable sector is 34, last usable sector is 29624360

Number  Start (sector)    End (sector)  Size Name
      1              40            2087 1024K primary
      2            2088          133159 64.0M boot-a
      3          133160          264231 64.0M boot-b
      4          264232         2361383 1024M rofs-a
      5         2361384         4458535 1024M rofs-b
      6         4458536        19138599 7168M rwfs
      7        19138600        29624359 5120M hostfw
# for i in 2 3 4 5 6; do mount /dev/mmcblk0p
mmcblk0p1  mmcblk0p2  mmcblk0p3  mmcblk0p4  mmcblk0p5  mmcblk0p6  mmcblk0p7
# for i in 2 3 4 5 6 7; do mount /dev/mmcblk0p$i /mnt; ls -l /mnt/; umount /mnt
; done
[   96.295287] EXT4-fs (mmcblk0p2): mounted filesystem f85bf458-5b4f-4785-bc9a-8a343798855c r/w with ordered data mode. Quota mode: disabled.
total 7942
-rw-r--r--    1 1000     1001       8120290 Jun 18  2024 fitImage
drwx------    2 root     root         12288 Apr  5  2011 lost+found
[   96.348707] EXT4-fs (mmcblk0p2): unmounting filesystem f85bf458-5b4f-4785-bc9a-8a343798855c.
[   96.391201] EXT4-fs (mmcblk0p3): mounted filesystem 815b73b7-d5c4-46e4-ba06-46dae5312632 r/w with ordered data mode. Quota mode: disabled.
total 7942
-rw-r--r--    1 1000     1001       8120290 Jun 18  2024 fitImage
drwx------    2 root     root         12288 Apr  5  2011 lost+found
[   96.429335] EXT4-fs (mmcblk0p3): unmounting filesystem 815b73b7-d5c4-46e4-ba06-46dae5312632.
[   96.523967] EXT4-fs (mmcblk0p4): mounted filesystem 6f526507-e73b-4094-8f08-f310b5da5b3a r/w with ordered data mode. Quota mode: disabled.
total 68
lrwxrwxrwx    1 root     root             7 Mar  9  2018 bin -> usr/bin
drwxr-xr-x    2 root     root          4096 Mar  9  2018 boot
drwxr-xr-x    2 root     root          4096 Mar  9  2018 dev
drwxr-xr-x   35 root     root          4096 Mar  9  2018 etc
drwxr-xr-x    4 root     root          4096 Mar  9  2018 home
lrwxrwxrwx    1 root     root             7 Mar  9  2018 lib -> usr/lib
drwx------    2 root     root         16384 Apr  5  2011 lost+found
drwxr-xr-x    2 root     root          4096 Mar  9  2018 media
drwxr-xr-x    2 root     root          4096 Mar  9  2018 mnt
dr-xr-xr-x    2 root     root          4096 Mar  9  2018 proc
drwxr-xr-x    2 root     root          4096 Mar  9  2018 run
lrwxrwxrwx    1 root     root             8 Mar  9  2018 sbin -> usr/sbin
drwxr-xr-x    2 root     root          4096 Mar  9  2018 srv
dr-xr-xr-x    2 root     root          4096 Mar  9  2018 sys
drwxrwxrwt    2 root     root          4096 Mar  9  2018 tmp
drwxr-xr-x   11 root     root          4096 Mar  9  2018 usr
drwxr-xr-x   11 root     root          4096 Mar  9  2018 var
[   96.632275] EXT4-fs (mmcblk0p4): unmounting filesystem 6f526507-e73b-4094-8f08-f310b5da5b3a.
[   96.734976] EXT4-fs (mmcblk0p5): mounted filesystem b6a31322-040c-46db-8519-38fb5658ec3e r/w with ordered data mode. Quota mode: disabled.
total 68
lrwxrwxrwx    1 root     root             7 Mar  9  2018 bin -> usr/bin
drwxr-xr-x    2 root     root          4096 Mar  9  2018 boot
drwxr-xr-x    2 root     root          4096 Mar  9  2018 dev
drwxr-xr-x   35 root     root          4096 Mar  9  2018 etc
drwxr-xr-x    4 root     root          4096 Mar  9  2018 home
lrwxrwxrwx    1 root     root             7 Mar  9  2018 lib -> usr/lib
drwx------    2 root     root         16384 Apr  5  2011 lost+found
drwxr-xr-x    2 root     root          4096 Mar  9  2018 media
drwxr-xr-x    2 root     root          4096 Mar  9  2018 mnt
dr-xr-xr-x    2 root     root          4096 Mar  9  2018 proc
drwxr-xr-x    2 root     root          4096 Mar  9  2018 run
lrwxrwxrwx    1 root     root             8 Mar  9  2018 sbin -> usr/sbin
drwxr-xr-x    2 root     root          4096 Mar  9  2018 srv
dr-xr-xr-x    2 root     root          4096 Mar  9  2018 sys
drwxrwxrwt    2 root     root          4096 Mar  9  2018 tmp
drwxr-xr-x   11 root     root          4096 Mar  9  2018 usr
drwxr-xr-x   11 root     root          4096 Mar  9  2018 var
[   96.846820] EXT4-fs (mmcblk0p5): unmounting filesystem b6a31322-040c-46db-8519-38fb5658ec3e.
[   97.229572] EXT4-fs (mmcblk0p6): mounted filesystem 6dc9b0da-2b0f-4822-9eac-df4dd782ddfc r/w with ordered data mode. Quota mode: disabled.
total 16
drwx------    2 root     root         16384 Jun 18  2024 lost+found
[   97.269409] EXT4-fs (mmcblk0p6): unmounting filesystem 6dc9b0da-2b0f-4822-9eac-df4dd782ddfc.
[   97.551223] EXT4-fs (mmcblk0p7): mounted filesystem 1552b5da-c1d9-4835-84c5-f92c376744ec r/w with ordered data mode. Quota mode: disabled.
total 16
drwx------    2 root     root         16384 Apr  5  2011 lost+found
[   97.588958] EXT4-fs (mmcblk0p7): unmounting filesystem 1552b5da-c1d9-4835-84c5-f92c376744ec.

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

[Guenter Roeck]

On 10/26/24 03:02, Cédric Le Goater wrote:
[ ... ]

>> I don't mind a single file. What bothers me is that the partitioning is made
>> mandatory for ast2600 even if not used.
> 
> Our only use case, in 2019, was to boot QEMU ast2600 machines from an
> eMMC device using an OpenBMC FW image like the ones we find on IBM
> Power10 Rainier systems. I agree we only focused on this scenario.
> Most of the support should be there for other use cases, and it's now
> a question of finding the right tunables for the user.
> 
> I did a quick experiment using 2 patches,
> 
> one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
> area in emmc image")
> 
>      @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>               sect = 0;
>           }
>           size = sect << HWBLOCK_SHIFT;
>      -    size -= sd_bootpart_offset(sd);
>      +    if (sd_is_emmc(sd)) {
>      +        size -= sd->boot_part_size * 2;
>      +    }
>           sect = sd_addr_to_wpnum(size) + 1;
> 
> and another on hw/arm/aspeed.c to remove the setting of the eMMC
> device properties when it is not bootable :
>      @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
>                   return;
>               }
>               card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>      -        if (emmc) {
>      +        if (emmc && boot_emmc) {
>                   qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>                   qdev_prop_set_uint8(card, "boot-config",
>                                       boot_emmc ? 0x1 << 3 : 0x0);
> (I am not saying this is correct)
> 

Works for me, though, and it is much better than mandating the existence
of boot partitions.

If you end up submitting those patches, please feel free to add

Tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

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

[Cédric Le Goater]

On 10/26/24 17:32, Guenter Roeck wrote:
> On 10/26/24 03:02, Cédric Le Goater wrote:
> [ ... ]
> 
>>> I don't mind a single file. What bothers me is that the partitioning is made
>>> mandatory for ast2600 even if not used.
>>
>> Our only use case, in 2019, was to boot QEMU ast2600 machines from an
>> eMMC device using an OpenBMC FW image like the ones we find on IBM
>> Power10 Rainier systems. I agree we only focused on this scenario.
>> Most of the support should be there for other use cases, and it's now
>> a question of finding the right tunables for the user.
>>
>> I did a quick experiment using 2 patches,
>>
>> one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
>> area in emmc image")
>>
>>      @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>>               sect = 0;
>>           }
>>           size = sect << HWBLOCK_SHIFT;
>>      -    size -= sd_bootpart_offset(sd);
>>      +    if (sd_is_emmc(sd)) {
>>      +        size -= sd->boot_part_size * 2;
>>      +    }
>>           sect = sd_addr_to_wpnum(size) + 1;
>>
>> and another on hw/arm/aspeed.c to remove the setting of the eMMC
>> device properties when it is not bootable :
>>      @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
>>                   return;
>>               }
>>               card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>      -        if (emmc) {
>>      +        if (emmc && boot_emmc) {
>>                   qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>>                   qdev_prop_set_uint8(card, "boot-config",
>>                                       boot_emmc ? 0x1 << 3 : 0x0);
>> (I am not saying this is correct)
>>
> 
> Works for me, though, and it is much better than mandating the existence
> of boot partitions.

Yes. However, if the emmc device was user creatable, we could use :

   -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
   -device emmc,bus=sdhci-bus.2,drive=emmc0

and with boot partitions:

   -M boot-emmc=true \
   -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
   -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8

The above would be my preferred approach if acceptable. The "sd-bus"
bus identifier should be changed in other machines tough.

> If you end up submitting those patches, please feel free to add
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

I should send these fixes for QEMU 9.2.

Thanks,

C.

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

[Guenter Roeck]

On 10/27/24 14:13, Cédric Le Goater wrote:
> On 10/26/24 17:32, Guenter Roeck wrote:
>> On 10/26/24 03:02, Cédric Le Goater wrote:
>> [ ... ]
>>
>>>> I don't mind a single file. What bothers me is that the partitioning is made
>>>> mandatory for ast2600 even if not used.
>>>
>>> Our only use case, in 2019, was to boot QEMU ast2600 machines from an
>>> eMMC device using an OpenBMC FW image like the ones we find on IBM
>>> Power10 Rainier systems. I agree we only focused on this scenario.
>>> Most of the support should be there for other use cases, and it's now
>>> a question of finding the right tunables for the user.
>>>
>>> I did a quick experiment using 2 patches,
>>>
>>> one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
>>> area in emmc image")
>>>
>>>      @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>>>               sect = 0;
>>>           }
>>>           size = sect << HWBLOCK_SHIFT;
>>>      -    size -= sd_bootpart_offset(sd);
>>>      +    if (sd_is_emmc(sd)) {
>>>      +        size -= sd->boot_part_size * 2;
>>>      +    }
>>>           sect = sd_addr_to_wpnum(size) + 1;
>>>
>>> and another on hw/arm/aspeed.c to remove the setting of the eMMC
>>> device properties when it is not bootable :
>>>      @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
>>>                   return;
>>>               }
>>>               card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>      -        if (emmc) {
>>>      +        if (emmc && boot_emmc) {
>>>                   qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>>>                   qdev_prop_set_uint8(card, "boot-config",
>>>                                       boot_emmc ? 0x1 << 3 : 0x0);
>>> (I am not saying this is correct)
>>>
>>
>> Works for me, though, and it is much better than mandating the existence
>> of boot partitions.
> 
> Yes. However, if the emmc device was user creatable, we could use :
> 
>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
>    -device emmc,bus=sdhci-bus.2,drive=emmc0
> 
> and with boot partitions:
> 
>    -M boot-emmc=true \
>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
>    -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
> 
> The above would be my preferred approach if acceptable. The "sd-bus"
> bus identifier should be changed in other machines tough.
> 

No real preference here, though my understanding is that emmc devices
are by definition built-in, and that is what emmc_class_init() says as well.
Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
not support any index values. That may be just my lack of knowledge, though.

Guenter

>> If you end up submitting those patches, please feel free to add
>>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> I should send these fixes for QEMU 9.2.
> 
> Thanks,
> 
> C.
> 

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

[Cédric Le Goater]

On 10/27/24 23:11, Guenter Roeck wrote:
> On 10/27/24 14:13, Cédric Le Goater wrote:
>> On 10/26/24 17:32, Guenter Roeck wrote:
>>> On 10/26/24 03:02, Cédric Le Goater wrote:
>>> [ ... ]
>>>
>>>>> I don't mind a single file. What bothers me is that the partitioning is made
>>>>> mandatory for ast2600 even if not used.
>>>>
>>>> Our only use case, in 2019, was to boot QEMU ast2600 machines from an
>>>> eMMC device using an OpenBMC FW image like the ones we find on IBM
>>>> Power10 Rainier systems. I agree we only focused on this scenario.
>>>> Most of the support should be there for other use cases, and it's now
>>>> a question of finding the right tunables for the user.
>>>>
>>>> I did a quick experiment using 2 patches,
>>>>
>>>> one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
>>>> area in emmc image")
>>>>
>>>>      @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>>>>               sect = 0;
>>>>           }
>>>>           size = sect << HWBLOCK_SHIFT;
>>>>      -    size -= sd_bootpart_offset(sd);
>>>>      +    if (sd_is_emmc(sd)) {
>>>>      +        size -= sd->boot_part_size * 2;
>>>>      +    }
>>>>           sect = sd_addr_to_wpnum(size) + 1;
>>>>
>>>> and another on hw/arm/aspeed.c to remove the setting of the eMMC
>>>> device properties when it is not bootable :
>>>>      @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
>>>>                   return;
>>>>               }
>>>>               card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>      -        if (emmc) {
>>>>      +        if (emmc && boot_emmc) {
>>>>                   qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>>>>                   qdev_prop_set_uint8(card, "boot-config",
>>>>                                       boot_emmc ? 0x1 << 3 : 0x0);
>>>> (I am not saying this is correct)
>>>>
>>>
>>> Works for me, though, and it is much better than mandating the existence
>>> of boot partitions.
>>
>> Yes. However, if the emmc device was user creatable, we could use :
>>
>>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
>>    -device emmc,bus=sdhci-bus.2,drive=emmc0
>>
>> and with boot partitions:
>>
>>    -M boot-emmc=true \
>>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
>>    -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
>>
>> The above would be my preferred approach if acceptable. The "sd-bus"
>> bus identifier should be changed in other machines tough.
>>
> 
> No real preference here, though my understanding is that emmc devices
> are by definition built-in, and that is what emmc_class_init() says as well.
> Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
> not support any index values. That may be just my lack of knowledge, though.


No, you are right. On a real ast2600-evb, the eMMC device is indeed
soldered on the board. But, for testing purposes, it is sometime
interesting to add some flexibility in the machine definition and
in the modeling too. This avoids "hard-coding" default devices in
the machines and lets the user define its own variant models using
the QEMU command line.

C.

> Guenter
> 
>>> If you end up submitting those patches, please feel free to add
>>>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>
>> I should send these fixes for QEMU 9.2.
>>
>> Thanks,
>>
>> C.
>>
> 

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

[Guenter Roeck]

On 10/27/24 15:26, Cédric Le Goater wrote:
> On 10/27/24 23:11, Guenter Roeck wrote:
>> On 10/27/24 14:13, Cédric Le Goater wrote:
>>> On 10/26/24 17:32, Guenter Roeck wrote:
>>>> On 10/26/24 03:02, Cédric Le Goater wrote:
>>>> [ ... ]
>>>>
>>>>>> I don't mind a single file. What bothers me is that the partitioning is made
>>>>>> mandatory for ast2600 even if not used.
>>>>>
>>>>> Our only use case, in 2019, was to boot QEMU ast2600 machines from an
>>>>> eMMC device using an OpenBMC FW image like the ones we find on IBM
>>>>> Power10 Rainier systems. I agree we only focused on this scenario.
>>>>> Most of the support should be there for other use cases, and it's now
>>>>> a question of finding the right tunables for the user.
>>>>>
>>>>> I did a quick experiment using 2 patches,
>>>>>
>>>>> one on hw/sd/sd.c to fix c8cb19876d3e ("hw/sd/sdcard: Support boot
>>>>> area in emmc image")
>>>>>
>>>>>      @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>>>>>               sect = 0;
>>>>>           }
>>>>>           size = sect << HWBLOCK_SHIFT;
>>>>>      -    size -= sd_bootpart_offset(sd);
>>>>>      +    if (sd_is_emmc(sd)) {
>>>>>      +        size -= sd->boot_part_size * 2;
>>>>>      +    }
>>>>>           sect = sd_addr_to_wpnum(size) + 1;
>>>>>
>>>>> and another on hw/arm/aspeed.c to remove the setting of the eMMC
>>>>> device properties when it is not bootable :
>>>>>      @@ -338,7 +338,7 @@ static void sdhci_attach_drive(SDHCIStat
>>>>>                   return;
>>>>>               }
>>>>>               card = qdev_new(emmc ? TYPE_EMMC : TYPE_SD_CARD);
>>>>>      -        if (emmc) {
>>>>>      +        if (emmc && boot_emmc) {
>>>>>                   qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
>>>>>                   qdev_prop_set_uint8(card, "boot-config",
>>>>>                                       boot_emmc ? 0x1 << 3 : 0x0);
>>>>> (I am not saying this is correct)
>>>>>
>>>>
>>>> Works for me, though, and it is much better than mandating the existence
>>>> of boot partitions.
>>>
>>> Yes. However, if the emmc device was user creatable, we could use :
>>>
>>>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
>>>    -device emmc,bus=sdhci-bus.2,drive=emmc0
>>>
>>> and with boot partitions:
>>>
>>>    -M boot-emmc=true \
>>>    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
>>>    -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
>>>
>>> The above would be my preferred approach if acceptable. The "sd-bus"
>>> bus identifier should be changed in other machines tough.
>>>
>>
>> No real preference here, though my understanding is that emmc devices
>> are by definition built-in, and that is what emmc_class_init() says as well.
>> Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
>> not support any index values. That may be just my lack of knowledge, though.
> 
> 
> No, you are right. On a real ast2600-evb, the eMMC device is indeed
> soldered on the board. But, for testing purposes, it is sometime
> interesting to add some flexibility in the machine definition and
> in the modeling too. This avoids "hard-coding" default devices in
> the machines and lets the user define its own variant models using
> the QEMU command line.
> 

I would agree, but I had a number of my patches rejected because while
they would be useful for testing they would not accurately reflect the
hardware. So nowadays I gave up even trying to upstream such changes.

Guenter

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

[Jan Lübbe]

On Sun, 2024-10-27 at 20:32 -0700, Guenter Roeck wrote:
> On 10/27/24 15:26, Cédric Le Goater wrote:
> > On 10/27/24 23:11, Guenter Roeck wrote:
> > > On 10/27/24 14:13, Cédric Le Goater wrote:
> > > > On 10/26/24 17:32, Guenter Roeck wrote:
> > > > > On 10/26/24 03:02, Cédric Le Goater wrote:
> > > > > [ ... ]
> > > > > 
> > > > > > 
> > > > > Works for me, though, and it is much better than mandating the existence
> > > > > of boot partitions.
> > > > 
> > > > Yes. However, if the emmc device was user creatable, we could use :
> > > > 
> > > >    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
> > > >    -device emmc,bus=sdhci-bus.2,drive=emmc0
> > > > 
> > > > and with boot partitions:
> > > > 
> > > >    -M boot-emmc=true \
> > > >    -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
> > > >    -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
> > > > 
> > > > The above would be my preferred approach if acceptable. The "sd-bus"
> > > > bus identifier should be changed in other machines tough.
> > > 
> > > No real preference here, though my understanding is that emmc devices
> > > are by definition built-in, and that is what emmc_class_init() says as well.
> > > Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
> > > not support any index values. That may be just my lack of knowledge, though.
> > 
> > No, you are right. On a real ast2600-evb, the eMMC device is indeed
> > soldered on the board. But, for testing purposes, it is sometime
> > interesting to add some flexibility in the machine definition and
> > in the modeling too. This avoids "hard-coding" default devices in
> > the machines and lets the user define its own variant models using
> > the QEMU command line.
> 
> I would agree, but I had a number of my patches rejected because while
> they would be useful for testing they would not accurately reflect the
> hardware. So nowadays I gave up even trying to upstream such changes.

My patch to make eMMCs user creatable [1] was applied to target-
arm.next by Peter Maydell [2] last week.

Jan

[1] https://lore.kernel.org/qemu-devel/20241015135649.4189256-1-jlu@pengutronix.de/
[2] https://lore.kernel.org/qemu-devel/CAFEAcA9sjszCj=Fu-A-=qQV_jawnomJ-Nqnd=Vx2vLKmYZ1-nQ@mail.gmail.com/

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

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

[Guenter Roeck]

On 10/28/24 01:41, Jan Lübbe wrote:
> On Sun, 2024-10-27 at 20:32 -0700, Guenter Roeck wrote:
>> On 10/27/24 15:26, Cédric Le Goater wrote:
>>> On 10/27/24 23:11, Guenter Roeck wrote:
>>>> On 10/27/24 14:13, Cédric Le Goater wrote:
>>>>> On 10/26/24 17:32, Guenter Roeck wrote:
>>>>>> On 10/26/24 03:02, Cédric Le Goater wrote:
>>>>>> [ ... ]
>>>>>>
>>>>>>>
>>>>>> Works for me, though, and it is much better than mandating the existence
>>>>>> of boot partitions.
>>>>>
>>>>> Yes. However, if the emmc device was user creatable, we could use :
>>>>>
>>>>>     -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
>>>>>     -device emmc,bus=sdhci-bus.2,drive=emmc0
>>>>>
>>>>> and with boot partitions:
>>>>>
>>>>>     -M boot-emmc=true \
>>>>>     -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
>>>>>     -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
>>>>>
>>>>> The above would be my preferred approach if acceptable. The "sd-bus"
>>>>> bus identifier should be changed in other machines tough.
>>>>
>>>> No real preference here, though my understanding is that emmc devices
>>>> are by definition built-in, and that is what emmc_class_init() says as well.
>>>> Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
>>>> not support any index values. That may be just my lack of knowledge, though.
>>>
>>> No, you are right. On a real ast2600-evb, the eMMC device is indeed
>>> soldered on the board. But, for testing purposes, it is sometime
>>> interesting to add some flexibility in the machine definition and
>>> in the modeling too. This avoids "hard-coding" default devices in
>>> the machines and lets the user define its own variant models using
>>> the QEMU command line.
>>
>> I would agree, but I had a number of my patches rejected because while
>> they would be useful for testing they would not accurately reflect the
>> hardware. So nowadays I gave up even trying to upstream such changes.
> 
> My patch to make eMMCs user creatable [1] was applied to target-
> arm.next by Peter Maydell [2] last week.
> 

That works for me with

drivecmd="-drive file=${ROOTFS},format=raw,if=none,id=d0"
drivecmd+=" -device emmc,drive=d0"

but unless I am missing something

drivecmd="-drive file=${ROOTFS},format=raw,if=none,id=d0"
drivecmd+=" -device sd-card,drive=d0"

also boots from the emmc controller. How do I provide the
bus and bus index ? "bus=sdhci-bus.2" doesn't work for me.
There is "sd-bus", but it does not have an index.

Thanks,
Guenter

backing storage for eMMC boot partitions

[Jan Lübbe]

On Tue, 2024-10-29 at 07:40 -0700, Guenter Roeck wrote:
> On 10/28/24 01:41, Jan Lübbe wrote:
> > On Sun, 2024-10-27 at 20:32 -0700, Guenter Roeck wrote:
> > > On 10/27/24 15:26, Cédric Le Goater wrote:
> > > > On 10/27/24 23:11, Guenter Roeck wrote:
> > > > > On 10/27/24 14:13, Cédric Le Goater wrote:
> > > > > > On 10/26/24 17:32, Guenter Roeck wrote:
> > > > > > > On 10/26/24 03:02, Cédric Le Goater wrote:
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > 
> > > > > > > Works for me, though, and it is much better than mandating the existence
> > > > > > > of boot partitions.
> > > > > > 
> > > > > > Yes. However, if the emmc device was user creatable, we could use :
> > > > > > 
> > > > > >     -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb-noboot.raw \
> > > > > >     -device emmc,bus=sdhci-bus.2,drive=emmc0
> > > > > > 
> > > > > > and with boot partitions:
> > > > > > 
> > > > > >     -M boot-emmc=true \
> > > > > >     -blockdev node-name=emmc0,driver=file,filename=mmc-ast2600-evb.raw \
> > > > > >     -device emmc,bus=sdhci-bus.2,drive=emmc0,boot-partition-size=1048576,boot-config=8
> > > > > > 
> > > > > > The above would be my preferred approach if acceptable. The "sd-bus"
> > > > > > bus identifier should be changed in other machines tough.
> > > > > 
> > > > > No real preference here, though my understanding is that emmc devices
> > > > > are by definition built-in, and that is what emmc_class_init() says as well.
> > > > > Also, there does not seem to be an sdhci-bus, only sd-bus, and that does
> > > > > not support any index values. That may be just my lack of knowledge, though.
> > > > 
> > > > No, you are right. On a real ast2600-evb, the eMMC device is indeed
> > > > soldered on the board. But, for testing purposes, it is sometime
> > > > interesting to add some flexibility in the machine definition and
> > > > in the modeling too. This avoids "hard-coding" default devices in
> > > > the machines and lets the user define its own variant models using
> > > > the QEMU command line.
> > > 
> > > I would agree, but I had a number of my patches rejected because while
> > > they would be useful for testing they would not accurately reflect the
> > > hardware. So nowadays I gave up even trying to upstream such changes.
> > 
> > My patch to make eMMCs user creatable [1] was applied to target-
> > arm.next by Peter Maydell [2] last week.
> > 
> 
> That works for me with
> 
> drivecmd="-drive file=${ROOTFS},format=raw,if=none,id=d0"
> drivecmd+=" -device emmc,drive=d0"
> 
> but unless I am missing something
> 
> drivecmd="-drive file=${ROOTFS},format=raw,if=none,id=d0"
> drivecmd+=" -device sd-card,drive=d0"
> 
> also boots from the emmc controller. How do I provide the
> bus and bus index ? "bus=sdhci-bus.2" doesn't work for me.
> There is "sd-bus", but it does not have an index.

I've not really understood how to assemble more complex setups using
qemu's commandline when the board already creates some of the devices. 

Perhaps Cédric can explain how the different boot options are
configured for aspeed?

I see three cases:
1. specify the blockdev driver and options in the simple case where the
board already creates the SD or eMMC device
2. specify some custom options for the eMMC
3. create a custom eMMC config on a generic machine via sdhci-pci


Case 1 is probably most common. The user has chosen a board and just
wants to boot a rootfs image and doesn't care about boot partitions or
anything else eMMC-specific.


Some users may want to emulate an eMMC with boot partitions, as that
allows them to emulate their physical boards more closely (case 2).
Note that eMMC boot partitions are usually *not* used for storing a
Linux kernel, but for the bootloader (including things like u-boot, TF-
A, OP-TEE, ...). The ROM-code on many SoCs supports loading directly
from eMMC boot partitions. One of the two boot partitions can be
activated with an atomic eMMC EXT CSD register write, allowing atomic
bootloader updates. I think this case was the motivation for Cédric's
eea55625df83 ("aspeed: Introduce a AspeedSoCClass 'boot_from_emmc'
handler").

These users are likely fine with assembling a backing file consisting
of e.g.
- bootloader image (boot0) @ offset 0MiB
- empty space for bootloader updates (boot1) @ offset 1MiB
- partitioned disk image (rootfs, ...) @ offset 2MiB
to get the same setup as their real hardware.


Case 3 is what I want to use the eMMC emulator for: Test eMMC-specific
functionality in Linux userspace, specifically the boot partition
update backend for RAUC, in a CI setup. To improve performance and
because we don't need to emulate any specific board for CI, we use an
x86 guest (q35). As it has PCIe, the easiest way to add the necessary
eMMC emulation is to use sdhci-pci. That was the motivation behind my
patch "hw/sd/sdcard: Allow user creation of eMMCs" [1].

For that case, having one backing file for boot partitions + main area
is fine as well.


If we wanted more flexibility via separate backing files per eMMC
partitions, it might work similar to NVMe Namespaces [2]. For me, that
seems like a lot of complexity a very niche case like eMMC boot
partitions.


Potential future features such as more eMMC data partitions, RPMB
support or separate backing files could be support in QEMU by new eMCC
device options or even additional devices (following the NVMe
approach), without breaking backwards compatibility.

So it seems to me, that Cédric's approach of enabling boot partitions
in hw/arm/aspeed.c only when configured to boot from them via the "hw-
strap1" property should solve cases 1 and 2 without introducing
backwards compatibility issues. Case 3 has explicit configuration (if a
boot partition is emulated), so shouldn't be a problem either.


Thanks,
Jan

[1] https://lore.kernel.org/qemu-devel/20241015135649.4189256-1-jlu@pengutronix.de/T/
[2] https://qemu-project.gitlab.io/qemu/system/devices/nvme.html#additional-namespaces

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

Re: backing storage for eMMC boot partitions

[Cédric Le Goater]

Hello,

[ ... ]

>> also boots from the emmc controller. How do I provide the
>> bus and bus index ? "bus=sdhci-bus.2" doesn't work for me.
>> There is "sd-bus", but it does not have an index.

Yes. Changes are required in the sdhci model and in the boards using it.

> I've not really understood how to assemble more complex setups using
> qemu's commandline when the board already creates some of the devices.

Yes. That's a "problem" to fix for emmc/sd devices. They should only
be created when defaults_enabled(), just like the flash devices.

> Perhaps Cédric can explain how the different boot options are
> configured for aspeed?

See https://qemu.readthedocs.io/en/v9.1.0/system/arm/aspeed.html#boot-options
and the avocado/functional tests for examples.

The eMMC case is missing from the docs. Here is a proposal :

   https://lore.kernel.org/qemu-devel/20241118090648.187720-1-clg@redhat.com/

Thanks,

C.


> 
> I see three cases:
> 1. specify the blockdev driver and options in the simple case where the
> board already creates the SD or eMMC device
> 2. specify some custom options for the eMMC
> 3. create a custom eMMC config on a generic machine via sdhci-pci
> 
> 
> Case 1 is probably most common. The user has chosen a board and just
> wants to boot a rootfs image and doesn't care about boot partitions or
> anything else eMMC-specific.
> 
> 
> Some users may want to emulate an eMMC with boot partitions, as that
> allows them to emulate their physical boards more closely (case 2).
> Note that eMMC boot partitions are usually *not* used for storing a
> Linux kernel, but for the bootloader (including things like u-boot, TF-
> A, OP-TEE, ...). The ROM-code on many SoCs supports loading directly
> from eMMC boot partitions. One of the two boot partitions can be
> activated with an atomic eMMC EXT CSD register write, allowing atomic
> bootloader updates. I think this case was the motivation for Cédric's
> eea55625df83 ("aspeed: Introduce a AspeedSoCClass 'boot_from_emmc'
> handler").
>
> 
> These users are likely fine with assembling a backing file consisting
> of e.g.
> - bootloader image (boot0) @ offset 0MiB
> - empty space for bootloader updates (boot1) @ offset 1MiB
> - partitioned disk image (rootfs, ...) @ offset 2MiB
> to get the same setup as their real hardware.
> 
> 
> Case 3 is what I want to use the eMMC emulator for: Test eMMC-specific
> functionality in Linux userspace, specifically the boot partition
> update backend for RAUC, in a CI setup. To improve performance and
> because we don't need to emulate any specific board for CI, we use an
> x86 guest (q35). As it has PCIe, the easiest way to add the necessary
> eMMC emulation is to use sdhci-pci. That was the motivation behind my
> patch "hw/sd/sdcard: Allow user creation of eMMCs" [1].
> 
> For that case, having one backing file for boot partitions + main area
> is fine as well.
> 
> 
> If we wanted more flexibility via separate backing files per eMMC
> partitions, it might work similar to NVMe Namespaces [2]. For me, that
> seems like a lot of complexity a very niche case like eMMC boot
> partitions.
> 
> 
> Potential future features such as more eMMC data partitions, RPMB
> support or separate backing files could be support in QEMU by new eMCC
> device options or even additional devices (following the NVMe
> approach), without breaking backwards compatibility.
> 
> So it seems to me, that Cédric's approach of enabling boot partitions
> in hw/arm/aspeed.c only when configured to boot from them via the "hw-
> strap1" property should solve cases 1 and 2 without introducing
> backwards compatibility issues. Case 3 has explicit configuration (if a
> boot partition is emulated), so shouldn't be a problem either.
> 
> 
> Thanks,
> Jan
> 
> [1] https://lore.kernel.org/qemu-devel/20241015135649.4189256-1-jlu@pengutronix.de/T/
> [2] https://qemu-project.gitlab.io/qemu/system/devices/nvme.html#additional-namespaces
> 

Re: backing storage for eMMC boot partitions

[Jan Lübbe]

Hi,

On Mon, 2024-11-18 at 10:14 +0100, Cédric Le Goater wrote:
> > > also boots from the emmc controller. How do I provide the
> > > bus and bus index ? "bus=sdhci-bus.2" doesn't work for me.
> > > There is "sd-bus", but it does not have an index.
> 
> Yes. Changes are required in the sdhci model and in the boards using
> it.
> 
> > I've not really understood how to assemble more complex setups
> > using qemu's commandline when the board already creates some of the
> > devices.
> 
> Yes. That's a "problem" to fix for emmc/sd devices. They should only
> be created when defaults_enabled(), just like the flash devices.

This is -nodefaults, right? The man-page for that says:
  -nodefaults
         Don't  create default devices. Normally, QEMU sets the de‐
         fault devices like serial  port,  parallel  port,  virtual
         console,  monitor  device,  VGA adapter, floppy and CD-ROM
         drive and others. The -nodefaults option will disable  all
         those default devices.

That sounds like it would require explicit configuration of the devices
on the SoC (such as UARTs) as well? Having to specify them (perhaps
even with their MMIO addresses) doesn't seem convenient. :)

aspeed seems to only (or additionally?) skip creation of the flash
devices with -nodefaults.

Isn't there a way to configure only default devices as disabled? Or to
dynamically attach a user-created SD/eMMC device to an existing SD
controller?

In system/vl.c, I found 'default_sdcard', which is disabled in some
cases. I wasn't able to figure out how to use it or if it would help
for this use-case.

Jan

> > Perhaps Cédric can explain how the different boot options are
> > configured for aspeed?
> 
> See
> https://qemu.readthedocs.io/en/v9.1.0/system/arm/aspeed.html#boot-options
> and the avocado/functional tests for examples.
> 
> The eMMC case is missing from the docs. Here is a proposal :
> https://lore.kernel.org/qemu-devel/20241118090648.187720-1-clg@redhat.com/
> 
> Thanks,
> 
> C.

Re: backing storage for eMMC boot partitions

[Cédric Le Goater]

>> Yes. That's a "problem" to fix for emmc/sd devices. They should only
>> be created when defaults_enabled(), just like the flash devices.
> 
> This is -nodefaults, right? The man-page for that says:
>    -nodefaults
>           Don't  create default devices. Normally, QEMU sets the de‐
>           fault devices like serial  port,  parallel  port,  virtual
>           console,  monitor  device,  VGA adapter, floppy and CD-ROM
>           drive and others. The -nodefaults option will disable  all
>           those default devices.
> 
> That sounds like it would require explicit configuration of the devices
> on the SoC (such as UARTs) as well? Having to specify them (perhaps
> even with their MMIO addresses) doesn't seem convenient. :)

I don't think the -nodefaults option applies to the platform devices.
These are part of the SoC and can not be replaced/removed on a real
config. However, any device attached to some bus, flash, eMMC, I2C,
PCI, etc. is a good candidate. Some would say that these devices
are soldered on the board. This is not always the case (some are in
sockets) and since HW vendors offer boards with different topologies,
I think  we can leverage QEMU to define custom boards on the command
line.

> aspeed seems to only (or additionally?) skip creation of the flash
> devices with -nodefaults.

Yes. Flash devices were a good (first) candidate. The goal was to
extend testing to all SPI controllers (some SoCs have 8) and also
to limit the proliferation of boards having only one different
flash model.

There is a cost to it :

   - it complexifies the command line
   - the machine boots in "execute-in-place" mode which is what HW
     does, so the model is much more precise, but it is also slower
     in emulation.

We thought about doing the same for I2C. Given the amount of I2C
devices on some boards, it didn't seem reasonable and the need
for it was limited. We can still attach on the command line I2C
devices and discover them after boot.

> Isn't there a way to configure only default devices as disabled? 

That's -nodefaults or I am misunderstanding. Am I ?

> Or to
> dynamically attach a user-created SD/eMMC device to an existing SD
> controller?

QEMU lacks a way to identify the SD bus the user wants to attach the
device to.

> In system/vl.c, I found 'default_sdcard', which is disabled in some
> cases. I wasn't able to figure out how to use it or if it would help
> for this use-case.

May be Philippe can answer. I am not an sd person.

Thanks,

C.

[PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Jan Luebbe]

The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.

Instead, subtract the boot_part_size directly (twice, as there are two
identical boot partitions defined by the eMMC spec).

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
---
 hw/sd/sd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2d3467c3d956..8430d5ae361c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
         sect = 0;
     }
     size = sect << HWBLOCK_SHIFT;
-    size -= sd_bootpart_offset(sd);
+    if (sd_is_emmc(sd)) {
+        size -= sd->boot_part_size * 2;
+    }
 
     sect = sd_addr_to_wpnum(size) + 1;
 
-- 
2.39.5

Re: [PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Cédric Le Goater]

On 10/28/24 17:23, Jan Luebbe wrote:
> The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.
> 
> Instead, subtract the boot_part_size directly (twice, as there are two
> identical boot partitions defined by the eMMC spec).

Fixes: c8cb19876d3e ("hw/sd/sdcard: Support boot area in emmc image")

> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>

Tested-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   hw/sd/sd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2d3467c3d956..8430d5ae361c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>           sect = 0;
>       }
>       size = sect << HWBLOCK_SHIFT;
> -    size -= sd_bootpart_offset(sd);
> +    if (sd_is_emmc(sd)) {
> +        size -= sd->boot_part_size * 2;
> +    }
>   
>       sect = sd_addr_to_wpnum(size) + 1;
>   

Re: [PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Cédric Le Goater]

Philippe,

I can take this patch through the aspeed branch.

Thanks,

C.

On 10/28/24 17:23, Jan Luebbe wrote:
> The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.
> 
> Instead, subtract the boot_part_size directly (twice, as there are two
> identical boot partitions defined by the eMMC spec).
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> --->   hw/sd/sd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2d3467c3d956..8430d5ae361c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>           sect = 0;
>       }
>       size = sect << HWBLOCK_SHIFT;
> -    size -= sd_bootpart_offset(sd);
> +    if (sd_is_emmc(sd)) {
> +        size -= sd->boot_part_size * 2;
> +    }
>   
>       sect = sd_addr_to_wpnum(size) + 1;
>   

Re: [PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Philippe Mathieu-Daudé]

On 2/11/24 12:06, Cédric Le Goater wrote:
> Philippe,
> 
> I can take this patch through the aspeed branch.

I missed this patch because it was deeply buried in a long thread.

Since you mentioned this feature will be reworked in the next cycle:

Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 
> Thanks,
> 
> C.
> 
> On 10/28/24 17:23, Jan Luebbe wrote:
>> The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.
>>
>> Instead, subtract the boot_part_size directly (twice, as there are two
>> identical boot partitions defined by the eMMC spec).
>>
>> Suggested-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
>> --->   hw/sd/sd.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 2d3467c3d956..8430d5ae361c 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>>           sect = 0;
>>       }
>>       size = sect << HWBLOCK_SHIFT;
>> -    size -= sd_bootpart_offset(sd);
>> +    if (sd_is_emmc(sd)) {
>> +        size -= sd->boot_part_size * 2;
>> +    }
>>       sect = sd_addr_to_wpnum(size) + 1;
> 

Re: [PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Michael Tokarev]

28.10.2024 19:23, Jan Luebbe wrote:
> The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.
> 
> Instead, subtract the boot_part_size directly (twice, as there are two
> identical boot partitions defined by the eMMC spec).

Should I pick this up for stable-9.1 branch?

Thanks,

/mjt

> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> ---
>   hw/sd/sd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2d3467c3d956..8430d5ae361c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -826,7 +826,9 @@ static void sd_reset(DeviceState *dev)
>           sect = 0;
>       }
>       size = sect << HWBLOCK_SHIFT;
> -    size -= sd_bootpart_offset(sd);
> +    if (sd_is_emmc(sd)) {
> +        size -= sd->boot_part_size * 2;
> +    }
>   
>       sect = sd_addr_to_wpnum(size) + 1;

Re: [PATCH] hw/sd/sdcard: Fix calculation of size when using eMMC boot partitions

[Cédric Le Goater]

On 11/5/24 17:13, Michael Tokarev wrote:
> 28.10.2024 19:23, Jan Luebbe wrote:
>> The sd_bootpart_offset() 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, so it shouldn't be used to calculate the main user data area size.
>>
>> Instead, subtract the boot_part_size directly (twice, as there are two
>> identical boot partitions defined by the eMMC spec).
> 
> Should I pick this up for stable-9.1 branch?

yes.

Thanks,

C.