Re: [libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()
On 03/05/19 15:38, Michal Privoznik wrote: > On 2/28/19 12:15 PM, Laszlo Ersek wrote: >> On 02/27/19 11:04, 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 >>> --- >>> src/qemu/qemu_firmware.c | 257 +++ >>> src/qemu/qemu_firmware.h | 7 ++ >>> 2 files changed, 264 insertions(+) >>> >>> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c >>> index 509927b154..90c611db2d 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" >>> @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) >>> return 0; >>> } >>> + >>> + >>> +static bool >>> +qemuFirmwareMatchDomain(const virDomainDef *def, >>> + const qemuFirmware *fw) >>> +{ >>> + size_t i; >>> + bool supportsS3 = false; >>> + bool supportsS4 = false; >>> + bool supportsSecureBoot = 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) >>> + 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) >>> + 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_SECURE_BOOT: >>> + supportsSecureBoot = true; >>> + break; >>> + case QEMU_FIRMWARE_FEATURE_AMD_SEV: >>> + supportsSEV = true; >>> + break; >>> + case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: >>> + case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: >>> + 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) >>> + return false; >>> + >>> + if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && >>> + !supportsS4) >>> + return false; >>> + >>> + if (def->os.loader && >>> + def->os.loader->secure == VIR_TRISTATE_BOOL_YES && >>> + !supportsSecureBoot) >>> + return false; >> >> This check doesn't seem correct. (Also the fact that >> QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.) > > [This is exactly why I wanted you to take a look at these patches, > because I was almost certain I would get this wrong. Thanks!] > >> >> The @secure attribute controls whether libvirtd generates the "-global >> driver=cfi.pflash01,property=secure,value=on" cmdline option. See >> qemuBuildDomainLoaderCommandLine(). That means that read/write accesses >> ("programming mode") to the pflash chips will be restricted to guest >> code that runs in (guest) SMM.
Re: [libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()
On 2/28/19 12:15 PM, Laszlo Ersek wrote: On 02/27/19 11:04, 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 --- src/qemu/qemu_firmware.c | 257 +++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, +const qemuFirmware *fw) +{ +size_t i; +bool supportsS3 = false; +bool supportsS4 = false; +bool supportsSecureBoot = 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) +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) +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_SECURE_BOOT: +supportsSecureBoot = true; +break; +case QEMU_FIRMWARE_FEATURE_AMD_SEV: +supportsSEV = true; +break; +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: +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) +return false; + +if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && +!supportsS4) +return false; + +if (def->os.loader && +def->os.loader->secure == VIR_TRISTATE_BOOL_YES && +!supportsSecureBoot) +return false; This check doesn't seem correct. (Also the fact that QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.) [This is exactly why I wanted you to take a look at these patches, because I was almost certain I would get this wrong. Thanks!] The @secure attribute controls whether libvirtd generates the "-global driver=cfi.pflash01,property=secure,value=on" cmdline option. See qemuBuildDomainLoaderCommandLine(). That means that read/write accesses ("programming mode") to the pflash chips will be restricted to guest code that runs in (guest) SMM. In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT. Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM, and irrelevant in itself for the QEMU command line. SECURE_BOOT is only relevant to what firmware interfaces are exposed within the guest. Now, security-wise, there *is* a connection between SECURE_BOOT and REQUIRES_SMM. Namely, it is bad practice (for production uses) for firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See the explanation in the schema JSON. So basically, here's what I
Re: [libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()
On 02/27/19 11:04, 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 > --- > src/qemu/qemu_firmware.c | 257 +++ > src/qemu/qemu_firmware.h | 7 ++ > 2 files changed, 264 insertions(+) > > diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c > index 509927b154..90c611db2d 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" > @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) > > return 0; > } > + > + > +static bool > +qemuFirmwareMatchDomain(const virDomainDef *def, > +const qemuFirmware *fw) > +{ > +size_t i; > +bool supportsS3 = false; > +bool supportsS4 = false; > +bool supportsSecureBoot = 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) > +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) > +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_SECURE_BOOT: > +supportsSecureBoot = true; > +break; > +case QEMU_FIRMWARE_FEATURE_AMD_SEV: > +supportsSEV = true; > +break; > +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: > +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: > +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) > +return false; > + > +if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && > +!supportsS4) > +return false; > + > +if (def->os.loader && > +def->os.loader->secure == VIR_TRISTATE_BOOL_YES && > +!supportsSecureBoot) > +return false; This check doesn't seem correct. (Also the fact that QEMU_FIRMWARE_FEATURE_REQUIRES_SMM is ignored above.) The @secure attribute controls whether libvirtd generates the "-global driver=cfi.pflash01,property=secure,value=on" cmdline option. See qemuBuildDomainLoaderCommandLine(). That means that read/write accesses ("programming mode") to the pflash chips will be restricted to guest code that runs in (guest) SMM. In other words, @secure is connected to REQUIRES_SMM, not SECURE_BOOT. Technically speaking, SECURE_BOOT is both independent of REQUIRES_SMM, and irrelevant in itself for the QEMU command line. SECURE_BOOT is only relevant to what firmware interfaces are exposed within the guest. Now, security-wise, there *is* a connection between SECURE_BOOT and REQUIRES_SMM. Namely, it is bad practice (for production uses) for firmware to offer SECURE_BOOT without also specifying REQUIRES_SMM. See the explanation in
[libvirt] [PATCH v1 11/15] qemu_firmware: Introduce qemuFirmwareFillDomain()
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 --- src/qemu/qemu_firmware.c | 257 +++ src/qemu/qemu_firmware.h | 7 ++ 2 files changed, 264 insertions(+) diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c index 509927b154..90c611db2d 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" @@ -1026,3 +1028,258 @@ qemuFirmwareFetchConfigs(char ***firmwares) return 0; } + + +static bool +qemuFirmwareMatchDomain(const virDomainDef *def, +const qemuFirmware *fw) +{ +size_t i; +bool supportsS3 = false; +bool supportsS4 = false; +bool supportsSecureBoot = 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) +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) +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_SECURE_BOOT: +supportsSecureBoot = true; +break; +case QEMU_FIRMWARE_FEATURE_AMD_SEV: +supportsSEV = true; +break; +case QEMU_FIRMWARE_FEATURE_ENROLLED_KEYS: +case QEMU_FIRMWARE_FEATURE_REQUIRES_SMM: +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) +return false; + +if (def->pm.s4 == VIR_TRISTATE_BOOL_YES && +!supportsS4) +return false; + +if (def->os.loader && +def->os.loader->secure == VIR_TRISTATE_BOOL_YES && +!supportsSecureBoot) +return false; + +if (def->sev && +def->sev->sectype == VIR_DOMAIN_LAUNCH_SECURITY_SEV && +!supportsSEV) +return false; + +return true; +} + + +static int +qemuFirmwareEnableFeatures(virQEMUDriverPtr driver, + virDomainDefPtr def, + const qemuFirmware *fw) +{ +VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); +const qemuFirmwareMappingFlash *flash = >mapping.data.flash; +const qemuFirmwareMappingKernel *kernel = >mapping.data.kernel; +const qemuFirmwareMappingMemory *memory = >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; +} + +