[PATCH] bhyve: add <os firmware='efi'> support
Michal Privoznik
mprivozn at redhat.com
Mon Mar 15 10:10:05 UTC 2021
On 2/27/21 5:34 AM, Roman Bogorodskiy wrote:
> Implement "<os firmware='efi'>" support for bhyve driver.
> As there are not really lot of options, try to find
> "BHYVE_UEFI.fd" firmware which is installed by the
> sysutils/uefi-edk2-bhyve FreeBSD port.
>
> If not found, just use the first found firmware
> in the firmwares directory (which is configurable via
> config file).
>
> Signed-off-by: Roman Bogorodskiy <bogorodskiy at gmail.com>
> ---
> Not extremely happy about the LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE knob,
> but not sure how to test this otherwise.
>
> po/POTFILES.in | 1 +
> src/bhyve/bhyve_domain.c | 5 +
> src/bhyve/bhyve_firmware.c | 91 +++++++++++++++++++
> src/bhyve/bhyve_firmware.h | 30 ++++++
> src/bhyve/bhyve_process.c | 15 +++
> src/bhyve/bhyve_process.h | 5 +
> src/bhyve/meson.build | 1 +
> .../bhyvexml2argv-firmware-efi.args | 11 +++
> .../bhyvexml2argv-firmware-efi.ldargs | 1 +
> .../bhyvexml2argv-firmware-efi.xml | 22 +++++
> tests/bhyvexml2argvtest.c | 83 ++++++++++++++---
> 11 files changed, 254 insertions(+), 11 deletions(-)
> create mode 100644 src/bhyve/bhyve_firmware.c
> create mode 100644 src/bhyve/bhyve_firmware.h
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.args
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.ldargs
> create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.xml
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 80c5f145be..413783ee35 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -14,6 +14,7 @@
> @SRCDIR at src/bhyve/bhyve_command.c
> @SRCDIR at src/bhyve/bhyve_domain.c
> @SRCDIR at src/bhyve/bhyve_driver.c
> + at SRCDIR@src/bhyve/bhyve_firmware.c
> @SRCDIR at src/bhyve/bhyve_monitor.c
> @SRCDIR at src/bhyve/bhyve_parse_command.c
> @SRCDIR at src/bhyve/bhyve_process.c
> diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
> index 8fbc554a0a..209e4d3905 100644
> --- a/src/bhyve/bhyve_domain.c
> +++ b/src/bhyve/bhyve_domain.c
> @@ -64,6 +64,9 @@ bhyveDomainDefNeedsISAController(virDomainDefPtr def)
> if (def->os.bootloader == NULL && def->os.loader)
> return true;
>
> + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI)
> + return true;
> +
> if (def->nserials || def->nconsoles)
> return true;
>
> @@ -230,6 +233,8 @@ virDomainDefParserConfig virBhyveDriverDomainDefParserConfig = {
> .domainPostParseCallback = bhyveDomainDefPostParse,
> .assignAddressesCallback = bhyveDomainDefAssignAddresses,
> .deviceValidateCallback = bhyveDomainDeviceDefValidate,
> +
> + .features = VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT,
> };
>
> static void
> diff --git a/src/bhyve/bhyve_firmware.c b/src/bhyve/bhyve_firmware.c
> new file mode 100644
> index 0000000000..ccc3a5ffc8
> --- /dev/null
> +++ b/src/bhyve/bhyve_firmware.c
> @@ -0,0 +1,91 @@
> +/*
> + * bhyve_firmware.c: bhyve firmware management
> + *
> + * Copyright (C) 2021 Roman Bogorodskiy
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +#include <dirent.h>
> +
> +#include "viralloc.h"
> +#include "virlog.h"
> +#include "virfile.h"
> +#include "bhyve_conf.h"
> +#include "bhyve_firmware.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_BHYVE
> +
> +VIR_LOG_INIT("bhyve.bhyve_firmware");
> +
> +
> +#define BHYVE_DEFAULT_FIRMWARE "BHYVE_UEFI.fd"
> +
> +int
> +bhyveFirmwareFillDomain(bhyveConnPtr driver,
> + virDomainDefPtr def,
> + unsigned int flags)
> +{
> + g_autoptr(DIR) dir = NULL;
> + virBhyveDriverConfigPtr cfg = virBhyveDriverGetConfig(driver);
virBhyveDriverGetConfig() returns a reference, thus needs to be coupled
with virObjectUnref(cfg); otherwise .. [1]
> + const char *firmware_dir_cfg = cfg->firmwareDir;
> + const char *firmware_dir_env = NULL, *firmware_dir = NULL;
One variable per line, please. It turned out to be useful (although in
this specific case it's not using virXXXPtr type so doesn't matter):
https://listman.redhat.com/archives/libvir-list/2021-March/msg00542.html
> + struct dirent *entry;
> + char *matching_firmware = NULL;
> + char *first_found = NULL;
> +
> + virCheckFlags(0, -1);
1: .. @cfg is leaked here .. [2]
> +
> + if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
> + return 0;
2: .. or here. You get the idea. Alternatively, you can take inspiration
from the QEMU driver, where we defined autoptr cleanup func and then use:
g_autoptr(virQEMUDriverConfig) cfg = NULL;
> +
> + if (virDirOpenIfExists(&dir, firmware_dir_cfg) > 0) {
> + while ((virDirRead(dir, &entry, firmware_dir)) > 0) {
> + if (STREQ(entry->d_name, BHYVE_DEFAULT_FIRMWARE)) {
> + matching_firmware = g_strdup(entry->d_name);
> + break;
> + }
> + if (!first_found)
> + first_found = g_strdup(entry->d_name);
> + }
> + }
> +
> + if (!matching_firmware) {
> + if (!first_found) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no firmwares found in %s"),
> + firmware_dir_cfg);
> + return -1;
> + } else {
> + matching_firmware = first_found;
3: ^^^
> + }
> + }
> +
> + if (!def->os.loader)
> + def->os.loader = g_new0(virDomainLoaderDef, 1);
> +
> + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> + def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
> +
> + VIR_FREE(def->os.loader->path);
> +
> + firmware_dir_env = g_getenv("LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE");
> + firmware_dir = firmware_dir_env ? firmware_dir_env : firmware_dir_cfg;
> + def->os.loader->path = g_build_filename(firmware_dir, matching_firmware, NULL);
I don't think that g_build_filename() consumes @matching_firmware (even
though GLib doesn't document this, looking at its source code - it sure
doesn't). Therefore, @matching_firmware should be freed in the end. The
same goes for @first_round - I can too find a path through this function
which leaks it. For [3] I think you can use g_steal_pointer() to
transfer the ownership.
> +
> + return 0;
> +}
> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
> index 197334f9c4..13b8e34c2b 100644
> --- a/tests/bhyvexml2argvtest.c
> +++ b/tests/bhyvexml2argvtest.c
> @@ -142,6 +163,35 @@ mymain(void)
> if (!(driver.remotePorts = virPortAllocatorRangeNew("display", 5900, 65535)))
> return EXIT_FAILURE;
>
> + if (!(driver.config = virBhyveDriverConfigNew()))
> + return EXIT_FAILURE;
> +
> + fakefirmwaredir = g_strdup(FAKEFIRMWAREDIRTEMPLATE);
> +
> + if (!g_mkdtemp(fakefirmwaredir)) {
> + fprintf(stderr, "Cannot create fakefirmwaredir");
> + abort();
> + }
> + driver.config->firmwareDir = fakefirmwaredir;
> +
> + emptyfirmwaredir = g_strdup(FAKEFIRMWAREDIRTEMPLATE);
> +
> + if (!g_mkdtemp(emptyfirmwaredir)) {
> + fprintf(stderr, "Cannot create emptyfirmwaredir");
> + abort();
> + }
> +
> + i = 0;
> + while (firmwares[i]) {
> + g_autofree char *firmware_path = g_strdup_printf("%s/%s", fakefirmwaredir, firmwares[i++]);
for() loop would be much more readable IMO.
> +
> + if (virFileTouch(firmware_path, 0600) < 0) {
> + fprintf(stderr, "Cannot create firmware file");
> + abort();
> + }
> + }
> +
> + g_setenv("LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE", "test_firmware_dir", TRUE);
Reviewed-by: Michal Privoznik <mprivozn at redhat.com>
Michal
More information about the libvir-list
mailing list