Add aarch64 support, and more responsive qemu interaction

[jlinton]

The qemu commands to start a aarch64 guest vary slightly
from the x86 version. Also, as the code to run the EFI
binary in qemu tends to get hung up in the menus on
aarch64 lets make it wait for the shell prompt
before trying to start the binary.

Signed-off-by: Jeremy Linton lintonrjeremy@gmail.com

[lersek]

@jlinton not sure why your PR got ignored, sorry about that -- is this use case still relevant? If so, would you please rebase and resubmit your patch? Thanks.

[lersek]

please also split the patch in smaller units, if possible -- according to the commit message, the patch does at least two things now ("lets make it wait for the shell prompt").

BTW I believe @puiterwijk has improved the synchronization between the python script and the guest UEFI shell, since this PR was submitted.

Thanks.

[dannf]

fyi, I was actually starting to look at doing this rebase yesterday. One change I was going to suggest is that we add a --arch to be explicit about which architecture we are targeting instead of trying to guess based on the qemu binary name. And when --qemu-binary is not provided, we adjust our default interpreter based on any given --arch. I was also thinking it might be good to use class inheritance to hide the architecture parameter differences (model, cpu, how to attach an ISO, etc), and avoid if/else prolification, which will only get worse with future archs.

[lersek]

(TBH I'm not much of a pythonista at this point, so I guess I'll defer on the technical review here to others)

[jlinton]

Sure, Peter just pointed this out, the email this github is attached to is rarely checked..

Give me a bit to test it out.

[jlinton]

After recreating 1/2 of the aarch64 enroll process again, I remembered where I stuffed the fixes:

https://src.fedoraproject.org/fork/jlinton/rpms/edk2/c/fa42e3a3723ecb46ab908e858807b1af01cbb047?branch=master

[nullr0ute]

After recreating 1/2 of the aarch64 enroll process again, I remembered where I stuffed the fixes:

Can you push them as a (updated) PR here?

[dannf]

After recreating 1/2 of the aarch64 enroll process again, I remembered where I stuffed the fixes:

Can you push them as a (updated) PR here?

Those appear to be changes to edk2 packaging that call ovmf-vars-generator, not to the ovmf-vars-generator tool itself.

[jlinton]

Right, I just pushed a fairly simple rebase of the existing patches (and split the functionality).

The linked rpm/edk patches were more a FYI than anything, because they are some of the fedora specific changes needed to build firmware images with this utility. They provide the examples needed to make this work.

I didn't break the arch/etc out because at the moment that will break the existing build process. (Plus, i'm not much of a python person myself, this patch was one of a half dozen scattered over a few projects needed to get aarch64 secure boot functioning a couple years ago).

[jlinton]

Hang on, I did a trivial cleanup I just noticed,

I will push it again in a few mins when I've verified it still works.

[jlinton]

Ok that should it be it, I added a --arch option to override the qemu_binary if needed.

Although, one might consider just doing this with bash+expect if it gets much more complex....

[lersek]

This patch looks OK to me. I'll have to regression-test it later, when the PR is otherwise ready to be merged.

[lersek]

You know what, screw github.com with its idiotic patch review interface. My previous comment refers only to commit 6c83d5a. I made that comment using the Review Changes button, while standing at the following URL: https://github.com/rhuefi/qemu-ovmf-secureboot/pull/23/commits/6c83d5a80e5e02c239680e72c6af9bd8c0e8e6d1.

How in hell again can I respond to a single patch in a series, but not to a particular location within the patch? How can I approve a single patch in a series, similarly to posting Reviewed-by: Laszlo Ersek <lersek@redhat.com> in response to a single patch email?

[lersek]

I guess I'll just have to do my own thing here.

review refers to first patch in the series only

[lersek]

> From 6c83d5a80e5e02c239680e72c6af9bd8c0e8e6d1 Mon Sep 17 00:00:00 2001
> From: Jeremy Linton <jeremy.linton@arm.com>
> Date: Wed, 16 Sep 2020 16:21:24 -0500
> Subject: [PATCH 1/3] Modify "expect" like logic to prepare for aarch64
>
> This commit should avoid getting stuck in the menus
> and hanging forever on aarch64. Modify the initial
> key sequence from esc (which edk normally uses as the

(1) Typo: please replace "edk" with "edk2".

> escape to bds), then wait for the shell prompt
> before trying to run the binaries.
>
> Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
> ---
>  ovmf-vars-generator | 47 +++++++++++++++-----
>  1 file changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/ovmf-vars-generator b/ovmf-vars-generator
> index 04bcda7ea189..e71e215d7cd5 100755
> --- a/ovmf-vars-generator
> +++ b/ovmf-vars-generator
> @@ -20,6 +20,7 @@ import tempfile
>  import shutil
>  import string
>  import subprocess
> +import select
>
>
>  def strip_special(line):
> @@ -106,6 +107,9 @@ def enroll_keys(args):
>          stderr=subprocess.STDOUT)
>      logging.info('Performing enrollment')
>      # Wait until the UEFI shell starts (first line is printed)
> +    pollobj = select.poll()
> +    pollobj.register(p.stdout, select.POLLIN)
> +
>      read = p.stdout.readline()
>      if b'char device redirected' in read:
>          read = p.stdout.readline()
> @@ -116,25 +120,46 @@ def enroll_keys(args):
>      if args.print_output:
>          print(strip_special(read), end='')
>          print()
> -    # Send the escape char to enter the UEFI shell early
> -    p.stdin.write(b'\x1b')
> -    p.stdin.flush()
> -    # And then run the following three commands from the UEFI shell:
> -    # change into the first file system device; install the default
> -    # keys and certificates, and reboot
> -    p.stdin.write(b'fs0:\r\n')
> -    p.stdin.write(b'EnrollDefaultKeys.efi\r\n')
> -    p.stdin.write(b'reset -s\r\n')
> +    # Send the press enter for the default boot (uefi shell)
> +    p.stdin.write(b'\r\n')
>      p.stdin.flush()
> +
> +    wait_timeout = 100;
>      while True:
> -        read = p.stdout.readline()
> +        poll_result = pollobj.poll(1000)
> +        if poll_result:
> +            # readline can get stuck in menus and other
> +            # UI elements in uefi which don't respond
> +            # with a CR the poll above doesn't help
> +            # with that case.
> +            read = p.stdout.readline()
> +        else:
> +            wait_timeout-=1
> +            if wait_timeout == 0:
> +                logging.info('Failed enrollment')
> +                # consider something stronger here
> +                # as we can still get stuck in the p.wait()
> +                break;
> +            continue
>          if args.print_output and len(read):
>              print('OUT: %s' % strip_special(read), end='')
>              print()
> -        if b'info: success' in read:
> +        if b'seconds to skip' in read:
> +            p.stdin.write(b'\r\n')
> +            p.stdin.flush()
> +        elif b'info: success' in read:
>              break
>          elif b'Reset with <null string>' in read:
>              break
> +        elif b'Shell>' in read:
> +            # And then run the following three commands from the UEFI shell:
> +            # change into the first file system device; install the default
> +            # keys and certificates, and reboot
> +            p.stdin.write(b'fs0:\r\n')
> +            p.stdin.write(b'EnrollDefaultKeys.efi\r\n')
> +            p.stdin.write(b'reset -s\r\n')
> +            p.stdin.flush()
> +
>      p.wait()
>      if args.print_output:
>          print(strip_special(p.stdout.read()), end='')
> --
> 2.19.1.3.g30247aa5d201
>

This patch looks OK to me. I'll have to regression-test it later, when
the PR is otherwise ready to be merged.

With (1) updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[lersek]

> From 1c2d73d6e840a07ec4cb5bdb63c336b5a84b21f2 Mon Sep 17 00:00:00 2001
> From: Jeremy Linton <lintonrjeremy@gmail.com>
> Date: Fri, 26 Oct 2018 16:00:30 -0500
> Subject: [PATCH 2/3] Add aarch64 support
>
> The qemu commands to start a aarch64 guest vary slightly
> from the x86 version. Lets key off the passed qemu binary
> and tweak the comand line.
>
> Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
> ---
>  ovmf-vars-generator | 29 ++++++++++++--------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/ovmf-vars-generator b/ovmf-vars-generator
> index e71e215d7cd5..85fdfb9ea432 100755
> --- a/ovmf-vars-generator
> +++ b/ovmf-vars-generator
> @@ -28,12 +28,24 @@ def strip_special(line):
>
>
>  def generate_qemu_cmd(args, readonly, *extra_args):
> -    if args.disable_smm:
> +    is_x86 = True
> +    arch_args = []
> +    if os.path.basename(args.qemu_binary) == 'qemu-system-aarch64':
> +        machinetype = 'virt'
> +        arch_args.extend(['-cpu', 'cortex-a57'])
> +        is_x86 = False
> +    elif args.disable_smm:
>          machinetype = 'pc'
>      else:
>          machinetype = 'q35,smm=on'
>      machinetype += ',accel=%s' % ('kvm' if args.enable_kvm else 'tcg')
>
> +    if is_x86 == True:
> +        arch_args.extend(['-chardev', 'pty,id=charserial1',
> +                           '-device', 'isa-serial,chardev=charserial1,id=serial1',
> +                           '-global', 'driver=cfi.pflash01,property=secure,value=%s' % (
> +                               'off' if args.disable_smm else 'on')])
> +
>      if args.oem_string is None:
>          oemstrings = []
>      else:

We're doing too many things at once here.

(1) Please write a separate patch for turning the machine type and the
 "cfi.pflash01.secure" property, into the "arch_args" list.

In other words, this would only be a refactoring at first. Suggested new
layout:

  if (disable_smm) {
    arch_args += "-machine pc"
  } else {
    arch_args += "-machine q35"
    arch_args += "-machine smm=on"
    arch_args += "-global driver=cfi.pflash01,property=secure,value=on"
  }

  if (enable_kvm) {
    arch_args += "-machine accel=kvm"
  } else {
    arch_args += "-machine accel=tcg"
  }


(2) The options related to the serial port and to the restriction of
pflash to SMM should be moved to "arch_args" in a separate patch. At
this point, it should not be conditional on target architecture.

  arch_args += "-chardev pty,id=charserial1"
  arch_args += "-device isa-serial,chardev=charserial1,id=serial1"


(3) The "cortex-a57" VCPU model is incorrect when the KVM accelerator is
enabled on aarch64. In that case, the "host" model is required, to my
understanding.


(4) So ultimately, the patch that introduces aarch64 support should lay
out the conditions as follows:

  if (aarch64) {
    arch_args += "-machine virt"
    if (kvm) {
      arch_args += "-cpu host"
    } else {
      arch_args += "-cpu cortex-a57"
    }
  } else {
    if (disable_smm) {
      arch_args += "-machine pc"
    } else {
      arch_args += "-machine q35"
      arch_args += "-machine smm=on"
      arch_args += "-global driver=cfi.pflash01,property=secure,value=on"
    }

    arch_args += "-chardev pty,id=charserial1"
    arch_args += "-device isa-serial,chardev=charserial1,id=serial1"
  }

  if (enable_kvm) {
    arch_args += "-machine accel=kvm"
  } else {
    arch_args += "-machine accel=tcg"
  }


> @@ -51,18 +63,14 @@ def generate_qemu_cmd(args, readonly, *extra_args):
>          '-no-user-config',
>          '-nodefaults',
>          '-m', '768',
> -        '-smp', '2,sockets=2,cores=1,threads=1',
> -        '-chardev', 'pty,id=charserial1',
> -        '-device', 'isa-serial,chardev=charserial1,id=serial1',
> -        '-global', 'driver=cfi.pflash01,property=secure,value=%s' % (
> -            'off' if args.disable_smm else 'on'),
> +        '-smp', '1,sockets=1,cores=1,threads=1',

(5) Please write a separate patch for changing the "-smp" option. In
particular, the option is being changed for x86 too. I expect that
should be fine, but such a change needs its own patch.


>          '-drive',
>          'file=%s,if=pflash,format=raw,unit=0,readonly=on' % (
>              args.ovmf_binary),
>          '-drive',
>          'file=%s,if=pflash,format=raw,unit=1,readonly=%s' % (
>              args.out_temp, 'on' if readonly else 'off'),
> -        '-serial', 'stdio'] + oemstrings + list(extra_args)
> +        '-serial', 'stdio'] + oemstrings + arch_args + list(extra_args)
>
>
>  def download(url, target, suffix, no_download):
> @@ -96,11 +104,8 @@ def enroll_keys(args):
>          args,
>          False,
>          '-drive',
> -        'file=%s,format=raw,if=none,media=cdrom,id=drive-cd1,'
> -        'readonly=on' % args.uefi_shell_iso,
> -        '-device',
> -        'ide-cd,drive=drive-cd1,id=cd1,'
> -        'bootindex=1')
> +        'file=%s,format=raw,if=virtio,media=cdrom,id=drive-cd1,'
> +        'readonly=on' % args.uefi_shell_iso)
>      p = subprocess.Popen(cmd,
>          stdin=subprocess.PIPE,
>          stdout=subprocess.PIPE,

This change is justified (no ide-cd on aarch64/virt), but this is
not the right way to express it.

(6) Please write a separate patch for replacing the current ide-cd --
*only* the front-end -- with a virtio-scsi CD-ROM front-end, as follows:

(6a) add a virtio-scsi controller with the following option:

  -device virtio-scsi-pci,id=scsi0

(6b) replace the "ide-cd" device model with the "scsi-hd" device model,

(6c) append the following property to the "scsi-hd" device model, using
a comma (",") as property separator:

  bus=scsi0.0

In other words, the "-drive" option (the back-end) should not be
modified at all.


In summary, this patch should be split into 5 (five) patches:

#1 Extract the machine type and the "cfi.pflash01.secure" global
   property to "arch_args"; introducing "arch_args" for the first time.
   The discriminators that need to be considered are "disable_smm" and
   "enable_kvm". Refactoring only.

#2 Extract the serial port related options to "arch_args". At this
   point, their usage is unconditional. Refactoring only.

#3 Replace the IDE CD-ROM with a virtio-scsi CD-ROM (front-end-only
   change). This is a guest-visible change but functionally (considering
   the end-goal) a no-op.

#4 Change the "-smp" option. Guest-visible change, but functionally
   (considering the end-goal of the script) a no-op.

#5 Introduce aarch64 support, based on QEMU binary name. Pick the VCPU
   model dependent on whether KVM is enabled.

Thanks!
Laszlo

PS: screw github.

> --
> 2.19.1.3.g30247aa5d201
>

[lersek]

> From 8a58490b2e18086c55bd456dd9c8e4f8435f0b2b Mon Sep 17 00:00:00 2001
> From: Jeremy Linton <jeremy.linton@arm.com>
> Date: Wed, 16 Sep 2020 18:05:37 -0500
> Subject: [PATCH 3/3] Force the architecture
>
> Now that we support multiple arches lets allows the user

(1) s/lets/let's/

> to specify them, as well as trying to determine it from
> the passed qemu_binary.
>
> Signed-off-by: Jeremy Linton <lintonrjeremy@gmail.com>
> ---
>  ovmf-vars-generator | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ovmf-vars-generator b/ovmf-vars-generator
> index 85fdfb9ea432..594246b9ab17 100755
> --- a/ovmf-vars-generator
> +++ b/ovmf-vars-generator
> @@ -30,7 +30,7 @@ def strip_special(line):
>  def generate_qemu_cmd(args, readonly, *extra_args):
>      is_x86 = True
>      arch_args = []
> -    if os.path.basename(args.qemu_binary) == 'qemu-system-aarch64':
> +    if os.path.basename(args.qemu_binary) == 'qemu-system-aarch64' or args.arch == 'aarch64':

(2) looks OK, but please try to stick with an 80 char line length.

IIRC, PEP-8 explains how to wrap such lines. Something like

    if (os.path.basename(args.qemu_binary) == 'qemu-system-aarch64' or
        args.arch == 'aarch64'):

>          machinetype = 'virt'
>          arch_args.extend(['-cpu', 'cortex-a57'])
>          is_x86 = False
> @@ -212,6 +212,8 @@ def test_keys(args):
>
>  def parse_args():
>      parser = argparse.ArgumentParser()
> +    parser.add_argument('--arch', '-a', help='Architecture hint',
> +                        choices=['x86_64', 'aarch64'])

(3) Please improve the help text -- "Target architecture" or "Guest
architecture" would be more helpful, in my opinion.

With (1) through (3) addressed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>      parser.add_argument('output', help='Filename for output vars file')
>      parser.add_argument('--out-temp', help=argparse.SUPPRESS)
>      parser.add_argument('--force', help='Overwrite existing output file',
> --
> 2.19.1.3.g30247aa5d201
>
>

[lersek]

@dannf regarding using "class inheritance to hide the architecture parameter differences", I'm not against that, but please let's work out aarch64 support first with" if/else proliferation". That's what I prefer to review. Then we can have a separate patch / pull request to turn all that into a class hierarchy (purely as a refactoring, not as a functional change). I wouldn't like to deal with both an "if -> classes" conversion and an arch addition in the same PR. (In fact I'd prefer not reviewing the "if->classes" conversion; I'll let someone else have fun with that.) Thanks.

[dannf]

Personally I'd prefer we not make any assumptions about what the qemu_binary path means and just blindly use whatever they pass in. I think --arch should be the one and only way to determine what arguments are passed to that binary. If the user doesn't specify a binary, we can then pick a reasonable default based on the arch (where I think x86_64 would be the reasonable default).

[dannf]

I suggest using the UEFI names x64 & aarch64, since that's really what we're referring to here.

[dannf]

Should we add default='x86_64' (or 'x64') here?

[dannf]

I'd suggest keying off of args.arch instead. Say if someone were to save off a copy of qemu-system-aarch64 to qemu-system-aarch64.working and re-run with that - a change in behavior would seem unexpected.

[dannf]

Why have less CPUs on ARM?

[jlinton]

I can't remember exactly why I reduced that, but IIRC it was because initially I was testing this in a VM with only a single core.

Frankly, a better question is why have more than 1 core, the entire firmware stack is single threaded.
A better change is probably to completely remove that line as its unnecessary.

[lersek]

Frankly, a better question is why have more than 1 core, the entire firmware stack is single threaded.

OVMF includes modules for providing EFI_PEI_MP_SERVICES_PPI, EFI_MP_SERVICES_PROTOCOL, and EFI_MM_MP_PROTOCOL. So the general statement that the entire firmware stack is single threaded is incorrect.

It is true that PI / UEFI services are generally only usable on the BSP.

It is also true that, for this particular use case, we do not need more than 1 VCPU.

[dannf]

fwiw, I've had good luck with pexpect: https://salsa.debian.org/qemu-team/edk2/-/blob/debian/debian/tests/shell.py

[jlinton]

Which serves a purpose if one is in a python env. OTOH, python isn't a great shell scripting language anymore than c. Worse, this script is being used on fedora in an actual rpmbuild/bash environment. In that case about 80% of this scripts functionality can be represented like:

/usr/bin/expect <<EOF
spawn /usr/bin/qemu-system-${MACHTYPE} ${QEMU_ARCH} ${COMMON} $FIRMWARE ${VARS}=off $CDROM $SMBIOS
expect -exact "Shell>"
send -- "\r\nfs0:\r\n"
send -- "EnrollDefaultKeys.efi\r\n"
send -- "reset -s\r\n"
expect eof
EOF

the remainder, which I've also done (dealing with multiple arches, and validating enrolled keys) is about 3-4x the size of that.

[dannf]

Which serves a purpose if one is in a python env. OTOH, python isn't a great shell scripting language anymore than c. Worse, this script is being used on fedora in an actual rpmbuild/bash environment.

I'm not arguing for/against this being in python vs. sh, just that since it is in python, pexpect is at our disposal.