[OE-core] [PATCH 8/9] runqemu: support UEFI with OVMF firmware

[Patrick Ohly]

On Mon, 2017-01-09 at 19:50 -0800, Ricardo Neri wrote:
> On Wed, 2017-01-04 at 10:43 +0100, Patrick Ohly wrote:
> > To
> > test that supporting separate code and variables also works, I've
> > implemented that locally so that ovmf.fd ovmf_secboot.fd, ovmf_code.fd,
> > ovmf_secboot_code.fd and ovmf_vars.fd get deployed and runqemu supports
> > more than one "ovmf" parameter - this worked nicely. Full change below.
> 
> ... Now that you have took the time to prototype the solution, we could
> put it to use.
> 
> > 
> > Now that I've implemented it, I wonder whether it would be worth
> > submitting that as part of rev2 of this patch series. Any opinions?
> > 
> > diff --git a/meta/recipes-core/ovmf/ovmf_git.bb b/meta/recipes-core/ovmf/ovmf_git.bb
> > index ef61b16..391274b 100644
> > --- a/meta/recipes-core/ovmf/ovmf_git.bb
> > +++ b/meta/recipes-core/ovmf/ovmf_git.bb
> > @@ -125,6 +125,8 @@ do_compile_class-target() {
> >      rm -rf ${S}/Build/Ovmf$OVMF_DIR_SUFFIX
> >      ${S}/OvmfPkg/build.sh $PARALLEL_JOBS -a $OVMF_ARCH -b RELEASE -t ${FIXED_GCCVER}
> >      ln ${build_dir}/FV/OVMF.fd ${WORKDIR}/ovmf/OVMF.fd
> > +    ln ${build_dir}/FV/OVMF_CODE.fd ${WORKDIR}/ovmf/OVMF.code.fd
> > +    ln ${build_dir}/FV/OVMF_VARS.fd ${WORKDIR}/ovmf/OVMF.vars.fd
> >  
> >      # See CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt and
> >      # https://src.fedoraproject.org/cgit/rpms/edk2.git/tree/ for
> > @@ -137,6 +139,7 @@ do_compile_class-target() {
> >      ( cd ${S}/CryptoPkg/Library/OpensslLib/ && ./Install.sh )
> >      ${S}/OvmfPkg/build.sh $PARALLEL_JOBS -a $OVMF_ARCH -b RELEASE -t ${FIXED_GCCVER} ${OVMF_SECURE_BOOT_FLAGS}
> >      ln ${build_dir}/FV/OVMF.fd ${WORKDIR}/ovmf/OVMF.secboot.fd
> > +    ln ${build_dir}/FV/OVMF_CODE.fd ${WORKDIR}/ovmf/OVMF.code.secboot.fd
> >      for i in Shell.efi EnrollDefaultKeys.efi; do
> >          ln ${build_dir}/${OVMF_ARCH}/$i ${WORKDIR}/ovmf/$i
> >      done
> > @@ -170,8 +173,9 @@ do_deploy() {
> >  }
> >  do_deploy_class-target() {
> >      # For use with "runqemu ovmf".
> > -    qemu-img convert -f raw -O qcow2 ${WORKDIR}/ovmf/OVMF.fd ${DEPLOYDIR}/ovmf.qcow2
> > -    qemu-img convert -f raw -O qcow2 ${WORKDIR}/ovmf/OVMF.secboot.fd ${DEPLOYDIR}/ovmf.secboot.qcow2
> > +    for i in OVMF OVMF.secboot OVMF.code OVMF.vars OVMF.code.secboot; do
> > +        qemu-img convert -f raw -O qcow2 ${WORKDIR}/ovmf/$i.fd ${DEPLOYDIR}/`echo $i | tr A-Z a-z`.qcow2
> 
> Will this preserve any previous OVMF_vars.fd that might exist in the
> directory.

No, it will overwrite ovmf.vars.qcow2 and any variables stored in that
get lost. That is consistent with rebuilding the disk image: a user who
wants to have a "persistent" virtual machine must copy the relevant file
and then use the full file paths. In this case, that means invoking
runqemu with the file path to the copy of ovmf.vars.qcow2 instead of
just "ovmf.vars".

> >          if self.ovmf_bios:
> > -            print('OVMF: [%s]' % self.ovmf_bios)
> > +            print('OVMF: %s' % self.ovmf_bios)
> 
> Is there a reason to remove the brackets here?

self.ovmf_bios is a list, so formatting it as string will add the
brackets. I found that more readable than the (from a semantic point of
view more correct) double brackets: [['.../ovmf.code.qcow2',
'.../ovmf.vars.qcow2']]

> > +        for ovmf in self.ovmf_bios:
> > +            format = ovmf.rsplit('.', 1)[-1]
> > +            self.qemu_opt += ' -drive if=pflash,format=%s,file=%s' % (format, ovmf)
> >          if self.ovmf_bios:
> > -            format = self.ovmf_bios.rsplit('.', 1)[-1]
> > -            self.qemu_opt += ' -drive if=pflash,format=%s,file=%s' % (format, self.ovmf_bios)
> >              # OVMF only supports normal VGA, i.e. we need to override a -vga vmware
> >              # that gets added for example for normal qemux86.
> >              self.qemu_opt += ' -vga std'
> > 
> > 
> I think this solution looks good as having separate file does not pose
> an extra hassle in the user: the recipe builds all that is needed and
> runqemu takes all that it needs.

But not automatically. "runqemu ovmf" would expand "ovmf" to
tmp/deploy/images/.../ovmf.qcow2 and thus use the combined file while
"runqemu ovmf.code ovmf.vars" tells runqemu that it is supposed to use
two flash drives, one with the code and one with the variables.


>  If in the future people shows an
> interest in having unified images, maybe that can be added as another
> PACKAGECONFIG?

As the unified code+vars is slightly easier to use, I'd prefer to keep
it around and just offer both. That's not such a big deal in terms of
performance and disk usage.


-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.