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@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@src/bhyve/bhyve_command.c @SRCDIR@src/bhyve/bhyve_domain.c @SRCDIR@src/bhyve/bhyve_driver.c +@SRCDIR@src/bhyve/bhyve_firmware.c @SRCDIR@src/bhyve/bhyve_monitor.c @SRCDIR@src/bhyve/bhyve_parse_command.c @SRCDIR@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@redhat.com> Michal