7:21 a.m.
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(a)redhat.com>
---
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@redhat.com
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(a)redhat.com>
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 */