Merge lp:~newell-jensen/curtin/add-arm64-support into lp:~curtin-dev/curtin/trunk

[Newell Jensen]

Add arm64 support to curtin. This branch also adds support to allow the creation of an extra /boot partition that is often times needed by arm64 boot loaders.

[Andres Rodriguez (andreserl)]

What we need to consider here is that what happens in we are booting arm64/generic. Curtin will continue to see this as aarch64. This would mean that we probably won't need to do the partitioning here, as it only applies to u-boot, right?

[Blake Rouse (blake-rouse)]

I made some comments on the code. I know its a work in-progress. I didn't comment much on the bash code, as I do not have that much experience.

[Jason Hobbs (jason-hobbs)]

You can check for the '/sys/firmware/efi' directory. If it exists, then the system was booted with UEFI, not U-Boot, and doesn't require the separate boot partition. It's my understanding that arm64/generic systems booting UEFI won't require the separate /boot partition, but we would need to confirm this with the vendor. I would expect that UEFI will be able to handle either scenario - separate boot partition or not.

I have a few comments inline below too.

[Scott Moser (smoser)]

I'll take a look further next week,
but some nitpick comments inline.

make sure you follow the code conventions in tools/common (4 space indentation).

[Newell Jensen (newell-jensen)]

Scott,

There was a newer revision checked in last night but you commented on the
old revision. Looking over your comments. As far as tabs, I am using
emacs and it is using 4-spaces so your comment on the tab indentation seems
odd and I have checked the file and everything lines up so maybe launchpad
shows it like that? I will update branch and push so you can have a look
at it on Monday.

On Fri, Jul 18, 2014 at 6:05 AM, Scott Moser <email address hidden> wrote:

> I'll take a look further next week,
> but some nitpick comments inline.
>
> make sure you follow the code conventions in tools/common (4 space
> indentation).
>
>
> Diff comments:
>
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py 2014-06-19 11:51:08 +0000
> > +++ curtin/commands/block_meta.py 2014-07-15 18:09:58 +0000
> > @@ -32,7 +32,7 @@
> > 'metavar': 'DEVICE', 'default': None, }),
> > ('--fstype', {'help': 'root filesystem type',
> > 'choices': ['ext4', 'ext3'], 'default': 'ext4'}),
> > - ('mode', {'help': 'meta-mode to use', 'choices': ['raid0',
> 'simple']}),
> > + ('mode', {'help': 'meta-mode to use', 'choices': ['raid0',
> 'simple', 'simple-boot']}),
> > )
> > )
> >
> > @@ -41,6 +41,8 @@
> > # main entry point for the block-meta command.
> > if args.mode == "simple":
> > meta_simple(args)
> > + elif args.mode == "simple-boot":
> > + meta_simple_boot(args)
> > else:
> > raise NotImplementedError("mode=%s is not implemenbed" %
> args.mode)
> >
> > @@ -131,6 +133,80 @@
> > return 0
> >
> >
> > +def meta_simple_boot(args):
> > + """Similar to meta_simple but it also creates an extra /boot
> partition.
> > + This is needed from some instances of u-boot.
> > + """
> > + state = util.load_command_environment()
> > +
> > + cfg = util.load_command_config(args, state)
> > +
> > + devices = args.devices
> > + if devices is None:
> > + devices = cfg.get('block-meta', {}).get('devices', [])
> > +
> > + # Remove duplicates but maintain ordering.
> > + devices = list(OrderedDict.fromkeys(devices))
> > +
> > + if len(devices) == 0:
> > + devices = block.get_installable_blockdevs()
> > + LOG.warn("simple-boot mode, no devices given. unused list: %s",
> devices)
> > +
> > + if len(devices) > 1:
> > + if args.devices is not None:
> > + LOG.warn("simple-boot mode but multiple devices given. "
> > + "using first found")
> > + available = [f for f in devices
> > + if block.is_valid_device(f)]
> > + target = sorted(available)[0]
> > + LOG.warn("mode is 'simple-boot'. multiple devices given. using
> '%s' "
> > + "(first available)", target)
> > + else:
> > + target = devices[0]
> > +
> > + if not block.is_valid_device(target):
> > + raise Exception("target device '%s' is not a valid device" %
> target)
> > +
> > + (devname, devnode) = block.get_dev_name_entry(target)
> > +
> > + LOG.info("installing in simple-boot mode to '%s'", devname)
> > +
> > + sources = cfg.get('source...

[lp:~newell-jensen/curtin/add-arm64-support]

145. By Newell Jensen on 2014-07-18

Added comments to common to help in readability and deleted some unused code.

[Newell Jensen (newell-jensen)]

Scott,

I am no bash expert so you might be correct that maxend might be "" but since I am setting it by doing:

        maxend=$((($size/512)-$start-$bootsize))

Won't it be something other than "" for sure?

[Scott Moser (smoser)]

you most certainly added tabs.

[Newell Jensen (newell-jensen)]

Yes, I did. Originally mis-read what you wrote. Will fix that.

On Mon, Jul 21, 2014 at 9:57 AM, Scott Moser <email address hidden> wrote:

> you most certainly added tabs.
>
> --
>
> https://code.launchpad.net/~newell-jensen/curtin/add-arm64-support/+merge/226196
> You are the owner of lp:~newell-jensen/curtin/add-arm64-support.
>

[lp:~newell-jensen/curtin/add-arm64-support]

146. By Newell Jensen on 2014-07-21

Replacing tabs with spaces.

[Scott Moser (smoser)]

comments remaining:
 - if user specifies 'simple-boot', but is_uefi_bootable, they will get uefi. that seems possibly wrong
 - boot is passed around in helpers/common as a number. makes more sense to be a boolean, no? use 'true' or 'false' as values.
 - i dont think i like having partitioning_commands['builtin'] being dynamic.
   I think i'd rather just have 'simple' do the right thing, but let a user provided config explicitly set that.
    Also, config can be set on the command line, so:
      curtin install --set block-meta/boot-partition/enabled=True
    is the same as you have here with:
      curtin block-meta --boot

I've just proposed merging:
 bzr+ssh://bazaar.launchpad.net/~smoser/curtin/add-arm64-support/
into here. It has some cleanups. the config based approach isn't completely done there.

[lp:~newell-jensen/curtin/add-arm64-support]

147. By Newell Jensen on 2014-07-21

Merging Scott Moser's changes in my branch.

148. By Newell Jensen on 2014-07-21

Added /boot partition support to UEFI.

149. By Newell Jensen on 2014-07-22

Deleted UEFI /boot partition capabilities. Stripped /boot size, sticking with
default of 512MiB. Created AUTO mode that will check for platform machine
architecture.

150. By Newell Jensen on 2014-07-22

Deleting AUTO mode to simplify things and move all the logic in block_meta.py

=== modified file 'curtin/commands/block_meta.py'
--- curtin/commands/block_meta.py	2014-07-21 17:07:39 +0000
+++ curtin/commands/block_meta.py	2014-07-22 21:12:03 +0000
@@ -22,24 +22,32 @@
 
 from . import populate_one_subcmd
 
+import os
+import platform
+
+SIMPLE = 'simple'
+SIMPLE_BOOT = 'simple-boot'
 
 CMD_ARGUMENTS = (
     ((('-D', '--devices'),
       {'help': 'which devices to operate on', 'action': 'append',
        'metavar': 'DEVICE', 'default': None, }),
-     ('--fstype', {'help': 'root filesystem type',
+     ('--fstype', {'help': 'root partition filesystem type',
                    'choices': ['ext4', 'ext3'], 'default': 'ext4'}),
-     ('mode', {'help': 'meta-mode to use', 'choices': ['raid0', 'simple']}),
+     ('--boot-fstype', {'help': 'boot partition filesystem type',
+                        'choices': ['ext4', 'ext3'], 'default': None}),
+     ('mode', {'help': 'meta-mode to use',
+               'choices': ['raid0', SIMPLE, SIMPLE_BOOT]}),
      )
 )
 
 
 def block_meta(args):
     # main entry point for the block-meta command.
-    if args.mode == "simple":
+    if args.mode in (SIMPLE, SIMPLE_BOOT):
         meta_simple(args)
     else:
-        raise NotImplementedError("mode=%s is not implemenbed" % args.mode)
+        raise NotImplementedError("mode=%s is not implemented" % args.mode)
 
 
 def logtime(msg, func, *args, **kwargs):
@@ -61,7 +69,30 @@
     return block.get_root_device([devname, ])
 
 
+def get_bootpt_cfg(cfg, enabled=False, fstype=None):
+    # 'cfg' looks like:
+    #   enabled: boolean
+    #   fstype: filesystem type (default to 'fstype')
+    #   label:  filesystem label (default to 'boot')
+    #   size:   filesystem size in M (default to 512)
+    # parm enable can enable, but not disable
+    # parm fstype overrides cfg['fstype']
+    def_boot = platform.machine() in ('aarch64')
+    ret = {'enabled': def_boot, 'fstype': None, 'size': 512, 'label': 'boot'}
+    ret.update(cfg)
+    if enabled:
+        ret['enabled'] = True
+    if ret['enabled']:
+        if fstype and not ret['fstype']:
+            ret['fstype'] = fstype
+    ret['size'] = int(ret['size'])
+    return ret
+
+
 def meta_simple(args):
+    """Creates a root partition. If args.mode == SIMPLE_BOOT, it will also
+    create a separate /boot partition.
+    """
     state = util.load_command_environment()
 
     cfg = util.load_command_config(args, state)
@@ -70,22 +101,27 @@
     if devices is None:
         devices = cfg.get('block-meta', {}).get('devices', [])
 
+    bootpt = get_bootpt_cfg(
+        cfg.get('block-meta', {}).get('boot-partition', {}),
+        enabled=args.mode == SIMPLE_BOOT, fstype=args.boot_fstype)
+
     # Remove duplicates but maintain ordering.
     devices = list(OrderedDict.fromkeys(devices))
 
     if len(devices) == 0:
         devices = block.get_installable_blockdevs()
-        LOG.warn("simple mode, no devices given. unused list: %s", devices)
+        LOG.warn("'%s' mode, no devices given. unused list: %s",
+                 (args.mode, devices))
 
     if len(devices) > 1:
         if args.devices is not None:
-            LOG.warn("simple mode but multiple devices given. "
-                     "using first found")
+            LOG.warn("'%s' mode but multiple devices given. "
+                     "using first found", args.mode)
         available = [f for f in devices
                      if block.is_valid_device(f)]
         target = sorted(available)[0]
-        LOG.warn("mode is 'simple'. multiple devices given. using '%s' "
-                 "(first available)", target)
+        LOG.warn("mode is '%s'. multiple devices given. using '%s' "
+                 "(first available)", (args.mode, target))
     else:
         target = devices[0]
 
@@ -94,10 +130,11 @@
 
     (devname, devnode) = block.get_dev_name_entry(target)
 
-    LOG.info("installing in simple mode to '%s'", devname)
+    LOG.info("installing in '%s' mode to '%s'", (args.mode, devname))
 
     sources = cfg.get('sources', {})
     dd_images = util.get_dd_images(sources)
+
     if len(dd_images):
         # we have at least one dd-able image
         # we will only take the first one
@@ -110,19 +147,37 @@
         logtime(
             "partition --format uefi %s" % devnode,
             util.subp, ("partition", "--format", "uefi", devnode))
+        bootpt['enabled'] = False
+    elif bootpt['enabled']:
+        logtime("partition --boot %s" % devnode,
+                util.subp, ("partition", "--boot", devnode))
+        bootdev = devnode + "1"
+        rootdev = devnode + "2"
     else:
         logtime(
             "partition %s" % devnode,
             util.subp, ("partition", devnode))
-
-    rootdev = devnode + "1"
-
+        rootdev = devnode + "1"
+
+    # mkfs for root partition first and mount
     cmd = ['mkfs.%s' % args.fstype, '-q', '-L', 'cloudimg-rootfs', rootdev]
     logtime(' '.join(cmd), util.subp, cmd)
-
     util.subp(['mount', rootdev, state['target']])
 
+    if bootpt['enabled']:
+        # create 'boot' directory in state['target']
+        boot_dir = os.path.join(state['target'], 'boot')
+        util.subp(['mkdir', boot_dir])
+        # mkfs for boot partition and mount
+        cmd = ['mkfs.%s' % bootpt['fstype'],
+               '-q', '-L', bootpt['label'], bootdev]
+        logtime(' '.join(cmd), util.subp, cmd)
+        util.subp(['mount', bootdev, boot_dir])
+
     with open(state['fstab'], "w") as fp:
+        if bootpt['enabled']:
+            fp.write("LABEL=%s /boot %s defaults 0 0\n" %
+                     (bootpt['label'], bootpt['fstype']))
         fp.write("LABEL=%s / %s defaults 0 0\n" %
                  ('cloudimg-rootfs', args.fstype))
 

=== modified file 'curtin/commands/install.py'
--- curtin/commands/install.py	2014-06-08 13:52:36 +0000
+++ curtin/commands/install.py	2014-07-22 21:12:03 +0000
@@ -29,13 +29,15 @@
 
 from . import populate_one_subcmd
 
+
 CONFIG_BUILTIN = {
     'sources': {},
     'stages': ['early', 'partitioning', 'network', 'extract', 'curthooks',
                'hook', 'late'],
     'extract_commands': {'builtin': ['curtin', 'extract']},
     'hook_commands': {'builtin': ['curtin', 'hook']},
-    'partitioning_commands': {'builtin': ['curtin', 'block-meta', 'simple']},
+    'partitioning_commands': {
+        'builtin': ['curtin', 'block-meta', 'simple']},
     'curthooks_commands': {'builtin': ['curtin', 'curthooks']},
     'late_commands': {'builtin': []},
     'network_commands': {'builtin': ['curtin', 'net-meta', 'auto']},
@@ -271,6 +273,8 @@
     finally:
         for d in ('sys', 'dev', 'proc'):
             util.do_umount(os.path.join(workingd.target, d))
+        if util.is_mounted(workingd.target, 'boot'):
+            util.do_umount(os.path.join(workingd.target, 'boot'))
         util.do_umount(workingd.target)
         shutil.rmtree(workingd.top)
 

=== modified file 'helpers/common'
--- helpers/common	2014-04-21 16:18:39 +0000
+++ helpers/common	2014-07-22 21:12:03 +0000
@@ -15,6 +15,7 @@
      -f | --format F   use partition table format F. [mbr, gpt, uefi]
                        default gpt
      -E | --end E      end the partition at E (unit 1k bytes)
+     -b | --boot       create a boot partition (512 MiB - default)
 EOF
     [ $# -eq 0 ] || echo "$@"
 }
@@ -60,8 +61,8 @@
 }
 
 pt_gpt() {
-    local target="$1" end=${2:-""} size="" s512="" ptype="L"
-    local start="2048" pt1size="" maxend=""
+    local target="$1" end=${2:-""} boot="$3" size="" s512="" ptype="L"
+    local start="2048" rootsize="" bootsize="1048576" maxend=""
     local isblk=false
     getsize "$target" ||
         { error "failed to get size of $target"; return 1; }
@@ -71,15 +72,33 @@
     else
         end=$(($end/512))
     fi
-    maxend=$((($size/512)-2048))
+
+    if [ "$boot" = true ]; then
+        maxend=$((($size/512)-$start-$bootsize))
+        if [ $maxend -lt 0 ]; then
+            error "Disk is not big enough for /boot partition on $target";
+            return 1;
+        fi
+    else
+        maxend=$((($size/512)-$start))
+    fi
     [ "$end" -gt "$maxend" ] && end="$maxend"
     debug 1 "maxend=$maxend end=$end size=$size"
 
     [ -b "$target" ] && isblk=true
 
-    sgdisk --new "15:2048:+1M" --typecode=15:ef02 \
-           --new "1::$end"     --typecode=1:8300 "$target" ||
-        { error "failed to gpt partition $target"; return 1; }
+    if [ "$boot" = true ]; then 
+        # Creating 'efi', '/boot' and '/' partitions
+        sgdisk --new "15:$start:+1M" --typecode=15:ef02 \
+            --new "1::+512M" --typecode=1:8300 \
+            --new "2::$end" --typecode=2:8300 "$target" ||
+            { error "failed to gpt partition $target"; return 1; }
+    else
+        # Creating 'efi' and '/' partitions 
+        sgdisk --new "15:$start:+1M" --typecode=15:ef02 \
+            --new "1::$end" --typecode=1:8300 "$target" ||
+            { error "failed to gpt partition $target"; return 1; }
+    fi
 
     if $isblk; then
         blockdev --rereadpt "$target"
@@ -88,12 +107,16 @@
             { error "no partition found ${target}1"; return 1; }
         [ -b "${target}15" ] ||
             { error "no partition found ${target}15"; return 1; }
+        if [ "$boot" = true ]; then
+            [ -b "${target}2" ] ||
+                { error "no partition found ${target}2"; return 1; }
+        fi
     fi
 }
 
 pt_uefi() {
     local target="$1" end=${2:-""} size="" s512="" ptype="L"
-    local start="2048" pt1size="" maxend=""
+    local start="2048" rootsize="" maxend=""
     local isblk=false
     getsize "$target" ||
         { error "failed to get size of $target"; return 1; }
@@ -103,14 +126,15 @@
     else
         end=$(($end/512))
     fi
-    maxend=$((($size/512)-2048))
+
+    maxend=$((($size/512)-$start))
     [ "$end" -gt "$maxend" ] && end="$maxend"
     debug 1 "maxend=$maxend end=$end size=$size"
 
     [ -b "$target" ] && isblk=true
 
-    # Part 15 is the UEFI partition, part 1 is root
-    sgdisk --new "15:2048:+512M" --typecode=15:ef00 "$target" \
+    # Creating 'UEFI' and '/' partitions 
+    sgdisk --new "15:2048:+512M" --typecode=15:ef00 \
            --new "1::$end" --typecode=1:8300 "$target" ||
         { error "failed to sgdisk for uefi to $target"; return 1; }
 
@@ -175,22 +199,23 @@
 }
 
 partition_main() {
-    local short_opts="hE:f:v"
-    local long_opts="help,end:,format:,verbose"
+    local short_opts="hE:f:bv"
+    local long_opts="help,end:,format:,boot,verbose"
     local getopt_out=$(getopt --name "${0##*/}" \
         --options "${short_opts}" --long "${long_opts}" -- "$@") &&
         eval set -- "${getopt_out}" ||
         { partition_main_usage 1>&2; return 1; }
 
     local cur="" next=""
-    local format="mbr" target="" end=""
+    local format="gpt" boot=false target="" end=""
 
     while [ $# -ne 0 ]; do
         cur="$1"; next="$2";
         case "$cur" in
             -h|--help) partition_main_usage ; exit 0;;
+            -E|--end) end=$next; shift;;
             -f|--format) format=$next; shift;;
-            -E|--end) end=$next; shift;;
+            -b|--boot) boot=true;;
             -v|--verbose) VERBOSITY=$((${VERBOSITY}+1));;
             --) shift; break;;
         esac
@@ -222,7 +247,7 @@
     if [ "$format" = "mbr" ]; then
         pt_mbr "$target" "$end"
     elif [ "$format" = "gpt" ]; then
-        pt_gpt "$target" "$end"]
+        pt_gpt "$target" "$end" "$boot"
     elif [ "$format" = "uefi" ]; then
         pt_uefi "$target" "$end"
     fi