[Merge] ~alexghiti/livecd-rootfs:int/alex/riscv_mpfs into livecd-rootfs:ubuntu/master
Łukasz Zemczak
mp+429819 at code.launchpad.net
Mon Sep 19 09:21:02 UTC 2022
Review: Needs Fixing
In principle looks good, but we need to fix it up a bit (more information inline). With such changes we always need to be aware of other devices like cloud images - and those use the same project as the preinstalled images we create. We already once broke them, so let's try not to do it again!
I also have some stylistic suggestions and such.
Diff comments:
> diff --git a/live-build/auto/config b/live-build/auto/config
> index c480215..0c11007 100755
> --- a/live-build/auto/config
> +++ b/live-build/auto/config
> @@ -352,7 +352,7 @@ fi
> # one also must request disk1-img-xz image format
> if [ "$IMAGEFORMAT" = "ext4" ] && [ "$PROJECT" = "ubuntu-cpc" ]; then
> case $ARCH:$SUBARCH in
> - armhf:raspi2|riscv64:sifive_*|riscv64:nezha|riscv64:visionfive|*:generic)
> + armhf:raspi2|riscv64:*|*:generic)
I don't think we can do it like this, we can't simply switch to a riscv64:* wildcard. We also produce riscv64 cloud images which currently don't do this configuration here and riscv64:* would catch SUBARCH which is empty. I think you simply need to add the two new SUBARCHes there.
> IMAGE_HAS_HARDCODED_PASSWORD=1
> if [ -z "${IMAGE_TARGETS:-}" ]; then
> export IMAGE_TARGETS="disk1-img-xz"
> @@ -969,7 +969,7 @@ case $PROJECT in
> riscv64*)
> if [ -n "$SUBARCH" ]; then
> case "${SUBARCH:-}" in
> - nezha)
> + nezha | licheerv)
To stay consistent, I'd change this to no-whitespaces in the conditional (so "nezha|licheerv)").
> KERNEL_FLAVOURS=allwinner
> ;;
> visionfive)
> @@ -1109,6 +1109,9 @@ case "$ARCH${SUBARCH:++$SUBARCH}" in
> BINARY_REMOVE_LINUX=false
> ;;
> riscv*+*)
> + if [ "${SUBARCH:-}" = "licheerv" ]; then
> + add_package install licheerv-rtl8723ds-dkms
I know this might be obvious right now, but could we add a comment outlining why this is needed for future reference?
> + fi
> # We'll add wpasupplicant to the seeds when we work on RISC-V seeds.
> add_package install wpasupplicant
> ;;
> diff --git a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> index f516e7a..3248d20 100755
> --- a/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> +++ b/live-build/ubuntu-cpc/hooks.d/base/disk-image-uefi.binary
> @@ -223,25 +237,65 @@ install_grub() {
> if [ -n "${SUBARCH:-}" ]; then
> # Per-device images
> case "${SUBARCH}" in
> - "nezha")
> - echo "Installing U-Boot for Nezha board" &1>2
> + "icicle")
> + echo "Installing GRUB for ${SUBARCH} board" &1>2
> + # flash-kernel is needed to install the dtb for update-grub: it uses the
> + # /proc/device-tree/model value to pick the correct dtb and as we are in a chroot,
> + # the model value is wrong and we need to use /etc/flash-kernel/machine instead.
> + # This explains why we install flash-kernel here.
> + chroot mountpoint mkdir -p /etc/flash-kernel/
> + chroot mountpoint bash -c "echo 'Microchip PolarFire-SoC Icicle Kit' > /etc/flash-kernel/machine"
> + chroot mountpoint bash -c 'FK_FORCE_CONTAINER=yes apt-get install -qqy grub-efi-riscv64 flash-kernel'
> + efi_target=riscv64-efi
> + # The real U-Boot
> + chroot mountpoint apt-get install -qqy u-boot-microchip
> + loader="/dev/mapper${loop_device///dev/}p13"
> + dd if=mountpoint/usr/lib/u-boot/microchip_icicle/u-boot.payload of=$loader
> + # Provide end-user modifyable CIDATA
> + cidata_dev="/dev/mapper${loop_device///dev/}p12"
> + setup_cidata "${cidata_dev}"
> + # Provide stock nocloud datasource
> + # Allow interactive login on baremetal board,
> + # without a cloud datasource.
> + setup_cinocloud mountpoint
> +
> + # u-boot-microchip will boot using UEFI if it does not find
> + # any extlinux.conf or boot.scr: but flash-kernel will
> + # install a boot.scr if it believes it did not boot in
> + # EFI mode, so make sure we don't leave a boot.scr
> + # behind.
> + chroot mountpoint rm -f /boot/boot.scr
> + ;;
> + "nezha" | "licheerv")
Same as in one of the above comments, let's do "nezha"|"licheerv" (or nezha|licheerv without the quotes even) for style consistency.
> + echo "Installing U-Boot for ${SUBARCH} board" &1>2
> # flash-kernel is needed to install the dtb for update-grub: it uses the
> # /proc/device-tree/model value to pick the correct dtb and as we are in a chroot,
> # the model value is wrong and we need to use /etc/flash-kernel/machine instead.
> # This explains why we install flash-kernel here.
> chroot mountpoint mkdir -p /etc/flash-kernel/
> - chroot mountpoint bash -c "echo 'Allwinner D1 Nezha' > /etc/flash-kernel/machine"
> + if [ "$SUBARCH" = "nezha" ]; then
> + chroot mountpoint bash -c "echo 'Allwinner D1 Nezha' > /etc/flash-kernel/machine"
> + elif [ "$SUBARCH" = "licheerv" ]; then
> + chroot mountpoint bash -c "echo 'Sipeed Lichee RV Dock' > /etc/flash-kernel/machine"
> + # cryptsetup-initramfs is a large contributor of the initrd size: we have to
> + # remove it for the LicheeRV board, otherwise it fails to boot. cryptsetup-initramfs
> + # needs to embed plymouth (and then the drm/gpu stuff) for interacting with the user
> + # to decrypt the rootfs (passphrase key).
> + chroot mountpoint bash -c "apt remove -qqy cryptsetup-initramfs"
> + fi
> chroot mountpoint bash -c 'FK_FORCE_CONTAINER=yes apt-get install -qqy grub-efi-riscv64 flash-kernel'
> efi_target=riscv64-efi
>
> + # nezha-boot0 is actually compatible with the LicheeRV boards (and probably other D1-based boards)
> chroot mountpoint apt-get install -qqy nezha-boot0
> # FSBL, which gets U-Boot SPL
> loader1="/dev/mapper${loop_device///dev/}p13"
> dd if=mountpoint/usr/lib/u-boot/nezha/boot0_sdcard_sun20iw1p1.bin of=$loader1
> # The real U-Boot
> + # u-boot-nezha actually contains both the LicheeRV and the Nezha boards support
> chroot mountpoint apt-get install -qqy u-boot-nezha
> loader2="/dev/mapper${loop_device///dev/}p14"
> - dd if=mountpoint/usr/lib/u-boot/nezha/u-boot.toc1 of=$loader2
> + dd if=mountpoint/usr/lib/u-boot/${SUBARCH}/u-boot.toc1 of=$loader2
> # Provide end-user modifyable CIDATA
> cidata_dev="/dev/mapper${loop_device///dev/}p12"
> setup_cidata "${cidata_dev}"
> @@ -336,7 +390,7 @@ EOF
> chroot mountpoint u-boot-update
> fi
>
> - if [ "${SUBARCH:-}" != "visionfive" ] && [ "${SUBARCH:-}" != "nezha" ]; then
> + if [ "${SUBARCH:-}" != "visionfive" ] && [ "${SUBARCH:-}" != "nezha" ] && [ "${SUBARCH:-}" != "licheerv" ] && [ "${SUBARCH:-}" != "icicle" ]; then
This is fine for now, but just leaving a comment for the future: since the list is getting longer and longer, I wonder if maybe we shouldn't switch this into a case-switch. aka.
case ${SUBARCH:-} in
visionfive|nezha|licheerv|icicle)
;;
*) # We do some special handling for other RISC-V platforms
# All the code below
> ## TODO remove below once we have grub-efi-riscv64 for the platforms
> rm mountpoint/tmp/device.map
> umount mountpoint/boot/efi
--
https://code.launchpad.net/~alexghiti/livecd-rootfs/+git/livecd-rootfs/+merge/429819
Your team Ubuntu Core Development Team is subscribed to branch livecd-rootfs:ubuntu/master.
More information about the Ubuntu-reviews
mailing list