[libvirt] [PATCH v2 2/3] qemu: Implement extended loader and nvram

[Laszlo Ersek]

notes below

On 08/15/14 15:43, Michal Privoznik wrote:
> QEMU now supports UEFI with the following command line:
> 
>   -drive file=/usr/share/OVMF/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \
>   -drive file=/usr/share/OVMF/OVMF_VARS.fd,if=pflash,format=raw,unit=1 \
> 
> where the first line reflects <loader> and the second one <nvram>.
> Moreover, these two lines obsoletes the -bios argument.
> 
> Note that UEFI is unusable without ACPI. This is handled properly now.
> Among with this extension, the variable file is expected to be
> writable and hence we need security drivers to label it.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 94 +++++++++++++++++++++-
>  src/security/security_dac.c                        |  8 ++
>  src/security/security_selinux.c                    |  8 ++
>  .../qemuxml2argvdata/qemuxml2argv-bios-nvram.args  | 10 +++
>  tests/qemuxml2argvtest.c                           |  2 +
>  5 files changed, 118 insertions(+), 4 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-bios-nvram.args

Again, I liked v1 2/3 (I had no comments), so I'm just comparing 2/3
between the two versions.

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 5142e2a..fe6b329 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7294,6 +7294,94 @@ qemuBuildChrDeviceCommandLine(virCommandPtr cmd,
>      return 0;
>  }
>  
> +static int
> +qemuBuilDomainLoaderCommandLine(virCommandPtr cmd,
> +                                virDomainDefPtr def,
> +                                virQEMUCapsPtr qemuCaps)
> +{
> +    int ret = -1;
> +    virDomainLoaderDefPtr loader = def->os.loader;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int unit = 0;

Okay, so you'll count the units as you go. If, for some other target
architecture, you won't need to introduce a pflash unit just for the
firmware executable, then the "rest" will have good unit numbers
automatically. I think this is in response to Paolo's review.

> +
> +    if (!loader)
> +        return 0;
> +
> +    switch ((virDomainLoader) loader->type) {
> +    case VIR_DOMAIN_LOADER_TYPE_ROM:
> +        virCommandAddArg(cmd, "-bios");
> +        virCommandAddArg(cmd, loader->path);
> +        break;
> +
> +    case VIR_DOMAIN_LOADER_TYPE_PFLASH:
> +        /* UEFI is supported inly for x86_64 currently */

harmless typo in new check: "inly"

[...]

Seems good to me.

Acked-by: Laszlo Ersek <lersek at redhat.com>

Thanks!
Laszlo