[PATCH v4 0/3] DIAG 308: extend subcode 10 to return UVC cmd id, RC and RRC values upon failure to enter secure mode

[PATCH v4 0/3] DIAG 308: extend subcode 10 to return UVC cmd id, RC and RRC values upon failure to enter secure mode

[Gautam Gala]

DIAG 308 (subcode 10 - performing secure execution unpack) response
code when the configuration is unable to enter secure mode has limited
usability as it is a fixed value (0xa02) for variety of different
reasons. The aim is to extend this DIAG to return UVC command ID, RC
and RRC values in addition to the diag response code. This feature can
be used by the stage3a bootloader (s390-tools/rust/pvimg/boot) to read
these new values from the corresponding register and print an
appropriate error message to help pin point the cause.

The response code, UVC RC, RRC, and command ID are returned in bit
positions 48-63, 32-47, 16-31, and 0-15 of register R1 + 1 if the
function does not complete successfully (Previously, only the
response code was returned in bits 48-63).

This patch version contains updates based on feedback from Janosch Frank
in message-id: <81a4da88-0af0-4fa6-8bfc-e26ae4a7c88f@linux.ibm.com>

Gautam Gala (3):
  target/s390x: Introduce constant when checking if PV header couldn't
    be decrypted
  target/s390x: Introduce function when exiting PV
  target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308
    Subcode 10 fails to enter secure mode

 hw/s390x/ipl.c             | 11 ++++---
 hw/s390x/ipl.h             |  6 ++--
 hw/s390x/s390-virtio-ccw.c | 14 +++++----
 target/s390x/kvm/pv.c      | 64 ++++++++++++++++++++++++--------------
 target/s390x/kvm/pv.h      | 26 +++++++++++-----
 5 files changed, 78 insertions(+), 43 deletions(-)

-- 
2.49.0

[PATCH v4 1/3] target/s390x: Introduce constant when checking if PV header couldn't be decrypted

[Gautam Gala]

Introduce a named constant when checking the Set Secure Configuration parameters
UV call return code for the case where no valid host key was found and therefore
the PV header couldn't be decrypted (0x108).

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
---
 target/s390x/kvm/pv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index b191a4a68a..3a0a971f0b 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -147,6 +147,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
     return true;
 }
 
+#define DIAG_308_UV_RC_INVAL_HOSTKEY    0x0108
 int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
 {
     int ret, pvrc;
@@ -158,7 +159,7 @@ int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
     ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
     if (ret) {
         error_setg(errp, "Failed to set secure execution parameters");
-        if (pvrc == 0x108) {
+        if (pvrc == DIAG_308_UV_RC_INVAL_HOSTKEY) {
             error_append_hint(errp, "Please check whether the image is "
                                     "correctly encrypted for this host\n");
         }
-- 
2.49.0

[PATCH v4 2/3] target/s390x: Introduce function when exiting PV

[Gautam Gala]

Replace an existing macro (s390_pv_cmd_exit) that looks like
a function with an actual function. The function will be used
when exiting PV instead of the macro.

Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
---
 target/s390x/kvm/pv.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 3a0a971f0b..66194caaae 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -59,14 +59,12 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
  */
 #define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
 #define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, pvrc)
-#define s390_pv_cmd_exit(cmd, data)    \
-{                                      \
-    int rc;                            \
-                                       \
-    rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
-    if (rc) {                          \
-        exit(1);                       \
-    }                                  \
+
+static void s390_pv_cmd_exit(uint32_t cmd, void *data)
+{
+    if (s390_pv_cmd(cmd, data)) {
+        exit(1);
+    }
 }
 
 int s390_pv_query_info(void)
-- 
2.49.0

[PATCH v4 3/3] target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308 Subcode 10 fails to enter secure mode

[Gautam Gala]

Extend DIAG308 subcode 10 to return the UVC RC, RRC and command code
in bit positions 32-47, 16-31, and 0-15 of register R1 + 1 if the
function does not complete successfully (in addition to the
previously returned diag response code in bit position 47-63).

Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
---
 hw/s390x/ipl.c             | 11 +++++----
 hw/s390x/ipl.h             |  6 +++--
 hw/s390x/s390-virtio-ccw.c | 14 ++++++-----
 target/s390x/kvm/pv.c      | 49 ++++++++++++++++++++++++++------------
 target/s390x/kvm/pv.h      | 26 ++++++++++++++------
 5 files changed, 71 insertions(+), 35 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index ce6f6078d7..7829a39483 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -26,7 +26,6 @@
 #include "hw/s390x/vfio-ccw.h"
 #include "hw/s390x/css.h"
 #include "hw/s390x/ebcdic.h"
-#include "target/s390x/kvm/pv.h"
 #include "hw/scsi/scsi.h"
 #include "hw/virtio/virtio-net.h"
 #include "ipl.h"
@@ -676,7 +675,7 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
     cpu_physical_memory_unmap(addr, len, 1, len);
 }
 
-int s390_ipl_prepare_pv_header(Error **errp)
+int s390_ipl_prepare_pv_header(struct S390PVResponse *pv_resp, Error **errp)
 {
     IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
     IPLBlockPV *ipib_pv = &ipib->pv;
@@ -685,12 +684,13 @@ int s390_ipl_prepare_pv_header(Error **errp)
 
     cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
                              ipib_pv->pv_header_len);
-    rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len, errp);
+    rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len,
+                               pv_resp, errp);
     g_free(hdr);
     return rc;
 }
 
-int s390_ipl_pv_unpack(void)
+int s390_ipl_pv_unpack(struct S390PVResponse *pv_resp)
 {
     IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
     IPLBlockPV *ipib_pv = &ipib->pv;
@@ -699,7 +699,8 @@ int s390_ipl_pv_unpack(void)
     for (i = 0; i < ipib_pv->num_comp; i++) {
         rc = s390_pv_unpack(ipib_pv->components[i].addr,
                             TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
-                            ipib_pv->components[i].tweak_pref);
+                            ipib_pv->components[i].tweak_pref,
+                            pv_resp);
         if (rc) {
             break;
         }
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 8e3882d506..e108aca369 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -18,6 +18,7 @@
 #include "hw/qdev-core.h"
 #include "hw/s390x/ipl/qipl.h"
 #include "qom/object.h"
+#include "target/s390x/kvm/pv.h"
 
 #define DIAG308_FLAGS_LP_VALID 0x80
 #define MAX_BOOT_DEVS 8 /* Max number of devices that may have a bootindex */
@@ -26,8 +27,9 @@ void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp);
 void s390_ipl_fmt_loadparm(uint8_t *loadparm, char *str, Error **errp);
 void s390_rebuild_iplb(uint16_t index, IplParameterBlock *iplb);
 void s390_ipl_update_diag308(IplParameterBlock *iplb);
-int s390_ipl_prepare_pv_header(Error **errp);
-int s390_ipl_pv_unpack(void);
+int s390_ipl_prepare_pv_header(struct S390PVResponse *pv_resp,
+                               Error **errp);
+int s390_ipl_pv_unpack(struct S390PVResponse *pv_resp);
 void s390_ipl_prepare_cpu(S390CPU *cpu);
 IplParameterBlock *s390_ipl_get_iplb(void);
 IplParameterBlock *s390_ipl_get_iplb_pv(void);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d9e683c5b4..942ebde94a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -364,7 +364,8 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
     ram_block_discard_disable(false);
 }
 
-static int s390_machine_protect(S390CcwMachineState *ms)
+static int s390_machine_protect(S390CcwMachineState *ms,
+                                struct S390PVResponse *pv_resp)
 {
     Error *local_err = NULL;
     int rc;
@@ -407,19 +408,19 @@ static int s390_machine_protect(S390CcwMachineState *ms)
     }
 
     /* Set SE header and unpack */
-    rc = s390_ipl_prepare_pv_header(&local_err);
+    rc = s390_ipl_prepare_pv_header(pv_resp, &local_err);
     if (rc) {
         goto out_err;
     }
 
     /* Decrypt image */
-    rc = s390_ipl_pv_unpack();
+    rc = s390_ipl_pv_unpack(pv_resp);
     if (rc) {
         goto out_err;
     }
 
     /* Verify integrity */
-    rc = s390_pv_verify();
+    rc = s390_pv_verify(pv_resp);
     if (rc) {
         goto out_err;
     }
@@ -451,6 +452,7 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
 static void s390_machine_reset(MachineState *machine, ResetType type)
 {
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+    struct S390PVResponse pv_resp;
     enum s390_reset reset_type;
     CPUState *cs, *t;
     S390CPU *cpu;
@@ -539,8 +541,8 @@ static void s390_machine_reset(MachineState *machine, ResetType type)
         }
         run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
 
-        if (s390_machine_protect(ms)) {
-            s390_pv_inject_reset_error(cs);
+        if (s390_machine_protect(ms, &pv_resp)) {
+            s390_pv_inject_reset_error(cs, pv_resp);
             /*
              * Continue after the diag308 so the guest knows something
              * went wrong.
diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
index 66194caaae..ccecfe28a3 100644
--- a/target/s390x/kvm/pv.c
+++ b/target/s390x/kvm/pv.c
@@ -30,7 +30,7 @@ static struct kvm_s390_pv_info_vm info_vm;
 static struct kvm_s390_pv_info_dump info_dump;
 
 static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
-                         int *pvrc)
+                         struct S390PVResponse *pv_resp)
 {
     struct kvm_pv_cmd pv_cmd = {
         .cmd = cmd,
@@ -47,8 +47,10 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
                      "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
                      rc);
     }
-    if (pvrc) {
-        *pvrc = pv_cmd.rc;
+    if (pv_resp) {
+        pv_resp->cmd = cmd;
+        pv_resp->rc = pv_cmd.rc;
+        pv_resp->rrc = pv_cmd.rrc;
     }
     return rc;
 }
@@ -57,8 +59,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
  * This macro lets us pass the command as a string to the function so
  * we can print it on an error.
  */
-#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
-#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, pvrc)
+#define s390_pv_cmd(cmd, data)  __s390_pv_cmd(cmd, #cmd, data, NULL)
+#define s390_pv_cmd_pv_resp(cmd, data, pv_resp) \
+                                __s390_pv_cmd(cmd, #cmd, data, pv_resp)
 
 static void s390_pv_cmd_exit(uint32_t cmd, void *data)
 {
@@ -146,18 +149,19 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
 }
 
 #define DIAG_308_UV_RC_INVAL_HOSTKEY    0x0108
-int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length,
+                          struct S390PVResponse *pv_resp, Error **errp)
 {
-    int ret, pvrc;
+    int ret;
     struct kvm_s390_pv_sec_parm args = {
         .origin = origin,
         .length = length,
     };
 
-    ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
+    ret = s390_pv_cmd_pv_resp(KVM_PV_SET_SEC_PARMS, &args, pv_resp);
     if (ret) {
         error_setg(errp, "Failed to set secure execution parameters");
-        if (pvrc == DIAG_308_UV_RC_INVAL_HOSTKEY) {
+        if (pv_resp->rc == DIAG_308_UV_RC_INVAL_HOSTKEY) {
             error_append_hint(errp, "Please check whether the image is "
                                     "correctly encrypted for this host\n");
         }
@@ -169,7 +173,8 @@ int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
 /*
  * Called for each component in the SE type IPL parameter block 0.
  */
-int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
+int s390_pv_unpack(uint64_t addr, uint64_t size,
+                   uint64_t tweak, struct S390PVResponse *pv_resp)
 {
     struct kvm_s390_pv_unp args = {
         .addr = addr,
@@ -177,7 +182,7 @@ int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
         .tweak = tweak,
     };
 
-    return s390_pv_cmd(KVM_PV_UNPACK, &args);
+    return s390_pv_cmd_pv_resp(KVM_PV_UNPACK, &args, pv_resp);
 }
 
 void s390_pv_prep_reset(void)
@@ -185,9 +190,9 @@ void s390_pv_prep_reset(void)
     s390_pv_cmd_exit(KVM_PV_PREP_RESET, NULL);
 }
 
-int s390_pv_verify(void)
+int s390_pv_verify(struct S390PVResponse *pv_resp)
 {
-    return s390_pv_cmd(KVM_PV_VERIFY, NULL);
+    return s390_pv_cmd_pv_resp(KVM_PV_VERIFY, NULL, pv_resp);
 }
 
 void s390_pv_unshare(void)
@@ -195,13 +200,27 @@ void s390_pv_unshare(void)
     s390_pv_cmd_exit(KVM_PV_UNSHARE_ALL, NULL);
 }
 
-void s390_pv_inject_reset_error(CPUState *cs)
+void s390_pv_inject_reset_error(CPUState *cs,
+                                struct S390PVResponse pv_resp)
 {
     int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
     CPUS390XState *env = &S390_CPU(cs)->env;
 
+    union {
+        struct {
+            uint16_t pv_cmd;
+            uint16_t pv_rrc;
+            uint16_t pv_rc;
+            uint16_t diag_rc;
+        };
+        uint64_t regs;
+    } resp = {.pv_cmd = pv_resp.cmd,
+              .pv_rrc = pv_resp.rrc,
+              .pv_rc = pv_resp.rc,
+              .diag_rc = DIAG_308_RC_INVAL_FOR_PV};
+
     /* Report that we are unable to enter protected mode */
-    env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
+    env->regs[r1 + 1] = resp.regs;
 }
 
 uint64_t kvm_s390_pv_dmp_get_size_cpu(void)
diff --git a/target/s390x/kvm/pv.h b/target/s390x/kvm/pv.h
index 5e9c8bd351..94e885e933 100644
--- a/target/s390x/kvm/pv.h
+++ b/target/s390x/kvm/pv.h
@@ -16,6 +16,12 @@
 #include "system/kvm.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 
+struct S390PVResponse {
+    uint16_t cmd;
+    uint16_t rrc;
+    uint16_t rc;
+};
+
 #ifdef CONFIG_KVM
 #include "cpu.h"
 
@@ -42,12 +48,15 @@ int s390_pv_query_info(void);
 int s390_pv_vm_enable(void);
 void s390_pv_vm_disable(void);
 bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms);
-int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp);
-int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length,
+                          struct S390PVResponse *pv_resp, Error **errp);
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak,
+                   struct S390PVResponse *pv_resp);
 void s390_pv_prep_reset(void);
-int s390_pv_verify(void);
+int s390_pv_verify(struct S390PVResponse *pv_resp);
 void s390_pv_unshare(void);
-void s390_pv_inject_reset_error(CPUState *cs);
+void s390_pv_inject_reset_error(CPUState *cs,
+                                struct S390PVResponse pv_resp);
 uint64_t kvm_s390_pv_dmp_get_size_cpu(void);
 uint64_t kvm_s390_pv_dmp_get_size_mem_state(void);
 uint64_t kvm_s390_pv_dmp_get_size_completion_data(void);
@@ -63,12 +72,15 @@ static inline int s390_pv_vm_enable(void) { return 0; }
 static inline void s390_pv_vm_disable(void) {}
 static inline bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms) { return false; }
 static inline int s390_pv_set_sec_parms(uint64_t origin, uint64_t length,
+                                        struct S390PVResponse *pv_resp,
                                         Error **errp) { return 0; }
-static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
+static inline int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak,
+                                 struct S390PVResponse *pv_resp) { return 0; }
 static inline void s390_pv_prep_reset(void) {}
-static inline int s390_pv_verify(void) { return 0; }
+static inline int s390_pv_verify(struct S390PVResponse *pv_resp) { return 0; }
 static inline void s390_pv_unshare(void) {}
-static inline void s390_pv_inject_reset_error(CPUState *cs) {};
+static inline void s390_pv_inject_reset_error(CPUState *cs,
+                                              struct S390PVResponse pv_resp) {};
 static inline uint64_t kvm_s390_pv_dmp_get_size_cpu(void) { return 0; }
 static inline uint64_t kvm_s390_pv_dmp_get_size_mem_state(void) { return 0; }
 static inline uint64_t kvm_s390_pv_dmp_get_size_completion_data(void) { return 0; }
-- 
2.49.0

Re: [PATCH v4 1/3] target/s390x: Introduce constant when checking if PV header couldn't be decrypted

[Janosch Frank]

On 4/17/25 2:37 PM, Gautam Gala wrote:
> Introduce a named constant when checking the Set Secure Configuration parameters
> UV call return code for the case where no valid host key was found and therefore
> the PV header couldn't be decrypted (0x108).
> 
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
> ---
>   target/s390x/kvm/pv.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index b191a4a68a..3a0a971f0b 100644
> --- a/target/s390x/kvm/pv.c
> +++ b/target/s390x/kvm/pv.c
> @@ -147,6 +147,7 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>       return true;
>   }
>   
> +#define DIAG_308_UV_RC_INVAL_HOSTKEY    0x0108

s/DIAG_308_UV_RC_INVAL_HOSTKEY/UV_RC_SSC_INVAL_HOSTKEY/

The naming of the constant should reflect to which UV command this rc 
belongs to.

Once changed:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

>   int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
>   {
>       int ret, pvrc;
> @@ -158,7 +159,7 @@ int s390_pv_set_sec_parms(uint64_t origin, uint64_t length, Error **errp)
>       ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
>       if (ret) {
>           error_setg(errp, "Failed to set secure execution parameters");
> -        if (pvrc == 0x108) {
> +        if (pvrc == DIAG_308_UV_RC_INVAL_HOSTKEY) {
>               error_append_hint(errp, "Please check whether the image is "
>                                       "correctly encrypted for this host\n");
>           }

Re: [PATCH v4 2/3] target/s390x: Introduce function when exiting PV

[Janosch Frank]

On 4/17/25 2:37 PM, Gautam Gala wrote:
> Replace an existing macro (s390_pv_cmd_exit) that looks like
> a function with an actual function. The function will be used
> when exiting PV instead of the macro.
> 
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v4 3/3] target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308 Subcode 10 fails to enter secure mode

[Janosch Frank]

On 4/17/25 2:37 PM, Gautam Gala wrote:
> Extend DIAG308 subcode 10 to return the UVC RC, RRC and command code
> in bit positions 32-47, 16-31, and 0-15 of register R1 + 1 if the
> function does not complete successfully (in addition to the
> previously returned diag response code in bit position 47-63).
> 
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
> ---

[...]

> +void s390_pv_inject_reset_error(CPUState *cs,
> +                                struct S390PVResponse pv_resp)
>   {
>       int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
>       CPUS390XState *env = &S390_CPU(cs)->env;
>   
> +    union {
> +        struct {
> +            uint16_t pv_cmd;
> +            uint16_t pv_rrc;
> +            uint16_t pv_rc;
> +            uint16_t diag_rc;
> +        };
> +        uint64_t regs;
> +    } resp = {.pv_cmd = pv_resp.cmd,
> +              .pv_rrc = pv_resp.rrc,
> +              .pv_rc = pv_resp.rc,
> +              .diag_rc = DIAG_308_RC_INVAL_FOR_PV};
> +

@Thomas: Is the formatting of the assignments correct or should there be 
no assignment on lines containing "{}"?

Checkpatch is happy, though personally I find it hard to read.

Once that's clear or fixed:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v4 3/3] target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308 Subcode 10 fails to enter secure mode

[Thomas Huth]

On 22/04/2025 15.17, Janosch Frank wrote:
> On 4/17/25 2:37 PM, Gautam Gala wrote:
>> Extend DIAG308 subcode 10 to return the UVC RC, RRC and command code
>> in bit positions 32-47, 16-31, and 0-15 of register R1 + 1 if the
>> function does not complete successfully (in addition to the
>> previously returned diag response code in bit position 47-63).
>>
>> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
>> ---
> 
> [...]
> 
>> +void s390_pv_inject_reset_error(CPUState *cs,
>> +                                struct S390PVResponse pv_resp)
>>   {
>>       int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
>>       CPUS390XState *env = &S390_CPU(cs)->env;
>> +    union {
>> +        struct {
>> +            uint16_t pv_cmd;
>> +            uint16_t pv_rrc;
>> +            uint16_t pv_rc;
>> +            uint16_t diag_rc;
>> +        };
>> +        uint64_t regs;
>> +    } resp = {.pv_cmd = pv_resp.cmd,
>> +              .pv_rrc = pv_resp.rrc,
>> +              .pv_rc = pv_resp.rc,
>> +              .diag_rc = DIAG_308_RC_INVAL_FOR_PV};
>> +
> 
> @Thomas: Is the formatting of the assignments correct or should there be no 
> assignment on lines containing "{}"?
> 
> Checkpatch is happy, though personally I find it hard to read.

I think I'd rather have the ".pv_cmd = ..." start on the next line.

  Thanks
   Thomas

Re: [PATCH v4 2/3] target/s390x: Introduce function when exiting PV

[Thomas Huth]

On 17/04/2025 14.37, Gautam Gala wrote:
> Replace an existing macro (s390_pv_cmd_exit) that looks like
> a function with an actual function. The function will be used
> when exiting PV instead of the macro.
> 
> Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> Signed-off-by: Gautam Gala <ggala@linux.ibm.com>
> ---
>   target/s390x/kvm/pv.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)

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