Split up the condition and report a different error message when the
host or host config results in S390 PV launch security being
unavailable.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2122534
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/qemu/qemu_validate.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 6403266559..63f3459c90 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1454,11 +1454,14 @@ qemuValidateDomainDef(const virDomainDef *def,
             break;
         case VIR_DOMAIN_LAUNCH_SECURITY_PV:
             if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) ||
-                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST) ||
-                !virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
+                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                               _("S390 PV launch security is not supported with "
-                                 "this QEMU binary"));
+                               _("S390 PV launch security is not supported with this QEMU binary"));
+                return -1;
+            }
+            if (!virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("S390 PV launch security is not supported by this host or kernel"));
                 return -1;
             }
             break;
-- 
2.37.1

Peter Krempa <pkrempa@redhat.com> writes:

> Split up the condition and report a different error message when the
> host or host config results in S390 PV launch security being
> unavailable.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2122534
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/qemu/qemu_validate.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 6403266559..63f3459c90 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -1454,11 +1454,14 @@ qemuValidateDomainDef(const virDomainDef *def,
>              break;
>          case VIR_DOMAIN_LAUNCH_SECURITY_PV:
>              if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) ||
> -                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST) ||
> -                !virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> +                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST)) {
>                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("S390 PV launch security is not supported with "
> -                                 "this QEMU binary"));
> +                               _("S390 PV launch security is not supported with this QEMU binary"));
> +                return -1;
> +            }
> +            if (!virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("S390 PV launch security is not supported by this host or kernel"));

Not sure if the error message is clear enough… PV also depends on the
kernel cmdline opt-in - `prot_virt=1` has to be set.

>                  return -1;
>              }
>              break;
> -- 
> 2.37.1
>

Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>

Thanks for fixing this.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Aug 30, 2022 at 15:17:36 +0200, Marc Hartmayer wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > Split up the condition and report a different error message when the
> > host or host config results in S390 PV launch security being
> > unavailable.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2122534
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/qemu/qemu_validate.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> > index 6403266559..63f3459c90 100644
> > --- a/src/qemu/qemu_validate.c
> > +++ b/src/qemu/qemu_validate.c
> > @@ -1454,11 +1454,14 @@ qemuValidateDomainDef(const virDomainDef *def,
> >              break;
> >          case VIR_DOMAIN_LAUNCH_SECURITY_PV:
> >              if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_CONFIDENTAL_GUEST_SUPPORT) ||
> > -                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST) ||
> > -                !virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> > +                !virQEMUCapsGet(qemuCaps, QEMU_CAPS_S390_PV_GUEST)) {
> >                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                               _("S390 PV launch security is not supported with "
> > -                                 "this QEMU binary"));
> > +                               _("S390 PV launch security is not supported with this QEMU binary"));
> > +                return -1;
> > +            }
> > +            if (!virQEMUCapsGetKVMSupportsSecureGuest(qemuCaps)) {
> > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                               _("S390 PV launch security is not supported by this host or kernel"));
> 
> Not sure if the error message is clear enough… PV also depends on the
> kernel cmdline opt-in - `prot_virt=1` has to be set.

I went for a generic error as there are multiple conditions when the
support is assumed to not be present in virQEMUCapsKVMSupportsSecureGuestS390.

The first condition seems to imply that also host firmware might be
involved and thus asking for the kernel parameter to be enabled might be
misleading.