Re: [libvirt] [PATCH v1 13/15] qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled

2019-03-05 Thread Michal Privoznik 

On 2/28/19 12:40 PM, Laszlo Ersek wrote:

On 02/27/19 11:04, Michal Privoznik wrote:

The firmware selection code will enable the feature if needed.
There's no need to require SMM to be enabled in that case.

Signed-off-by: Michal Privoznik 
---
  src/qemu/qemu_domain.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc3a01397c..b25f26f8b0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4157,7 +4157,9 @@ qemuDomainDefValidate(const virDomainDef *def,
  goto cleanup;
  }
  
-if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) {

+/* SMM will be enabled by qemuFirmwareFillDomain() if needed. */
+if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
+def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
 _("Secure boot requires SMM feature enabled"));
  goto cleanup;



I'm not familiar with the ordering between the code in the previous
patch and the code in this patch. What is the order? Do we first
validate and then prepare/fill, or vice versa?


Yeah, I can probably put this one before 11/15 which enables SMM if needed.



Also, what happens if the domain config explicitly disables SMM
emulation in , but the firmware requires it? See my point (2)
in message
.
Will qemuFirmwareEnableFeatures() simply overwrite the setting? I think
we should reject the firmware descriptor instead, in
qemuFirmwareMatchDomain().


Good point. I'll make qemuFirmwareEnableFeatures() fail in that case and 
thus starting the domain will fail. After all, in point (1) you suggest 
that for matching FW @secure == REQUIRES_SMM, therefore transitively if 
@secure='yes' then def->features[SMM] = VIR_TRISTATE_SWITCH_ON. Hence, 
@secure='yes' and SMM='off' is nonsensical combination.


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 13/15] qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled

2019-02-28 Thread Laszlo Ersek 
On 02/27/19 11:04, Michal Privoznik wrote:
> The firmware selection code will enable the feature if needed.
> There's no need to require SMM to be enabled in that case.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cc3a01397c..b25f26f8b0 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4157,7 +4157,9 @@ qemuDomainDefValidate(const virDomainDef *def,
>  goto cleanup;
>  }
>  
> -if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
> +/* SMM will be enabled by qemuFirmwareFillDomain() if needed. */
> +if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
> +def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) 
> {
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("Secure boot requires SMM feature enabled"));
>  goto cleanup;
> 

I'm not familiar with the ordering between the code in the previous
patch and the code in this patch. What is the order? Do we first
validate and then prepare/fill, or vice versa?

Also, what happens if the domain config explicitly disables SMM
emulation in , but the firmware requires it? See my point (2)
in message
.
Will qemuFirmwareEnableFeatures() simply overwrite the setting? I think
we should reject the firmware descriptor instead, in
qemuFirmwareMatchDomain().

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v1 13/15] qemuDomainDefValidate: Don't require SMM if automatic firmware selection enabled

2019-02-27 Thread Michal Privoznik 
The firmware selection code will enable the feature if needed.
There's no need to require SMM to be enabled in that case.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cc3a01397c..b25f26f8b0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4157,7 +4157,9 @@ qemuDomainDefValidate(const virDomainDef *def,
 goto cleanup;
 }
 
-if (def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) {
+/* SMM will be enabled by qemuFirmwareFillDomain() if needed. */
+if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE &&
+def->features[VIR_DOMAIN_FEATURE_SMM] != VIR_TRISTATE_SWITCH_ON) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Secure boot requires SMM feature enabled"));
 goto cleanup;
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list