Re: [PATCH v3 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 9:40 AM, 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>
---

[...]

@@ -450,8 +451,17 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
  
  static void s390_machine_reset(MachineState *machine, ResetType type)
  {
+    union Diag308Response {
+        struct {
+            struct S390PvResponse pv_resp;
+            uint16_t diag_rc;

For this to make sense you'd need to know that S390PvResponse is 3 half 
words. I'd much rather have all 4 half words directly visible than 
hiding 3 of them in the struct.

+        };
+        uint64_t regs;
+    };

Why are you trying to assemble the r1 + 1 value outside of 
s390_pv_inject_reset_error()?

You can pass struct S390PvResponse to s390_pv_inject_reset_error() and 
assemble the full register there, no?

+
      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
      enum s390_reset reset_type;
+    union Diag308Response resp;
      CPUState *cs, *t;
      S390CPU *cpu;
  
@@ -539,8 +549,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_resp)) {
+            resp.diag_rc = DIAG_308_RC_INVAL_FOR_PV;
+            s390_pv_inject_reset_error(cs, resp.regs);

You're moving diag 308 related code into s390-virtio-ccw.c which, as 
stated above, I don't really like.
It's messy enough that we're not able to have it in the diag308 handler 
and I don't want to have bit fiddling in s390_machine_reset().

              /*
               * 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..3483603811 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,

[...]

  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..57bdd558dd 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 {

Can we capitalize the "v" please?

+    uint16_t cmd;
+    uint16_t rrc;
+    uint16_t rc;
+};

The rest looks fine to me.