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

[Thomas Huth]

 Hi!

On 14/04/2025 17.48, 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 <gg...@linux.ibm.com>
---
...
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d9e683c5b4..e35997acbd 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -53,6 +53,13 @@
  
  static Error *pv_mig_blocker;
  
+struct Diag308Response {
+    uint16_t pv_cmd;
+    uint16_t pv_rrc;
+    uint16_t pv_rc;
+    uint16_t diag_rc;
+};
+
  static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
                                Error **errp)
  {
@@ -364,7 +371,10 @@ 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,
+                                uint16_t *pv_cmd,
+                                uint16_t *pv_rc,
+                                uint16_t *pv_rrc)

Instead of adding these three pointer parameters to each and every 
functions, how's about only adding a Diag308Response pointer instead?

...
@@ -539,8 +550,9 @@ 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, &resp.pv_cmd, &resp.pv_rc, &resp.pv_rrc)) 
{
+            resp.diag_rc = DIAG_308_RC_INVAL_FOR_PV;
+            s390_pv_inject_reset_error(cs, (uint64_t *)(&resp));

Uh, this smells like it could break strict aliasing rules, please don't do 
type-punning like this. I'd suggest to either wrap Diag308Response in an 
union with an uint64_t as second field, or calculate the value by shifting 
the individual fields and pass that 64-bit result to the function.

 Thanks,
  Thomas


-void s390_pv_inject_reset_error(CPUState *cs)
+void s390_pv_inject_reset_error(CPUState *cs, uint64_t *resp)
  {
      int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
      CPUS390XState *env = &S390_CPU(cs)->env;
  
      /* Report that we are unable to enter protected mode */
-    env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
+    env->regs[r1 + 1] = *resp;
  }