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

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

[Cédric Le Goater]

Hello,

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

Thanks,

C.

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 (4):
  s390x/pv: Implement a CGS check helper
  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/hw/s390x/pv.h |  2 +
 hw/s390x/pv.c         | 86 +++++++++++++++++++++++++++++++++++++++++++
 target/s390x/diag.c   |  6 +--
 3 files changed, 91 insertions(+), 3 deletions(-)

-- 
2.38.1

[PATCH v2 1/4] 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>
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.38.1

Re: [PATCH v2 1/4] s390x/pv: Implement a CGS check helper

[Thomas Huth]

On 06/01/2023 08.53, 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
> 
> 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>
> 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;

Looks good to me now.

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

Re: [PATCH v2 1/4] s390x/pv: Implement a CGS check helper

[Cédric Le Goater]

On 1/9/23 14:34, Thomas Huth wrote:
> On 06/01/2023 08.53, 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
>>
>> 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>
>> 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;
> 
> Looks good to me now.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I think we could move the huge page test in s390_pv_guest_check() also.
We are finishing a discussion with Janosch on the runtime test and I will
send a v3.

Thanks,

C.

Re: [PATCH v2 1/4] s390x/pv: Implement a CGS check helper

[Thomas Huth]

On 09/01/2023 14.57, Cédric Le Goater wrote:
> On 1/9/23 14:34, Thomas Huth wrote:
>> On 06/01/2023 08.53, 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
>>>
>>> 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>
>>> 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;
>>
>> Looks good to me now.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> I think we could move the huge page test in s390_pv_guest_check() also.
> We are finishing a discussion with Janosch on the runtime test and I will
> send a v3.

Core question is likely: What if the hypervisor admin does not know whether 
the guest will run in protected mode or not, and thus always wants to enable 
the feature (so that the owner of the guest can decide)? So we cannot know 
right from the start whether we have a confidential guest or not? ... should 
we then really check the condition at the beginning, or is it better to 
check when the guest tries to switch to protected mode?

  Thomas

Re: [PATCH v2 1/4] s390x/pv: Implement a CGS check helper

[Cédric Le Goater]

On 1/9/23 15:12, Thomas Huth wrote:
> On 09/01/2023 14.57, Cédric Le Goater wrote:
>> On 1/9/23 14:34, Thomas Huth wrote:
>>> On 06/01/2023 08.53, 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
>>>>
>>>> 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>
>>>> 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;
>>>
>>> Looks good to me now.
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>
>> I think we could move the huge page test in s390_pv_guest_check() also.
>> We are finishing a discussion with Janosch on the runtime test and I will
>> send a v3.
> 
> Core question is likely: What if the hypervisor admin does not know whether the guest will run in protected mode or not, and thus always wants to enable the feature (so that the owner of the guest can decide)? 

If we take this direction, then, on a protected host, libvirt (or QEMU ?)
would need to set the VM as protected always : machine option and CGS
object passed on the command line.

> So we cannot know right from the start whether we have a confidential 
> guest or not? ... 

AFAIUI, it only depends on the kernel of the guest OS. A non secure
kernel runs fine in a protected VM. The PV switch is not performed
in diag308.

C.


> should we then really check the condition at the beginning, 
> or is it better to check when the guest tries to switch to protected mode?

[PATCH v2 2/4] 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.

Reviewed-by: Thomas Huth <thuth@redhat.com>
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 8a1c71436b..d53ef8fd38 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"
@@ -280,9 +281,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(ConfidentialGuestSupport *cgs, Error **errp)
 {
-    return s390_pv_check_cpus(errp);
+    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
 }
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
-- 
2.38.1

Re: [PATCH v2 2/4] s390x/pv: Check for support on the host

[Janosch Frank]

On 1/6/23 08:53, 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.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

Any reason why you didn't use KVM_CAP_S390_PROTECTED?

The sysfs interface isn't meant to be parsed by programs, it's been 
introduced for humans. Most of the interface's data has therefore been 
made available via the UV info API.

> ---
>   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 8a1c71436b..d53ef8fd38 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"
> @@ -280,9 +281,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(ConfidentialGuestSupport *cgs, Error **errp)
>   {
> -    return s390_pv_check_cpus(errp);
> +    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>   }
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)

Re: [PATCH v2 2/4] s390x/pv: Check for support on the host

[Cédric Le Goater]

On 1/9/23 09:45, Janosch Frank wrote:
> On 1/6/23 08:53, 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.
>>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> 
> Any reason why you didn't use KVM_CAP_S390_PROTECTED?

I think my setup was incorrect when I did the patch. I just verified and QEMU
indeed reports :

  qemu-system-s390x: CPU model does not support Protected Virtualization

which means S390_FEAT_UNPACK was not set.


> The sysfs interface isn't meant to be parsed by programs, it's been introduced for humans. Most of the interface's data has therefore been made available via the UV info API.

Well, QEMU is user space and does peek around in sysfs to collect some info.
Unneeded in that case.

Thanks,

C.



>> ---
>>   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 8a1c71436b..d53ef8fd38 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"
>> @@ -280,9 +281,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(ConfidentialGuestSupport *cgs, Error **errp)
>>   {
>> -    return s390_pv_check_cpus(errp);
>> +    return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>>   }
>>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> 

Re: [PATCH v2 2/4] s390x/pv: Check for support on the host

[Janosch Frank]

On 1/9/23 10:44, Cédric Le Goater wrote:
> On 1/9/23 09:45, Janosch Frank wrote:
>> On 1/6/23 08:53, 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.
>>>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>
>> Any reason why you didn't use KVM_CAP_S390_PROTECTED?
> 
> I think my setup was incorrect when I did the patch. I just verified and QEMU
> indeed reports :
> 
>    qemu-system-s390x: CPU model does not support Protected Virtualization
> 
> which means S390_FEAT_UNPACK was not set.
> 
> 
>> The sysfs interface isn't meant to be parsed by programs, it's been introduced for humans. Most of the interface's data has therefore been made available via the UV info API.
> 
> Well, QEMU is user space and does peek around in sysfs to collect some info.
> Unneeded in that case.

I meant the UV/PV sysfs files, not sysfs in general :)

[PATCH v2 3/4] 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         | 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 d53ef8fd38..13c6116076 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -327,6 +327,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 s390_pv_guest_check(ms->cgs, 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

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

[Janosch Frank]

On 1/6/23 08:53, 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.

Most of the time you see nothing in the console because libvirt is too 
slow. If you start the VM in paused mode, attach a console and then 
resume it, then you'll see a nice error message.

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

If we don't have PV support then the subcodes >=8 are a specification 
exception so this is never executed AFAIK.

>       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:

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

[Cédric Le Goater]

On 1/9/23 10:04, Janosch Frank wrote:
> On 1/6/23 08:53, 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.
> 
> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.

If you wait long enough, the VM fails to mount / and falls into the dracut
initrams.

>> Perform the checks on Confidential Guest support at runtime with an
>> helper called from the service call switching the guest to protected
>> mode.
> 
> If we don't have PV support then the subcodes >=8 are a specification exception 
> so this is never executed AFAIK.

It is. The test on huge page size was added just above this change.

Thanks,

C.

> 
>>       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:
> 

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

[Janosch Frank]

On 1/9/23 10:27, Cédric Le Goater wrote:
> On 1/9/23 10:04, Janosch Frank wrote:
>> On 1/6/23 08:53, 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.
>>
>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
> 
> If you wait long enough, the VM fails to mount / and falls into the dracut
> initrams.

I have the feeling that we're not talking about the same thing here.

A PV VM always starts out as a non-PV VM and is put into PV mode via two 
diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions 
if the host isn't enabled for PV.

There is no way to run a secure image in a non-PV environment. What's 
being run is a non-secure bootloader that initiates the switch into 
secure mode.

Either the switch fails and we return with DIAG_308_RC_INVAL_FOR_PV to 
the non-PV bootloader or we start running in PV mode and __never__ 
return to the bootloader without a reboot.

> 
>>> Perform the checks on Confidential Guest support at runtime with an
>>> helper called from the service call switching the guest to protected
>>> mode.
>>
>> If we don't have PV support then the subcodes >=8 are a specification exception
>> so this is never executed AFAIK.
> 
> It is. The test on huge page size was added just above this change.

And the huge page test only applies if the PV feature is in the cpumodel 
which also means that the host is PV enabled since it's based on the 
capability:

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


> 
> Thanks,
> 
> C.
> 
>>
>>>        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:
>>
> 

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

[Cédric Le Goater]

On 1/9/23 10:49, Janosch Frank wrote:
> On 1/9/23 10:27, Cédric Le Goater wrote:
>> On 1/9/23 10:04, Janosch Frank wrote:
>>> On 1/6/23 08:53, 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.
>>>
>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>
>> If you wait long enough, the VM fails to mount / and falls into the dracut
>> initrams.
> 
> I have the feeling that we're not talking about the same thing here.>
  > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.

The corner case this patch is trying to address is for a PV-enabled host,
a secure enabled OS and !PV-enabled QEMU.

Please run this command on a secure disk image :

   qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio

and tell me what you get.

Thanks,

C.

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

[Janosch Frank]

On 1/9/23 14:30, Cédric Le Goater wrote:
> On 1/9/23 10:49, Janosch Frank wrote:
>> On 1/9/23 10:27, Cédric Le Goater wrote:
>>> On 1/9/23 10:04, Janosch Frank wrote:
>>>> On 1/6/23 08:53, 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.
>>>>
>>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>>
>>> If you wait long enough, the VM fails to mount / and falls into the dracut
>>> initrams.
>>
>> I have the feeling that we're not talking about the same thing here.>
>    > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.
> 
> The corner case this patch is trying to address is for a PV-enabled host,
> a secure enabled OS and !PV-enabled QEMU.
> 
> Please run this command on a secure disk image :
> 
>     qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
> 
> and tell me what you get.
> 

qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive 
file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial 
mon:stdio
LOADPARM=[        ]
Using virtio-blk.
Using SCSI scheme.
.............................................................................................................................
Secure unpack facility is not available


> Thanks,
> 
> C.

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

[Cédric Le Goater]

On 1/9/23 14:45, Janosch Frank wrote:
> On 1/9/23 14:30, Cédric Le Goater wrote:
>> On 1/9/23 10:49, Janosch Frank wrote:
>>> On 1/9/23 10:27, Cédric Le Goater wrote:
>>>> On 1/9/23 10:04, Janosch Frank wrote:
>>>>> On 1/6/23 08:53, 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.
>>>>>
>>>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>>>
>>>> If you wait long enough, the VM fails to mount / and falls into the dracut
>>>> initrams.
>>>
>>> I have the feeling that we're not talking about the same thing here.>
>>    > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.
>>
>> The corner case this patch is trying to address is for a PV-enabled host,
>> a secure enabled OS and !PV-enabled QEMU.
>>
>> Please run this command on a secure disk image :
>>
>>     qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>
>> and tell me what you get.
>>
> 
> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
> LOADPARM=[        ]
> Using virtio-blk.
> Using SCSI scheme.
> .............................................................................................................................
> Secure unpack facility is not available

Yes. That's with a !PV-enabled host. Correct ?

Can you try with prot_virt=1 on the host please ?

Thanks,

C.

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

[Janosch Frank]

On 1/9/23 14:53, Cédric Le Goater wrote:
> On 1/9/23 14:45, Janosch Frank wrote:
>> On 1/9/23 14:30, Cédric Le Goater wrote:
>>> On 1/9/23 10:49, Janosch Frank wrote:
>>>> On 1/9/23 10:27, Cédric Le Goater wrote:
>>>>> On 1/9/23 10:04, Janosch Frank wrote:
>>>>>> On 1/6/23 08:53, 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.
>>>>>>
>>>>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>>>>
>>>>> If you wait long enough, the VM fails to mount / and falls into the dracut
>>>>> initrams.
>>>>
>>>> I have the feeling that we're not talking about the same thing here.>
>>>     > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.
>>>
>>> The corner case this patch is trying to address is for a PV-enabled host,
>>> a secure enabled OS and !PV-enabled QEMU.
>>>
>>> Please run this command on a secure disk image :
>>>
>>>      qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>>
>>> and tell me what you get.
>>>
>>
>> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>> LOADPARM=[        ]
>> Using virtio-blk.
>> Using SCSI scheme.
>> .............................................................................................................................
>> Secure unpack facility is not available
> 
> Yes. That's with a !PV-enabled host. Correct ?
> 
> Can you try with prot_virt=1 on the host please ?

With prot_virt=1 it boots until it doesn't find the file system (at 
least if you give it a bit more memory than the standard 256MB):

qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive 
file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial 
mon:stdio -m 4096
[Linux boot stuff]
ALERT!  /dev/disk/by-path/ccw-0.0.0000-part1 does not exist.  Dropping 
to a shell!

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

[Janosch Frank]

On 1/9/23 15:31, Janosch Frank wrote:
> On 1/9/23 14:53, Cédric Le Goater wrote:
>> On 1/9/23 14:45, Janosch Frank wrote:
>>> On 1/9/23 14:30, Cédric Le Goater wrote:
>>>> On 1/9/23 10:49, Janosch Frank wrote:
>>>>> On 1/9/23 10:27, Cédric Le Goater wrote:
>>>>>> On 1/9/23 10:04, Janosch Frank wrote:
>>>>>>> On 1/6/23 08:53, 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.
>>>>>>>
>>>>>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>>>>>
>>>>>> If you wait long enough, the VM fails to mount / and falls into the dracut
>>>>>> initrams.
>>>>>
>>>>> I have the feeling that we're not talking about the same thing here.>
>>>>      > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.
>>>>
>>>> The corner case this patch is trying to address is for a PV-enabled host,
>>>> a secure enabled OS and !PV-enabled QEMU.
>>>>
>>>> Please run this command on a secure disk image :
>>>>
>>>>       qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>>>
>>>> and tell me what you get.
>>>>
>>>
>>> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>> LOADPARM=[        ]
>>> Using virtio-blk.
>>> Using SCSI scheme.
>>> .............................................................................................................................
>>> Secure unpack facility is not available
>>
>> Yes. That's with a !PV-enabled host. Correct ?
>>
>> Can you try with prot_virt=1 on the host please ?
> 
> With prot_virt=1 it boots until it doesn't find the file system (at
> least if you give it a bit more memory than the standard 256MB):
> 
> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive
> file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial
> mon:stdio -m 4096
> [Linux boot stuff]
> ALERT!  /dev/disk/by-path/ccw-0.0.0000-part1 does not exist.  Dropping
> to a shell!
> 

I.e. it boots into secure mode just fine.

Now if you add iommu_platform=true to the device then it'll even boot 
from disk.

So we'd rather need an error message if you attach a device without the 
iommu being set to true. The whole topic of PV iommu problems has a few 
windings which I don't fully want to bring to electronic paper right now.

Either you start a secure guest that has devices with manual iommu 
entries or you go the launch security route and let libvirt/qemu handle 
it for you.

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

[Cédric Le Goater]

On 1/9/23 15:52, Janosch Frank wrote:
> On 1/9/23 15:31, Janosch Frank wrote:
>> On 1/9/23 14:53, Cédric Le Goater wrote:
>>> On 1/9/23 14:45, Janosch Frank wrote:
>>>> On 1/9/23 14:30, Cédric Le Goater wrote:
>>>>> On 1/9/23 10:49, Janosch Frank wrote:
>>>>>> On 1/9/23 10:27, Cédric Le Goater wrote:
>>>>>>> On 1/9/23 10:04, Janosch Frank wrote:
>>>>>>>> On 1/6/23 08:53, 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.
>>>>>>>>
>>>>>>>> Most of the time you see nothing in the console because libvirt is too slow. If you start the VM in paused mode, attach a console and then resume it, then you'll see a nice error message.
>>>>>>>
>>>>>>> If you wait long enough, the VM fails to mount / and falls into the dracut
>>>>>>> initrams.
>>>>>>
>>>>>> I have the feeling that we're not talking about the same thing here.>
>>>>>      > A PV VM always starts out as a non-PV VM and is put into PV mode via two diag308 subcodes (8 & 10). ALL PV subcodes (8 - 10) are spec exceptions if the host isn't enabled for PV.
>>>>>
>>>>> The corner case this patch is trying to address is for a PV-enabled host,
>>>>> a secure enabled OS and !PV-enabled QEMU.
>>>>>
>>>>> Please run this command on a secure disk image :
>>>>>
>>>>>       qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=<file>,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>>>>
>>>>> and tell me what you get.
>>>>>
>>>>
>>>> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial mon:stdio
>>>> LOADPARM=[        ]
>>>> Using virtio-blk.
>>>> Using SCSI scheme.
>>>> .............................................................................................................................
>>>> Secure unpack facility is not available
>>>
>>> Yes. That's with a !PV-enabled host. Correct ?
>>>
>>> Can you try with prot_virt=1 on the host please ?
>>
>> With prot_virt=1 it boots until it doesn't find the file system (at
>> least if you give it a bit more memory than the standard 256MB):
>>
>> qemu-system-s390x -M s390-ccw-virtio -accel kvm -drive
>> file=u2204.qcow2,if=virtio,format=qcow2 -nographic -nodefaults -serial
>> mon:stdio -m 4096
>> [Linux boot stuff]
>> ALERT!  /dev/disk/by-path/ccw-0.0.0000-part1 does not exist.  Dropping
>> to a shell!
>>
> 
> I.e. it boots into secure mode just fine.
> 
> Now if you add iommu_platform=true to the device then it'll even boot from disk.
> 
> So we'd rather need an error message if you attach a device without the iommu being set to true. The whole topic of PV iommu problems has a few windings which I don't fully want to bring to electronic paper right now.
> 
> Either you start a secure guest that has devices with manual iommu entries or you go the launch security route and let libvirt/qemu handle it for you.

aaahh. So "-machine confidential-guest-support=pv0 -object s390-pv-guest,id=pv0"
is equivalent to "-global virtio-device.iommu_platform=true". I should have
looked closer :/

Thanks for the clarification,

C.

[PATCH v2 4/4] 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.

Reviewed-by: Thomas Huth <thuth@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 13c6116076..b8f53a0247 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -301,9 +301,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(ConfidentialGuestSupport *cgs, 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);
 }
 
 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.38.1