[Merge] ~alexghiti/livecd-rootfs:int/alex/riscv_no_beaglev into livecd-rootfs:ubuntu/master
Łukasz Zemczak
mp+424678 at code.launchpad.net
Fri Jul 1 12:07:40 UTC 2022
Review: Needs Fixing
In principle, this works. Inline (in the code) I have asked a few questions and requested a few changes, but in overall it's basically good. Some of the questions might be quite obvious to someone that knows the board, but I'm clearly missing some context. Can you take a look at those + the changes I mentioned? Thanks!
Diff comments:
> diff --git a/live-build/auto/config b/live-build/auto/config
> index dd9fa17..40f5a19 100755
> --- a/live-build/auto/config
> +++ b/live-build/auto/config
> @@ -957,7 +957,13 @@ case $PROJECT in
> ;;
> riscv64*)
> if [ -n "$SUBARCH" ]; then
> - KERNEL_FLAVOURS=generic
> + if [ "${SUBARCH:-}" = "nezha" ]; then
This is fine, but maybe for cleanness this should be turned into a switch statement? Since we might have more riscv64 variants coming up, who knows. So something in the likes of:
case "${SUBARCH:-}" in
nezha)
KERNEL_FLAVOURS=allwinner
;;
...
> + KERNEL_FLAVOURS=allwinner
> + elif [ "${SUBARCH:-}" = "visionfive" ]; then
> + KERNEL_FLAVOURS=starfive
> + else
> + KERNEL_FLAVOURS=generic
> + fi
> fi
> ;;
> esac
> 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 e802848..0062266 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
> @@ -183,39 +220,114 @@ install_grub() {
> ;;
> riscv64)
> # TODO grub-efi-riscv64 does not exist yet on riscv64
> - chroot mountpoint apt-get install -qqy u-boot-menu #grub-efi-riscv64
> - efi_target=riscv64-efi
> + if [ -n "${SUBARCH:-}" ]; then
> + case "${SUBARCH}" in
> + "nezha")
> + echo "Installing U-Boot for Nezha board" &1>2
> + chroot mountpoint mkdir -p /etc/flash-kernel/
> + chroot mountpoint bash -c "echo 'Allwinner D1 Nezha' > /etc/flash-kernel/machine"
Another place where I'd probably need some context: why is the need to override the flash-kernel machine detection here? Is it because flash-kernel is being invoked during image build in the chroot and we want to make sure it installs the right bits? Or is it because even on the final system this is the only way for flash-kernel to detect the board?
> + chroot mountpoint apt-get install -qqy grub-efi-riscv64 flash-kernel
I would like to know more about why flash-kernel is being installed at this stage. By principle, to keep things 'consistent', we should install all the packages that we need from the rootfs POV via live-build while creating the chroot, so basically if we need flash kernel on the images, this would result in it being either pulled in via seeds (metapackage), or - like we do for raspi and others, by adding a `add_package install flash-kernel` line in `live-build/auto/config`. livecd-rootfs is already a mess, since we basically install custom things in various places, but this makes more sense in overall. That being said, maybe there is a reason to install it later in the process, after the chroot is already 'done'? Can you provide some context?
> + efi_target=riscv64-efi
>
> - chroot mountpoint u-boot-update
> + 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
> + 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
> + # 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 SiFive board,
> + # without a cloud datasource.
> + setup_cinocloud mountpoint
>
> - if [ -n "${SUBARCH:-}" ]; then
> - u_boot_arch="${SUBARCH}"
> - if [ "${u_boot_arch}" = "hifive" ]; then
> - u_boot_arch=sifive_fu540
> - fi
> - chroot mountpoint apt-get install -qqy u-boot-sifive
> - # FSBL, which gets U-Boot SPL
> - loader1="/dev/mapper${loop_device///dev/}p13"
> - # The real U-Boot
> - loader2="/dev/mapper${loop_device///dev/}p14"
> - dd if=mountpoint/usr/lib/u-boot/${u_boot_arch}/u-boot-spl.bin of=$loader1
> - dd if=mountpoint/usr/lib/u-boot/${u_boot_arch}/u-boot.itb of=$loader2
> - # 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 SiFive board,
> - # without a cloud datasource.
> - setup_cinocloud mountpoint
> + # u-boot-nezha 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
> + ;;
> + "visionfive")
> + echo "Installing GRUB for VisionFive board" &1>2
> + chroot mountpoint mkdir -p /etc/flash-kernel/
> + chroot mountpoint bash -c "echo 'StarFive VisionFive V1' > /etc/flash-kernel/machine"
> + chroot mountpoint apt-get install -qqy grub-efi-riscv64 flash-kernel
Same two questions as above ^
> + efi_target=riscv64-efi
> +
> + # factory u-boot requires a p3 partition with /boot/uEnv.txt file
> + uenv_dev="/dev/mapper${loop_device///dev/}p3"
> + mkfs.ext4 "${uenv_dev}"
> + uenv_mnt_dir=`mktemp -d uenvXXX`
> + mount "${uenv_dev}" "${uenv_mnt_dir}"
> + mkdir -p "${uenv_mnt_dir}"/boot
> +
> + cat <<'EOF' >${uenv_mnt_dir}/boot/uEnv.txt
Oh, so just the u-boot env file? I'm wondering if we'll ever need to update this. I suppose that would be flash-kernel's job. Does flash-kernel handle updating this env file on the VisionFive?
> +scriptaddr=0x88100000
> +script_offset_f=0x1fff000
> +script_size_f=0x1000
> +
> +kernel_addr_r=0x84000000
> +kernel_comp_addr_r=0x90000000
> +kernel_comp_size=0x10000000
> +
> +fdt_addr_r=0x88000000
> +ramdisk_addr_r=0x88300000
> +
> +bootcmd=load mmc 0:f ${kernel_addr_r} /EFI/ubuntu/grubriscv64.efi; bootefi ${kernel_addr_r}
> +bootcmd_mmc0=devnum=0; run mmc_boot
> +
> +ipaddr=192.168.120.200
> +netmask=255.255.255.0
> +EOF
> +
> + umount "${uenv_mnt_dir}"
> + rmdir "${uenv_mnt_dir}"
> + # 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 SiFive board,
> + # without a cloud datasource.
> + setup_cinocloud mountpoint
> + ;;
> + *)
> + u_boot_arch="${SUBARCH}"
> + if [ "${u_boot_arch}" = "hifive" ]; then
> + u_boot_arch=sifive_fu540
> + fi
> + chroot mountpoint apt-get install -qqy u-boot-sifive
> + # FSBL, which gets U-Boot SPL
> + loader1="/dev/mapper${loop_device///dev/}p13"
> + # The real U-Boot
> + loader2="/dev/mapper${loop_device///dev/}p14"
> + dd if=mountpoint/usr/lib/u-boot/${u_boot_arch}/u-boot-spl.bin of=$loader1
> + dd if=mountpoint/usr/lib/u-boot/${u_boot_arch}/u-boot.itb of=$loader2
> + # 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 SiFive board,
> + # without a cloud datasource.
> + setup_cinocloud mountpoint
> + ;;
> + esac
> + fi
> +
> + if [ "${SUBARCH}" != "visionfive" && "${SUBARCH}" != "nezha" ]; then
> + ## TODO remove below once we have grub-efi-riscv64
> + rm mountpoint/tmp/device.map
> + umount mountpoint/boot/efi
> + mount
> + umount_partition mountpoint
> + rmdir mountpoint
> + return
> + ##
> fi
> - ## TODO remove below once we have grub-efi-riscv64
> - rm mountpoint/tmp/device.map
> - umount mountpoint/boot/efi
> - mount
> - umount_partition mountpoint
> - rmdir mountpoint
> - return
> - ##
> ;;
> esac
>
--
https://code.launchpad.net/~alexghiti/livecd-rootfs/+git/livecd-rootfs/+merge/424678
Your team Ubuntu Core Development Team is subscribed to branch livecd-rootfs:ubuntu/master.
More information about the Ubuntu-reviews
mailing list