[PATCH 0/5] s390x/pv: Improve protected VM support

[PATCH 0/5] s390x/pv: Improve protected VM support

[Cédric Le Goater]

Hello,

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

Thanks,

C.

Cédric Le Goater (5):
  confidential guest support: Introduce a 'check' class handler
  s390x/pv: Implement CGS check handler
  s390x/pv: Check for support on the host
  s390x/pv: Introduce a s390_pv_check() helper for runtime
  s390x/pv: Move check on hugepage under s390_pv_guest_check()

 include/exec/confidential-guest-support.h |  4 +-
 include/hw/s390x/pv.h                     |  2 +
 hw/core/machine.c                         | 11 ++--
 hw/s390x/pv.c                             | 70 +++++++++++++++++++++++
 target/s390x/diag.c                       |  6 +-
 5 files changed, 84 insertions(+), 9 deletions(-)

-- 
2.38.1

[PATCH 1/5] confidential guest support: Introduce a 'check' class handler

[Cédric Le Goater]

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

Some machines have specific requirements to activate confidential
guest support. Add a class handler to the confidential guest support
interface to let the arch implementation perform extra checks.

Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Yanan Wang <wangyanan55@huawei.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/exec/confidential-guest-support.h |  4 +++-
 hw/core/machine.c                         | 11 ++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5df..9e6d362b26 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -23,7 +23,8 @@
 #include "qom/object.h"
 
 #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
-OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
+OBJECT_DECLARE_TYPE(ConfidentialGuestSupport, ConfidentialGuestSupportClass,
+                    CONFIDENTIAL_GUEST_SUPPORT)
 
 struct ConfidentialGuestSupport {
     Object parent;
@@ -55,6 +56,7 @@ struct ConfidentialGuestSupport {
 
 typedef struct ConfidentialGuestSupportClass {
     ObjectClass parent;
+    bool (*check)(const Object *obj, Error **errp);
 } ConfidentialGuestSupportClass;
 
 #endif /* !CONFIG_USER_ONLY */
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f589b92909..bab43cd675 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -502,11 +502,12 @@ static void machine_check_confidential_guest_support(const Object *obj,
                                                      Object *new_target,
                                                      Error **errp)
 {
-    /*
-     * So far the only constraint is that the target has the
-     * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
-     * by the QOM core
-     */
+    ConfidentialGuestSupportClass *cgsc =
+        CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(new_target);
+
+    if (cgsc->check) {
+        cgsc->check(obj, errp);
+    }
 }
 
 static bool machine_get_nvdimm(Object *obj, Error **errp)
-- 
2.38.1

[PATCH 2/5] s390x/pv: Implement CGS check handler

[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

Implement a test for this limitation in the ConfidentialGuestSupportClass
check handler and provide some valid information to the user before the
machine starts.

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

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8dfe92d8df..3a7ec70634 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     return 0;
 }
 
+static bool s390_pv_check_cpus(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    uint32_t pv_max_cpus = mc->max_cpus - 1;
+
+    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(const Object *obj, Error **errp)
+{
+    return s390_pv_check_cpus(errp);
+}
+
 OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
                                    s390_pv_guest,
                                    S390_PV_GUEST,
@@ -275,6 +295,9 @@ OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
 
 static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
 {
+    ConfidentialGuestSupportClass *cgsc = CONFIDENTIAL_GUEST_SUPPORT_CLASS(oc);
+
+    cgsc->check = s390_pv_guest_check;
 }
 
 static void s390_pv_guest_init(Object *obj)
-- 
2.38.1

[PATCH 3/5] s390x/pv: Check for support on the host

[Cédric Le Goater]

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

Support for protected VMs should have been enabled on the host with
the kernel parameter 'prot_virt=1'. If the hardware supports the
feature, it is reflected under sysfs.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/s390x/pv.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 3a7ec70634..8d0d3f4adc 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -14,6 +14,7 @@
 #include <linux/kvm.h>
 
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
 #include "qom/object_interfaces.h"
@@ -281,9 +282,29 @@ static bool s390_pv_check_cpus(Error **errp)
     return true;
 }
 
+#define S390_PV_HOST "/sys/firmware/uv/prot_virt_host"
+
+static bool s390_pv_check_host(Error **errp)
+{
+    gchar *s = NULL;
+    uint64_t pv_host = 0;
+
+    if (g_file_get_contents(S390_PV_HOST, &s, NULL, NULL)) {
+        pv_host = g_ascii_strtoull(s, NULL, 10);
+    }
+    g_free(s);
+
+    if (pv_host != 1) {
+        error_setg(errp, "Host does not support protected VMs");
+        return false;
+    }
+
+    return true;
+}
+
 static bool s390_pv_guest_check(const Object *obj, Error **errp)
 {
-    return s390_pv_check_cpus(errp);
+    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
 }
 
 OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
-- 
2.38.1

[PATCH 4/5] 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>
---
 include/hw/s390x/pv.h |  2 ++
 hw/s390x/pv.c         | 14 ++++++++++++++
 target/s390x/diag.c   |  7 +++++++
 3 files changed, 23 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 8d0d3f4adc..96c0728ec9 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -307,6 +307,20 @@ static bool s390_pv_guest_check(const Object *obj, Error **errp)
     return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
 }
 
+bool s390_pv_check(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    Object *obj = OBJECT(ms->cgs);
+
+    if (!obj) {
+        error_setg(errp, "Protected VM started without a Confidential"
+                   " Guest support interface");
+        return false;
+    }
+
+    return s390_pv_guest_check(obj, errp);
+}
+
 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.38.1

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

[Cédric Le Goater]

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

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

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 96c0728ec9..4e1f991d98 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -302,9 +302,21 @@ static bool s390_pv_check_host(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(const Object *obj, Error **errp)
 {
-    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
+    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp) &&
+        s390_pv_check_hpage(errp);
 }
 
 bool s390_pv_check(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.38.1

Re: [PATCH 1/5] confidential guest support: Introduce a 'check' class handler

[Thomas Huth]

On 04/01/2023 12.51, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Some machines have specific requirements to activate confidential
> guest support. Add a class handler to the confidential guest support
> interface to let the arch implementation perform extra checks.
> 
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/exec/confidential-guest-support.h |  4 +++-
>   hw/core/machine.c                         | 11 ++++++-----
>   2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index ba2dd4b5df..9e6d362b26 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -23,7 +23,8 @@
>   #include "qom/object.h"
>   
>   #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> -OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
> +OBJECT_DECLARE_TYPE(ConfidentialGuestSupport, ConfidentialGuestSupportClass,
> +                    CONFIDENTIAL_GUEST_SUPPORT)
>   
>   struct ConfidentialGuestSupport {
>       Object parent;
> @@ -55,6 +56,7 @@ struct ConfidentialGuestSupport {
>   
>   typedef struct ConfidentialGuestSupportClass {
>       ObjectClass parent;
> +    bool (*check)(const Object *obj, Error **errp);
>   } ConfidentialGuestSupportClass;
>   
>   #endif /* !CONFIG_USER_ONLY */
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f589b92909..bab43cd675 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -502,11 +502,12 @@ static void machine_check_confidential_guest_support(const Object *obj,
>                                                        Object *new_target,
>                                                        Error **errp)
>   {
> -    /*
> -     * So far the only constraint is that the target has the
> -     * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
> -     * by the QOM core
> -     */
> +    ConfidentialGuestSupportClass *cgsc =
> +        CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(new_target);
> +
> +    if (cgsc->check) {
> +        cgsc->check(obj, errp);

I assume the caller is checking *errp, so it's ok to ignore the return value 
of the check function here?

> +    }
>   }
>   
>   static bool machine_get_nvdimm(Object *obj, Error **errp)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/5] confidential guest support: Introduce a 'check' class handler

[Philippe Mathieu-Daudé]

On 5/1/23 09:46, Thomas Huth wrote:
> On 04/01/2023 12.51, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> Some machines have specific requirements to activate confidential
>> guest support. Add a class handler to the confidential guest support
>> interface to let the arch implementation perform extra checks.
>>
>> Cc: Eduardo Habkost <eduardo@habkost.net>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>> Cc: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/exec/confidential-guest-support.h |  4 +++-
>>   hw/core/machine.c                         | 11 ++++++-----
>>   2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/exec/confidential-guest-support.h 
>> b/include/exec/confidential-guest-support.h
>> index ba2dd4b5df..9e6d362b26 100644
>> --- a/include/exec/confidential-guest-support.h
>> +++ b/include/exec/confidential-guest-support.h
>> @@ -23,7 +23,8 @@
>>   #include "qom/object.h"
>>   #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
>> -OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, 
>> CONFIDENTIAL_GUEST_SUPPORT)
>> +OBJECT_DECLARE_TYPE(ConfidentialGuestSupport, 
>> ConfidentialGuestSupportClass,
>> +                    CONFIDENTIAL_GUEST_SUPPORT)
>>   struct ConfidentialGuestSupport {
>>       Object parent;
>> @@ -55,6 +56,7 @@ struct ConfidentialGuestSupport {
>>   typedef struct ConfidentialGuestSupportClass {
>>       ObjectClass parent;
>> +    bool (*check)(const Object *obj, Error **errp);
>>   } ConfidentialGuestSupportClass;
>>   #endif /* !CONFIG_USER_ONLY */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index f589b92909..bab43cd675 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -502,11 +502,12 @@ static void 
>> machine_check_confidential_guest_support(const Object *obj,
>>                                                        Object 
>> *new_target,
>>                                                        Error **errp)
>>   {
>> -    /*
>> -     * So far the only constraint is that the target has the
>> -     * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
>> -     * by the QOM core
>> -     */
>> +    ConfidentialGuestSupportClass *cgsc =
>> +        CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(new_target);
>> +
>> +    if (cgsc->check) {
>> +        cgsc->check(obj, errp);
> 
> I assume the caller is checking *errp, so it's ok to ignore the return 
> value of the check function here?

Agreed, can we change by:

   if (cgsc->check && !cgsc->check(obj, errp)) {
       return;
   }

?

Also since the handler name is not very self-descriptive, can we
add a comment in its declaration in ConfidentialGuestSupportClass?

>> +    }
>>   }

Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Thomas Huth]

On 04/01/2023 12.51, Cédric Le Goater wrote:
> 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
> 
> Implement a test for this limitation in the ConfidentialGuestSupportClass
> check handler and provide some valid information to the user before the
> machine starts.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/s390x/pv.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 8dfe92d8df..3a7ec70634 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>       return 0;
>   }
>   
> +static bool s390_pv_check_cpus(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    uint32_t pv_max_cpus = mc->max_cpus - 1;

Not sure whether "mc->max_cpus - 1" is the right approach here. I think it 
would be better to calculate the amount of CPUs that we can support.

So AFAIK the problem is that SCLP information that is gathered during 
read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled, 
everything has to fit in one page (i.e. 4096 bytes) there.

So we have space for

  (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)

CPUs.

With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct 
ReadInfo in sclp.h), otherwise it is 128.

That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:

  (4096 - 144) / 16 = 247 CPUs

which is what you were trying to check with the mc->max_cpus - 1 here.

But with "-cpu els=off", it sounds like we could fit all 248 also with 
protected VMs? Could you please give it a try?

Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use 
something like this instead:

  int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
                      offsetof(ReadInfo, entries) :
                      SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
  pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);

   Thomas

Re: [PATCH 3/5] s390x/pv: Check for support on the host

[Thomas Huth]

On 04/01/2023 12.51, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Support for protected VMs should have been enabled on the host with
> the kernel parameter 'prot_virt=1'. If the hardware supports the
> feature, it is reflected under sysfs.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/s390x/pv.c | 23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 3a7ec70634..8d0d3f4adc 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -14,6 +14,7 @@
>   #include <linux/kvm.h>
>   
>   #include "qapi/error.h"
> +#include "qemu/cutils.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/kvm.h"
>   #include "qom/object_interfaces.h"
> @@ -281,9 +282,29 @@ static bool s390_pv_check_cpus(Error **errp)
>       return true;
>   }
>   
> +#define S390_PV_HOST "/sys/firmware/uv/prot_virt_host"
> +
> +static bool s390_pv_check_host(Error **errp)
> +{
> +    gchar *s = NULL;
> +    uint64_t pv_host = 0;
> +
> +    if (g_file_get_contents(S390_PV_HOST, &s, NULL, NULL)) {
> +        pv_host = g_ascii_strtoull(s, NULL, 10);
> +    }
> +    g_free(s);
> +
> +    if (pv_host != 1) {
> +        error_setg(errp, "Host does not support protected VMs");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>   static bool s390_pv_guest_check(const Object *obj, Error **errp)
>   {
> -    return s390_pv_check_cpus(errp);
> +    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>   }
>   
>   OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 4/5] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Thomas Huth]

On 04/01/2023 12.51, 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>
> ---
>   include/hw/s390x/pv.h |  2 ++
>   hw/s390x/pv.c         | 14 ++++++++++++++
>   target/s390x/diag.c   |  7 +++++++
>   3 files changed, 23 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 8d0d3f4adc..96c0728ec9 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -307,6 +307,20 @@ static bool s390_pv_guest_check(const Object *obj, Error **errp)
>       return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>   }
>   
> +bool s390_pv_check(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    Object *obj = OBJECT(ms->cgs);
> +
> +    if (!obj) {
> +        error_setg(errp, "Protected VM started without a Confidential"
> +                   " Guest support interface");
> +        return false;
> +    }
> +
> +    return s390_pv_guest_check(obj, errp);
> +}
> +
>   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;

I hope someone from IBM can double-check whether that return code is fine in 
this case here.

If so, the patch looks fine to me.

  Thomas


> +            return;
> +        }
> +
>           s390_ipl_reset_request(cs, S390_RESET_PV);
>           break;
>       default:

Re: [PATCH 5/5] s390x/pv: Move check on hugepage under s390_pv_guest_check()

[Thomas Huth]

On 04/01/2023 12.51, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/s390x/pv.c       | 14 +++++++++++++-
>   target/s390x/diag.c |  7 -------
>   2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 96c0728ec9..4e1f991d98 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -302,9 +302,21 @@ static bool s390_pv_check_host(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(const Object *obj, Error **errp)
>   {
> -    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
> +    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp) &&
> +        s390_pv_check_hpage(errp);
>   }
>   
>   bool s390_pv_check(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;

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 1/5] confidential guest support: Introduce a 'check' class handler

[Cédric Le Goater]

On 1/5/23 09:46, Thomas Huth wrote:
> On 04/01/2023 12.51, Cédric Le Goater wrote:
>> From: Cédric Le Goater <clg@redhat.com>
>>
>> Some machines have specific requirements to activate confidential
>> guest support. Add a class handler to the confidential guest support
>> interface to let the arch implementation perform extra checks.
>>
>> Cc: Eduardo Habkost <eduardo@habkost.net>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>> Cc: Yanan Wang <wangyanan55@huawei.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/exec/confidential-guest-support.h |  4 +++-
>>   hw/core/machine.c                         | 11 ++++++-----
>>   2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
>> index ba2dd4b5df..9e6d362b26 100644
>> --- a/include/exec/confidential-guest-support.h
>> +++ b/include/exec/confidential-guest-support.h
>> @@ -23,7 +23,8 @@
>>   #include "qom/object.h"
>>   #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
>> -OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport, CONFIDENTIAL_GUEST_SUPPORT)
>> +OBJECT_DECLARE_TYPE(ConfidentialGuestSupport, ConfidentialGuestSupportClass,
>> +                    CONFIDENTIAL_GUEST_SUPPORT)
>>   struct ConfidentialGuestSupport {
>>       Object parent;
>> @@ -55,6 +56,7 @@ struct ConfidentialGuestSupport {
>>   typedef struct ConfidentialGuestSupportClass {
>>       ObjectClass parent;
>> +    bool (*check)(const Object *obj, Error **errp);
>>   } ConfidentialGuestSupportClass;
>>   #endif /* !CONFIG_USER_ONLY */
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index f589b92909..bab43cd675 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -502,11 +502,12 @@ static void machine_check_confidential_guest_support(const Object *obj,
>>                                                        Object *new_target,
>>                                                        Error **errp)
>>   {
>> -    /*
>> -     * So far the only constraint is that the target has the
>> -     * TYPE_CONFIDENTIAL_GUEST_SUPPORT interface, and that's checked
>> -     * by the QOM core
>> -     */
>> +    ConfidentialGuestSupportClass *cgsc =
>> +        CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(new_target);
>> +
>> +    if (cgsc->check) {
>> +        cgsc->check(obj, errp);
> 
> I assume the caller is checking *errp, so it's ok to ignore the return value of the check function here?

yes. routine machine_check_confidential_guest_support() is a direct parameter
of object_class_property_add_link() call, which checks *errp. See routine
object_set_link_property().

Thanks,

C.

> 
>> +    }
>>   }
>>   static bool machine_get_nvdimm(Object *obj, Error **errp)
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Claudio Imbrenda]

On Thu, 5 Jan 2023 12:42:54 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 04/01/2023 12.51, Cédric Le Goater wrote:
> > 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
> > 
> > Implement a test for this limitation in the ConfidentialGuestSupportClass
> > check handler and provide some valid information to the user before the
> > machine starts.
> > 
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > ---
> >   hw/s390x/pv.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> > 
> > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> > index 8dfe92d8df..3a7ec70634 100644
> > --- a/hw/s390x/pv.c
> > +++ b/hw/s390x/pv.c
> > @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >       return 0;
> >   }
> >   
> > +static bool s390_pv_check_cpus(Error **errp)
> > +{
> > +    MachineState *ms = MACHINE(qdev_get_machine());
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +    uint32_t pv_max_cpus = mc->max_cpus - 1;  
> 
> Not sure whether "mc->max_cpus - 1" is the right approach here. I think it 
> would be better to calculate the amount of CPUs that we can support.
> 
> So AFAIK the problem is that SCLP information that is gathered during 
> read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled, 
> everything has to fit in one page (i.e. 4096 bytes) there.
> 
> So we have space for
> 
>   (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)
> 
> CPUs.
> 
> With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct 
> ReadInfo in sclp.h), otherwise it is 128.
> 
> That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:
> 
>   (4096 - 144) / 16 = 247 CPUs
> 
> which is what you were trying to check with the mc->max_cpus - 1 here.
> 
> But with "-cpu els=off", it sounds like we could fit all 248 also with 
> protected VMs? Could you please give it a try?
> 
> Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use 
> something like this instead:
> 
>   int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
>                       offsetof(ReadInfo, entries) :
>                       SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
>   pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);

I agree with Thomas here

> 
>    Thomas
> 
> 

Re: [PATCH 4/5] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Claudio Imbrenda]

On Wed,  4 Jan 2023 12:51:10 +0100
Cédric Le Goater <clg@kaod.org> 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>
> ---
>  include/hw/s390x/pv.h |  2 ++
>  hw/s390x/pv.c         | 14 ++++++++++++++
>  target/s390x/diag.c   |  7 +++++++
>  3 files changed, 23 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 8d0d3f4adc..96c0728ec9 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -307,6 +307,20 @@ static bool s390_pv_guest_check(const Object *obj, Error **errp)
>      return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>  }
>  
> +bool s390_pv_check(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    Object *obj = OBJECT(ms->cgs);
> +
> +    if (!obj) {
> +        error_setg(errp, "Protected VM started without a Confidential"
> +                   " Guest support interface");
> +        return false;
> +    }
> +
> +    return s390_pv_guest_check(obj, errp);
> +}
> +
>  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;
> +        }
> +

in general yes

however I have noticed that we don't always return a PGM_SPECIFICATION
when PV is not available (we currently do it only for DIAG308_PV_SET). I
think we should return the PGM_SPECIFICATION for all PV subcodes when
the feature is not present (but this is a separate issue)

let me add Janosch in CC since he wrote that code

>          s390_ipl_reset_request(cs, S390_RESET_PV);
>          break;
>      default:

Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Cédric Le Goater]

On 1/5/23 14:58, Claudio Imbrenda wrote:
> On Thu, 5 Jan 2023 12:42:54 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 04/01/2023 12.51, Cédric Le Goater wrote:
>>> 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
>>>
>>> Implement a test for this limitation in the ConfidentialGuestSupportClass
>>> check handler and provide some valid information to the user before the
>>> machine starts.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>    hw/s390x/pv.c | 23 +++++++++++++++++++++++
>>>    1 file changed, 23 insertions(+)
>>>
>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>>> index 8dfe92d8df..3a7ec70634 100644
>>> --- a/hw/s390x/pv.c
>>> +++ b/hw/s390x/pv.c
>>> @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>>        return 0;
>>>    }
>>>    
>>> +static bool s390_pv_check_cpus(Error **errp)
>>> +{
>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>> +    uint32_t pv_max_cpus = mc->max_cpus - 1;
>>
>> Not sure whether "mc->max_cpus - 1" is the right approach here. I think it
>> would be better to calculate the amount of CPUs that we can support.
>>
>> So AFAIK the problem is that SCLP information that is gathered during
>> read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled,
>> everything has to fit in one page (i.e. 4096 bytes) there.
>>
>> So we have space for
>>
>>    (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)
>>
>> CPUs.
>>
>> With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct
>> ReadInfo in sclp.h), otherwise it is 128.
>>
>> That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:
>>
>>    (4096 - 144) / 16 = 247 CPUs
>>
>> which is what you were trying to check with the mc->max_cpus - 1 here.

yes. That's much better.

>> But with "-cpu els=off", it sounds like we could fit all 248 also with
>> protected VMs? Could you please give it a try?

It runs. Unfortunately, QEMU also complains with :

   qemu-system-s390x: warning: 'diag318' requires 'els'.
   qemu-system-s390x: warning: 'diag318' requires 'els'.
   qemu-system-s390x: warning: 'diag318' requires 'els'.

when els is off.


>> Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use
>> something like this instead:
>>
>>    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
>>                        offsetof(ReadInfo, entries) :
>>                        SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
>>    pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
> 
> I agree with Thomas here


The problem is that QEMU doesn't know about the S390_FEAT_EXTENDED_LENGTH_SCCB
feature when the PV object link is first checked. So #248 CPUs is considered
valid, but when DIAG308_PV_START is called, it fails.

Let's simplify and use :

     int offset_cpu = offsetof(ReadInfo, entries);
     pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);

?

Thanks,

C.

Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Thomas Huth]

On 05/01/2023 15.47, Cédric Le Goater wrote:
> On 1/5/23 14:58, Claudio Imbrenda wrote:
>> On Thu, 5 Jan 2023 12:42:54 +0100
>> Thomas Huth <thuth@redhat.com> wrote:
>>
>>> On 04/01/2023 12.51, Cédric Le Goater wrote:
>>>> 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
>>>>
>>>> Implement a test for this limitation in the ConfidentialGuestSupportClass
>>>> check handler and provide some valid information to the user before the
>>>> machine starts.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>> ---
>>>>    hw/s390x/pv.c | 23 +++++++++++++++++++++++
>>>>    1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>>>> index 8dfe92d8df..3a7ec70634 100644
>>>> --- a/hw/s390x/pv.c
>>>> +++ b/hw/s390x/pv.c
>>>> @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, 
>>>> Error **errp)
>>>>        return 0;
>>>>    }
>>>> +static bool s390_pv_check_cpus(Error **errp)
>>>> +{
>>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>>> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>>>> +    uint32_t pv_max_cpus = mc->max_cpus - 1;
>>>
>>> Not sure whether "mc->max_cpus - 1" is the right approach here. I think it
>>> would be better to calculate the amount of CPUs that we can support.
>>>
>>> So AFAIK the problem is that SCLP information that is gathered during
>>> read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled,
>>> everything has to fit in one page (i.e. 4096 bytes) there.
>>>
>>> So we have space for
>>>
>>>    (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)
>>>
>>> CPUs.
>>>
>>> With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct
>>> ReadInfo in sclp.h), otherwise it is 128.
>>>
>>> That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:
>>>
>>>    (4096 - 144) / 16 = 247 CPUs
>>>
>>> which is what you were trying to check with the mc->max_cpus - 1 here.
> 
> yes. That's much better.
> 
>>> But with "-cpu els=off", it sounds like we could fit all 248 also with
>>> protected VMs? Could you please give it a try?
> 
> It runs. Unfortunately, QEMU also complains with :
> 
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
>    qemu-system-s390x: warning: 'diag318' requires 'els'.
> 
> when els is off.

There is also a switch for that: -cpu els=off,diag318=off

>>> Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use
>>> something like this instead:
>>>
>>>    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
>>>                        offsetof(ReadInfo, entries) :
>>>                        SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
>>>    pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
>>
>> I agree with Thomas here
> 
> 
> The problem is that QEMU doesn't know about the S390_FEAT_EXTENDED_LENGTH_SCCB
> feature when the PV object link is first checked. So #248 CPUs is considered
> valid, but when DIAG308_PV_START is called, it fails.

Drat. Is there any chance that the check could be done somewhere later?

> Let's simplify and use :
> 
>      int offset_cpu = offsetof(ReadInfo, entries);
>      pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
> 
> ?

Depends ... if it is possible to use 248 CPUs with -cpu els=off,diag318=off 
then it would be nicer to allow that, too?

  Thomas

Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Cédric Le Goater]

[ .. ]

>> The problem is that QEMU doesn't know about the S390_FEAT_EXTENDED_LENGTH_SCCB
>> feature when the PV object link is first checked. So #248 CPUs is considered
>> valid, but when DIAG308_PV_START is called, it fails.
> 
> Drat. Is there any chance that the check could be done somewhere later?
  
s390_pv_init() could be the place. It looks like a good option since the CPUs have
been initialized. To be explored.

Thanks,

C.


>> Let's simplify and use :
>>
>>      int offset_cpu = offsetof(ReadInfo, entries);
>>      pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
>>
>> ?
> 
> Depends ... if it is possible to use 248 CPUs with -cpu els=off,diag318=off then it would be nicer to allow that, too?
> 
>   Thomas
>