On 03/07/19 10:29, Michal Privoznik wrote:
> And finally the last missing piece. This is what puts it all
> together.
>
> At the beginning, qemuFirmwareFillDomain() loads all possible
> firmware description files based on algorithm described earlier.
> Then it tries to find description which matches given domain.
> The criteria are:
>
> - firmware is the right type (e.g. it's bios when bios was
> requested in domain XML)
> - firmware is suitable for guest architecture/machine type
> - firmware allows desired guest features to stay enabled (e.g.
> if s3/s4 is enabled for guest then firmware has to support
> it too)
>
> Once the desired description has been found it is then used to
> set various bits of virDomainDef so that proper qemu cmd line is
> constructed as demanded by the description file. For instance,
> secure boot enabled firmware might request SMM -> it will be
> enabled if needed.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_firmware.c | 329 +++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_firmware.h | 7 +
> 2 files changed, 336 insertions(+)
>
> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index a818f60c91..c8b337cf2a 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -23,6 +23,8 @@
> #include "qemu_firmware.h"
> #include "configmake.h"
> #include "qemu_capabilities.h"
> +#include "qemu_domain.h"
> +#include "qemu_process.h"
> #include "virarch.h"
> #include "virfile.h"
> #include "virhash.h"
> @@ -1033,3 +1035,330 @@ qemuFirmwareFetchConfigs(char ***firmwares)
>
> return 0;
> }
> +
> +
> +static bool
> +qemuFirmwareMatchDomain(const virDomainDef *def,
> + const qemuFirmware *fw,
> + const char *path)
> +{
> + size_t i;
> + bool supportsS3 = false;
> + bool supportsS4 = false;
> + bool requiresSMM = false;
> + bool supportsSEV = false;
> +
> + for (i = 0; i < fw->ninterfaces; i++) {
> + if ((def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS &&
> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_BIOS) ||
> + (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI &&
> + fw->interfaces[i] == QEMU_FIRMWARE_OS_INTERFACE_UEFI))
> + break;
> + }
> +
> + if (i == fw->ninterfaces) {
> + VIR_DEBUG("No matching interface in '%s'", path);
> + return false;
> + }
> +
> + for (i = 0; i < fw->ntargets; i++) {
> + size_t j;
> +
> + if (def->os.arch != fw->targets[i]->architecture)
> + continue;
> +
> + for (j = 0; j < fw->targets[i]->nmachines; j++) {
> + if (virStringMatch(def->os.machine, fw->targets[i]->machines[j]))
> + break;
> + }
> +
> + if (j == fw->targets[i]->nmachines)
> + continue;
> +
> + break;
> + }
> +
> + if (i == fw->ntargets) {
> + VIR_DEBUG("No matching machine type in '%s'", path);
> + return false;
> + }
> +
> + for (i = 0; i < fw->nfeatures; i++) {
> + switch (fw->features[i]) {
> + case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> + supportsS3 = true;
> + break;
> + case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> + supportsS4 = true;
> + break;
> + case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> + supportsSEV = true;
> + break;
> + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> + requiresSMM = true;
> + break;
> +
> + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> + case QEMU_FIRMWARE_FEATURE_NONE:
> + case QEMU_FIRMWARE_FEATURE_LAST:
> + break;
> + }
> + }
> +
> + if (def->pm.s3 == VIR_TRISTATE_BOOL_YES &&
> + !supportsS3) {
> + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path);
> + return false;
> + }
> +
> + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES &&
> + !supportsS4) {
> + VIR_DEBUG("Domain requires S3, firmware '%s' doesn't support it", path);
(1) This debug message should refer to "S4", not "S3".
> + return false;
> + }
> +
> + if (def->os.loader &&
> + ((def->os.loader->secure == VIR_TRISTATE_BOOL_YES) != requiresSMM)) {
> + VIR_DEBUG("Not matching secure boot/SMM in '%s'", path);
> + return false;
> + }
This check is too strict. Please refer to point (1) in:
http://mid.mail-archive.com/9835d837-cfa4-d708-2f41-3d29e2254de4@xxxxxxxxxx
There I wrote:
> (1) if REQUIRES_SMM is absent from the firmware descriptor, then
> @secure must be "no" in the domain config. Equivalently, if @secure is
> "yes", then the firmware descriptor must come with REQUIRES_SMM.
This means that "@secure *implies* REQUIRES_SMM":
@secure --> REQUIRES_SMM
and that an equivalent form to write the exact same logical implication
is:
!REQUIRES_SMM --> !@secure
But the C condition above is stricter than this. It will reject
(!@secure && REQUIRES_SMM). That's wrong. @secure=no *should* accept a
firmware both with and without REQUIRES_SMM. @secure=yes should only
accept a firmware with REQUIRES_SMM.
The way to write implications in the C language is to first transform
the logical predicate from the "implication operator" to the logical
negation operator and the disjunction ("or") operator. In general:
A --> B
is equivalent to
!A or B
(because: false implies anything; otherwise, if "A" is true, then "B"
must be true as well.)
Note that, from this spelling of the "implication operator", it is
evident that A-->B is equivalent to (!B)-->(!A):
!(!B) or (!A)
B or !A
!A or B
(ignoring the short-circuit behavior of the || operator in the C
language, for this discussion).
This is why I wrote that the following two forms were equivalent:
@secure --> REQUIRES_SMM
!REQUIRES_SMM --> !@secure
... Anyway, so we know how to write "@secure --> REQUIRES_SMM" with
logical negation and logical-or. In the "if" condition however, we want
to catch the negation of that condition. Let's calculate that:
!(A --> B) // substitute formula with "or"
!(!A or B) // enter De Morgan, for pushing down the negation
A and !B
(2) Therefore, in the C source code, we should write:
if (def->os.loader &&
def->os.loader->secure == VIR_TRISTATE_BOOL_YES &&
!requiresSMM) {
VIR_DEBUG("Domain restricts pflash programming to SMM, "
"but firmware '%s' doesn't support SMM", path);
return false;
}
... Unfortunately, the term "requires SMM" is quite confusing. It is a
single term that expresses:
- *both* that the firmware *supports* SMM (therefore it makes sense for
the user to restrict pflash r/w access to code that runs in SMM, via
@secure=yes),
- *and* that the firmware *requires* SMM emulation from the underlying
QEMU board (hence the requirement on <smm state='on'/>, later).
Back to the patch:
On 03/07/19 10:29, Michal Privoznik wrote:
> +
> + if (def->sev &&
> + def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV &&
> + !supportsSEV) {
> + VIR_DEBUG("Domain requires SEV, firmware '%s' doesn't support it", path);
> + return false;
> + }
> +
> + VIR_DEBUG("Firmware '%s' matches domain requirements", path);
> + return true;
> +}
> +
> +
> +static int
> +qemuFirmwareEnableFeatures(virQEMUDriverPtr driver,
> + virDomainDefPtr def,
> + const qemuFirmware *fw)
> +{
> + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);
> + const qemuFirmwareMappingFlash *flash = &fw->mapping.data.flash;
> + const qemuFirmwareMappingKernel *kernel = &fw->mapping.data.kernel;
> + const qemuFirmwareMappingMemory *memory = &fw->mapping.data.memory;
> + size_t i;
> +
> + switch (fw->mapping.device) {
> + case QEMU_FIRMWARE_DEVICE_FLASH:
> + if (!def->os.loader &&
> + VIR_ALLOC(def->os.loader) < 0)
> + return -1;
> +
> + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
> + def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
> +
> + if (STRNEQ(flash->executable.format, "raw")) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("unsupported flash format '%s'"),
> + flash->executable.format);
> + return -1;
> + }
> +
> + VIR_FREE(def->os.loader->path);
> + if (VIR_STRDUP(def->os.loader->path,
> + flash->executable.filename) < 0)
> + return -1;
> +
> + if (STRNEQ(flash->nvram_template.format, "raw")) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("unsupported nvram template format '%s'"),
> + flash->nvram_template.format);
> + return -1;
> + }
> +
> + VIR_FREE(def->os.loader->templt);
> + if (VIR_STRDUP(def->os.loader->templt,
> + flash->nvram_template.filename) < 0)
> + return -1;
> +
> + if (qemuDomainNVRAMPathGenerate(cfg, def) < 0)
> + return -1;
> +
> + VIR_DEBUG("decided on firmware '%s' varstore template '%s'",
> + def->os.loader->path,
> + def->os.loader->templt);
> + break;
> +
> + case QEMU_FIRMWARE_DEVICE_KERNEL:
> + VIR_FREE(def->os.kernel);
> + if (VIR_STRDUP(def->os.kernel, kernel->filename) < 0)
> + return -1;
> +
> + VIR_DEBUG("decided on kernel '%s'",
> + def->os.kernel);
> + break;
> +
> + case QEMU_FIRMWARE_DEVICE_MEMORY:
> + if (!def->os.loader &&
> + VIR_ALLOC(def->os.loader) < 0)
> + return -1;
> +
> + def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_ROM;
> + if (VIR_STRDUP(def->os.loader->path, memory->filename) < 0)
> + return -1;
> +
> + VIR_DEBUG("decided on loader '%s'",
> + def->os.loader->path);
> + break;
> +
> + case QEMU_FIRMWARE_DEVICE_NONE:
> + case QEMU_FIRMWARE_DEVICE_LAST:
> + break;
> + }
> +
> + for (i = 0; i < fw->nfeatures; i++) {
> + switch (fw->features[i]) {
> + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> + switch (def->features[VIR_DOMAIN_FEATURE_SMM]) {
> + case VIR_TRISTATE_SWITCH_OFF:
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("domain has SMM turned off "
> + "but chosen firmware requires it"));
> + return -1;
> + break;
> + case VIR_TRISTATE_SWITCH_ABSENT:
> + VIR_DEBUG("Enabling SMM feature");
> + def->features[VIR_DOMAIN_FEATURE_SMM] = VIR_TRISTATE_SWITCH_ON;
> + break;
> +
> + case VIR_TRISTATE_SWITCH_ON:
> + case VIR_TRISTATE_SWITCH_LAST:
> + break;
> + }
> + break;
Right, this is good!
> +
> + case QEMU_FIRMWARE_FEATURE_NONE:
> + case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> + case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> + case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> + case QEMU_FIRMWARE_FEATURE_LAST:
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuFirmwareSanityCheck(const qemuFirmware *fw,
> + const char *filename)
> +{
> + size_t i;
> + bool requiresSMM = false;
> + bool supportsSecureBoot = false;
> +
> + for (i = 0; i < fw->nfeatures; i++) {
> + switch (fw->features[i]) {
> + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM:
> + requiresSMM = true;
> + break;
> + case QEMU_FIRMWARE_FEATURE_SECURE_BOOT:
> + supportsSecureBoot = true;
> + break;
> + case QEMU_FIRMWARE_FEATURE_NONE:
> + case QEMU_FIRMWARE_FEATURE_ACPI_S3:
> + case QEMU_FIRMWARE_FEATURE_ACPI_S4:
> + case QEMU_FIRMWARE_FEATURE_AMD_SEV:
> + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_DYNAMIC:
> + case QEMU_FIRMWARE_FEATURE_VERBOSE_STATIC:
> + case QEMU_FIRMWARE_FEATURE_LAST:
> + break;
> + }
> + }
> +
> + if (supportsSecureBoot != requiresSMM) {
> + VIR_WARN("Firmware description '%s' has invalid set of features: "
> + "%s = %d, %s = %d",
> + filename,
> + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_REQUIRES_SMM),
> + requiresSMM,
> + qemuFirmwareFeatureTypeToString(QEMU_FIRMWARE_FEATURE_SECURE_BOOT),
> + supportsSecureBoot);
> + }
This is also good. I feel tempted to suggest replacing "invalid" with
"dubious" or "questionable", but in fact your wording is clearer and
better. Outside of development, such configs can be considered invalid.
In summary:
- please fix the typo in (1),
- please fix the condition, and the debug message too, in (2),
then you can add:
Reviewed-by: Laszlo Ersek <lersek@xxxxxxxxxx>
Thank you!
Laszlo
> +}
> +
> +
> +int
> +qemuFirmwareFillDomain(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + unsigned int flags)
> +{
> + VIR_AUTOSTRINGLIST paths = NULL;
> + size_t npaths = 0;
> + qemuFirmwarePtr *firmwares = NULL;
> + size_t nfirmwares = 0;
> + const qemuFirmware *theone = NULL;
> + size_t i;
> + int ret = -1;
> +
> + if (!(flags & VIR_QEMU_PROCESS_START_NEW))
> + return 0;
> +
> + if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
> + return 0;
> +
> + if (qemuFirmwareFetchConfigs(&paths) < 0)
> + return -1;
> +
> + npaths = virStringListLength((const char **)paths);
> +
> + if (VIR_ALLOC_N(firmwares, npaths) < 0)
> + return -1;
> +
> + nfirmwares = npaths;
> +
> + for (i = 0; i < nfirmwares; i++) {
> + if (!(firmwares[i] = qemuFirmwareParse(paths[i])))
> + goto cleanup;
> + }
> +
> + for (i = 0; i < nfirmwares; i++) {
> + if (qemuFirmwareMatchDomain(vm->def, firmwares[i], paths[i])) {
> + theone = firmwares[i];
> + VIR_DEBUG("Found matching firmware (description path '%s')",
> + paths[i]);
> + break;
> + }
> + }
> +
> + if (!theone) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("Unable to find any firmware to satisfy '%s'"),
> + virDomainOsDefFirmwareTypeToString(vm->def->os.firmware));
> + goto cleanup;
> + }
> +
> + /* Firstly, let's do some sanity checks. If either of these
> + * fail we can still start the domain successfully, but it's
> + * likely that admin/FW manufacturer messed up. */
> + qemuFirmwareSanityCheck(theone, paths[i]);
> +
> + if (qemuFirmwareEnableFeatures(driver, vm->def, theone) < 0)
> + goto cleanup;
> +
> + vm->def->os.firmware = VIR_DOMAIN_OS_DEF_FIRMWARE_NONE;
> +
> + ret = 0;
> + cleanup:
> + for (i = 0; i < nfirmwares; i++)
> + qemuFirmwareFree(firmwares[i]);
> + VIR_FREE(firmwares);
> + return ret;
> +}
> diff --git a/src/qemu/qemu_firmware.h b/src/qemu/qemu_firmware.h
> index 321169f56c..5d42b8d172 100644
> --- a/src/qemu/qemu_firmware.h
> +++ b/src/qemu/qemu_firmware.h
> @@ -21,7 +21,9 @@
> #ifndef LIBVIRT_QEMU_FIRMWARE_H
> # define LIBVIRT_QEMU_FIRMWARE_H
>
> +# include "domain_conf.h"
> # include "viralloc.h"
> +# include "qemu_conf.h"
>
> typedef struct _qemuFirmware qemuFirmware;
> typedef qemuFirmware *qemuFirmwarePtr;
> @@ -40,4 +42,9 @@ qemuFirmwareFormat(qemuFirmwarePtr fw);
> int
> qemuFirmwareFetchConfigs(char ***firmwares);
>
> +int
> +qemuFirmwareFillDomain(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + unsigned int flags);
> +
> #endif /* LIBVIRT_QEMU_FIRMWARE_H */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
