-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aarch64 support, and more responsive qemu interaction #23
base: master
Are you sure you want to change the base?
Conversation
@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. |
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. |
fyi, I was actually starting to look at doing this rebase yesterday. One change I was going to suggest is that we add a |
(TBH I'm not much of a pythonista at this point, so I guess I'll defer on the technical review here to others) |
Sure, Peter just pointed this out, the email this github is attached to is rarely checked.. Give me a bit to test it out. |
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 |
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). |
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. |
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 escape to bds), then wait for the shell prompt before trying to run the binaries. Signed-off-by: Jeremy Linton <[email protected]>
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 <[email protected]>
Now that we support multiple arches lets allows the user to specify them, as well as trying to determine it from the passed qemu_binary. Signed-off-by: Jeremy Linton <[email protected]>
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.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch looks OK to me. I'll have to regression-test it later, when the PR is otherwise ready to be merged.
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: 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 |
I guess I'll just have to do my own thing here. |
review refers to first patch in the series only
|
|
|
@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. |
@@ -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': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -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']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using the UEFI names x64
& aarch64
, since that's really what we're referring to here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add default='x86_64' (or 'x64') here?
ovmf-vars-generator
Outdated
if args.disable_smm: | ||
is_x86 = True | ||
arch_args = [] | ||
if os.path.basename(args.qemu_binary) == 'qemu-system-aarch64': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
'-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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have less CPUs on ARM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
p.stdin.write(b'EnrollDefaultKeys.efi\r\n') | ||
p.stdin.write(b'reset -s\r\n') | ||
p.stdin.flush() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, I've had good luck with pexpect: https://salsa.debian.org/qemu-team/edk2/-/blob/debian/debian/tests/shell.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 [email protected]