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

2025-04-22 Thread 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 
---


[...]


+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 3/3] target/s390x: Return UVC cmd code, RC and RRC value when DIAG 308 Subcode 10 fails to enter secure mode

2025-04-22 Thread 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 
---


[...]


+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

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

2025-04-17 Thread 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 
---
 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)) {
-