[PATCH v3 0/3]s390x/pv: Improve error reporting of protected VMs

[PATCH v3 0/3]s390x/pv: Improve error reporting of protected VMs

[Cédric Le Goater]

Hello,

Here is a little series improving error reporting of protected VMs.

Thanks,

C.

Changes in v3:

 - dropped 's390x/pv: Check for support on the host'. This is already
   covered by the KVM capability.   
 - in s390_pv_check() (patch 2) drop the call to s390_pv_guest_check()
   since init time has already checked the same conditions. However,
   to report an error when the machine is not protected and the kernel
   secure, we still need s390_pv_check().

Changes in v2:

 - dropped ConfidentialGuestSupportClass handler. The check is now
   done from s390_pv_init() which is called after memory and CPU
   initialization. This gives us a better chance to tune the limits
   correctly.
 - pv_max_cpus now computed from the available size of the response
   buffer of the Read SCP Info Service Call (Thomas)
 
Cédric Le Goater (3):
  s390x/pv: Implement a CGS check helper
  s390x/pv: Introduce a s390_pv_check() helper for runtime
  s390x/pv: Move check on hugepage under s390_pv_guest_check()

 include/hw/s390x/pv.h |  2 ++
 hw/s390x/pv.c         | 64 +++++++++++++++++++++++++++++++++++++++++++
 target/s390x/diag.c   |  6 ++--
 3 files changed, 69 insertions(+), 3 deletions(-)

-- 
2.39.0

[PATCH v3 1/3] s390x/pv: Implement a CGS check helper

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

When a protected VM is started with the maximum number of CPUs (248),
the service call providing information on the CPUs requires more
buffer space than allocated and QEMU disgracefully aborts :

    LOADPARM=[........]
    Using virtio-blk.
    Using SCSI scheme.
    ...................................................................................
    qemu-system-s390x: KVM_S390_MEM_OP failed: Argument list too long

When protected virtualization is initialized, compute the maximum
number of vCPUs supported by the machine and return useful information
to the user before the machine starts in case of error.

Suggested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/pv.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8dfe92d8df..8a1c71436b 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -20,6 +20,7 @@
 #include "exec/confidential-guest-support.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/pv.h"
+#include "hw/s390x/sclp.h"
 #include "target/s390x/kvm/kvm_s390x.h"
 
 static bool info_valid;
@@ -249,6 +250,41 @@ struct S390PVGuestClass {
     ConfidentialGuestSupportClass parent_class;
 };
 
+/*
+ * If protected virtualization is enabled, the amount of data that the
+ * Read SCP Info Service Call can use is limited to one page. The
+ * available space also depends on the Extended-Length SCCB (ELS)
+ * feature which can take more buffer space to store feature
+ * information. This impacts the maximum number of CPUs supported in
+ * the machine.
+ */
+static uint32_t s390_pv_get_max_cpus(void)
+{
+    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+        offsetof(ReadInfo, entries) : SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
+
+    return (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry);
+}
+
+static bool s390_pv_check_cpus(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    uint32_t pv_max_cpus = s390_pv_get_max_cpus();
+
+    if (ms->smp.max_cpus > pv_max_cpus) {
+        error_setg(errp, "Protected VMs support a maximum of %d CPUs",
+                   pv_max_cpus);
+        return false;
+    }
+
+    return true;
+}
+
+static bool s390_pv_guest_check(ConfidentialGuestSupport *cgs, Error **errp)
+{
+    return s390_pv_check_cpus(errp);
+}
+
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
     if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
@@ -261,6 +297,10 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         return -1;
     }
 
+    if (!s390_pv_guest_check(cgs, errp)) {
+        return -1;
+    }
+
     cgs->ready = true;
 
     return 0;
-- 
2.39.0

[PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

If a secure kernel is started in a non-protected VM, the OS will hang
during boot without giving a proper error message to the user.

Perform the checks on Confidential Guest support at runtime with an
helper called from the service call switching the guest to protected
mode.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

  In s390_pv_check(), drop the call to s390_pv_guest_check() since
  init time has already checked the same conditions. However, to
  report an error when the machine is not protected and the kernel
  secure, we still need s390_pv_check().

 include/hw/s390x/pv.h |  2 ++
 hw/s390x/pv.c         | 13 +++++++++++++
 target/s390x/diag.c   |  7 +++++++
 3 files changed, 22 insertions(+)

diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 9360aa1091..ca7dac2e20 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -55,6 +55,7 @@ int kvm_s390_dump_init(void);
 int kvm_s390_dump_cpu(S390CPU *cpu, void *buff);
 int kvm_s390_dump_mem_state(uint64_t addr, size_t len, void *dest);
 int kvm_s390_dump_completion_data(void *buff);
+bool s390_pv_check(Error **errp);
 #else /* CONFIG_KVM */
 static inline bool s390_is_pv(void) { return false; }
 static inline int s390_pv_query_info(void) { return 0; }
@@ -75,6 +76,7 @@ static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff) { return 0; }
 static inline int kvm_s390_dump_mem_state(uint64_t addr, size_t len,
                                           void *dest) { return 0; }
 static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
+static inline bool s390_pv_check(Error **errp) { return false; }
 #endif /* CONFIG_KVM */
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8a1c71436b..8405e73a1b 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -306,6 +306,19 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     return 0;
 }
 
+bool s390_pv_check(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+
+    if (!ms->cgs) {
+        error_setg(errp, "Protected VM is started without Confidential"
+                   " Guest support");
+        return false;
+    }
+
+    return true;
+}
+
 OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
                                    s390_pv_guest,
                                    S390_PV_GUEST,
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 76b01dcd68..9b16e25930 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -79,6 +79,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
     uint64_t addr =  env->regs[r1];
     uint64_t subcode = env->regs[r3];
     IplParameterBlock *iplb;
+    Error *local_err = NULL;
 
     if (env->psw.mask & PSW_MASK_PSTATE) {
         s390_program_interrupt(env, PGM_PRIVILEGED, ra);
@@ -176,6 +177,12 @@ out:
             return;
         }
 
+        if (!s390_pv_check(&local_err)) {
+            error_report_err(local_err);
+            env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
+            return;
+        }
+
         s390_ipl_reset_request(cs, S390_RESET_PV);
         break;
     default:
-- 
2.39.0

Re: [PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Thomas Huth]

On 16/01/2023 18.46, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> If a secure kernel is started in a non-protected VM, the OS will hang
> during boot without giving a proper error message to the user.
> 
> Perform the checks on Confidential Guest support at runtime with an
> helper called from the service call switching the guest to protected
> mode.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 
>    In s390_pv_check(), drop the call to s390_pv_guest_check() since
>    init time has already checked the same conditions. However, to
>    report an error when the machine is not protected and the kernel
>    secure, we still need s390_pv_check().

Basically Ack for this patch ... I'm just wondering whether we should maybe 
use a different name for the function. We now have s390_pv_guest_check() and 
390_pv_check() ... hard to distinguish. Maybe we should call them 
s390_pv_initial_check() and s390_pv_runtime_check() (or 
s390_pv_diag308_check()) or something similar instead?

  Thomas

Re: [PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Janosch Frank]

On 1/16/23 18:46, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> If a secure kernel is started in a non-protected VM, the OS will hang
> during boot without giving a proper error message to the user.

Didn't we establish that you were missing the IOMMU flag so this 
statement isn't correct anymore?


I haven't yet fully ingested my coffee, but from what I understand you 
would block a switch into PV mode if cgs is not set. Which would mean 
that PV KVM unit tests wouldn't start anymore as well as any VMs that 
have the unpack feature but not cgs.

And that's not something that we want.

You can start a PV VM without cgs if unpack is in the CPU model. The 
ONLY requirement that we should fail on is unpack.

Have a look at what David Gibson put in the commit message when he 
introduced that in 651615d9:

"""
To integrate this with the option used by other platforms, we
implement the following compromise:

  - When the confidential-guest-support option is set, s390 will
    recognize it, verify that the CPU can support PV (failing if not)
    and set virtio default options necessary for encrypted or protected
    guests, as on other platforms.  i.e. if confidential-guest-support
    is set, we will either create a guest capable of entering PV mode,
    or fail outright.

  - If confidential-guest-support is not set, guests might still be
    able to enter PV mode, if the CPU has the right model.  This may be
    a little surprising, but shouldn't actually be harmful.
"""

Re: [PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Cédric Le Goater]

On 1/17/23 09:40, Janosch Frank wrote:
> On 1/16/23 18:46, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> If a secure kernel is started in a non-protected VM, the OS will hang
>> during boot without giving a proper error message to the user.
> 
> Didn't we establish that you were missing the IOMMU flag so this statement isn't correct anymore?

yes. Which means it is pointless to run the machine because it will fail
to boot with no means to understand why.
  
> I haven't yet fully ingested my coffee, but from what I understand you would block a switch into PV mode if cgs is not set. Which would mean that PV KVM unit tests wouldn't start anymore as well as any VMs that have the unpack feature but not cgs.
>
> 
> And that's not something that we want.
> 
> You can start a PV VM without cgs if unpack is in the CPU model. The ONLY requirement that we should fail on is unpack.

ok.

> Have a look at what David Gibson put in the commit message when he introduced that in 651615d9:
> 
> """
> To integrate this with the option used by other platforms, we
> implement the following compromise:
> 
>   - When the confidential-guest-support option is set, s390 will
>     recognize it, verify that the CPU can support PV (failing if not)
>     and set virtio default options necessary for encrypted or protected
>     guests, as on other platforms.  i.e. if confidential-guest-support
>     is set, we will either create a guest capable of entering PV mode,
>     or fail outright.
> 
>   - If confidential-guest-support is not set, guests might still be
>     able to enter PV mode, if the CPU has the right model.  This may be
>     a little surprising, but shouldn't actually be harmful.
> """

yes and it is not that clear how a s390 PV machine should be started, even
for a developer.

Thanks for looking,

C.
  

Re: [PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Thomas Huth]

On 17/01/2023 09.40, Janosch Frank wrote:
> On 1/16/23 18:46, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> If a secure kernel is started in a non-protected VM, the OS will hang
>> during boot without giving a proper error message to the user.
> 
> Didn't we establish that you were missing the IOMMU flag so this statement 
> isn't correct anymore?
> 
> 
> I haven't yet fully ingested my coffee, but from what I understand you would 
> block a switch into PV mode if cgs is not set. Which would mean that PV KVM 
> unit tests wouldn't start anymore as well as any VMs that have the unpack 
> feature but not cgs.
> 
> And that's not something that we want.
> 
> You can start a PV VM without cgs if unpack is in the CPU model. The ONLY 
> requirement that we should fail on is unpack.

So would it make sense to check for S390_FEAT_UNPACK (or something else?) 
here, or should the patch completely be dropped instead?

  Thomas

Re: [PATCH v3 2/3] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Janosch Frank]

On 1/17/23 10:09, Thomas Huth wrote:
> On 17/01/2023 09.40, Janosch Frank wrote:
>> On 1/16/23 18:46, Cédric Le Goater wrote:
>>> From: Cédric Le Goater <clg@redhat.com>
>>>
>>> If a secure kernel is started in a non-protected VM, the OS will hang
>>> during boot without giving a proper error message to the user.
>>
>> Didn't we establish that you were missing the IOMMU flag so this statement
>> isn't correct anymore?
>>
>>
>> I haven't yet fully ingested my coffee, but from what I understand you would
>> block a switch into PV mode if cgs is not set. Which would mean that PV KVM
>> unit tests wouldn't start anymore as well as any VMs that have the unpack
>> feature but not cgs.
>>
>> And that's not something that we want.
>>
>> You can start a PV VM without cgs if unpack is in the CPU model. The ONLY
>> requirement that we should fail on is unpack.
> 
> So would it make sense to check for S390_FEAT_UNPACK (or something else?)
> here, or should the patch completely be dropped instead?

Subcodes 8 - 10 already result in a spec PGM for !unpack and if memory 
serves me right then Marc will catch that and print a warning in the 
genprotimg kernel:

if (subcode >= DIAG308_PV_SET && !s390_has_feat(S390_FEAT_UNPACK)) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
}

So other than shuffling code around I see no benefit in this patch.

[PATCH v3 3/3] s390x/pv: Move check on hugepage under s390_pv_guest_check()

[Cédric Le Goater]

From: Cédric Le Goater <clg@redhat.com>

Such conditions on Protected Virtualization can now be checked at init
time. This is possible because Protected Virtualization is initialized
after memory where the page size is set.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/pv.c       | 13 ++++++++++++-
 target/s390x/diag.c |  7 -------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8405e73a1b..679d860f54 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -280,9 +280,20 @@ static bool s390_pv_check_cpus(Error **errp)
     return true;
 }
 
+static bool s390_pv_check_hpage(Error **errp)
+{
+    if (kvm_s390_get_hpage_1m()) {
+        error_setg(errp, "Protected VMs can currently not be backed with "
+                   "huge pages");
+        return false;
+    }
+
+    return true;
+}
+
 static bool s390_pv_guest_check(ConfidentialGuestSupport *cgs, Error **errp)
 {
-    return s390_pv_check_cpus(errp);
+    return s390_pv_check_cpus(errp) && s390_pv_check_hpage(errp);
 }
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 9b16e25930..28f4350aed 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -170,13 +170,6 @@ out:
             return;
         }
 
-        if (kvm_enabled() && kvm_s390_get_hpage_1m()) {
-            error_report("Protected VMs can currently not be backed with "
-                         "huge pages");
-            env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
-            return;
-        }
-
         if (!s390_pv_check(&local_err)) {
             error_report_err(local_err);
             env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
-- 
2.39.0