[PATCH 0/9] target/s390x: Remove legacy cpu_physical_memory_*() calls

[PATCH 0/9] target/s390x: Remove legacy cpu_physical_memory_*() calls

[Philippe Mathieu-Daudé]

The cpu_physical_memory API is legacy (see commit b7ecba0f6f6):

  ``cpu_physical_memory_*``
  ~~~~~~~~~~~~~~~~~~~~~~~~~

  These are convenience functions which are identical to
  ``address_space_*`` but operate specifically on the system address space,
  always pass a ``MEMTXATTRS_UNSPECIFIED`` set of memory attributes and
  ignore whether the memory transaction succeeded or failed.
  For new code they are better avoided:
  ...

This series convert the following methods in target/s390x/:
  - cpu_physical_memory_map()
  - cpu_physical_memory_unmap()
  - cpu_physical_memory_write()
to the address_space_*() equivalent.

Based-on: <20251001175448.18933-1-philmd@linaro.org>

Philippe Mathieu-Daudé (9):
  target/s390x: Factor diag_iplb_write() out
  target/s390x: Factor diag_iplb_read() out
  target/s390x: Replace legacy cpu_physical_memory_read/write() calls
  target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (1/3)
  target/s390x: Propagate CPUS390XState to cpu_unmap_lowcore()
  target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (2/3)
  target/s390x: Reduce s390_store_adtl_status() scope
  target/s390x: Reduce s390_store_status() scope
  target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (3/3)

 target/s390x/s390x-internal.h  |   5 +-
 target/s390x/diag.c            |  55 ++++++++++-----
 target/s390x/helper.c          | 122 +++------------------------------
 target/s390x/sigp.c            | 117 +++++++++++++++++++++++++++++++
 target/s390x/tcg/excp_helper.c |  19 ++---
 target/s390x/tcg/misc_helper.c |   4 +-
 6 files changed, 179 insertions(+), 143 deletions(-)

-- 
2.51.0

[PATCH 1/9] target/s390x: Factor diag_iplb_write() out

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/diag.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index da44b0133ed..ed320fc0c1f 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -73,6 +73,17 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
     return 0;
 }
 
+static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
+{
+    const size_t iplb_len = be32_to_cpu(iplb->len);
+
+    if (s390_is_pv()) {
+        s390_cpu_pv_mem_write(cpu, 0, iplb, iplb_len);
+    } else {
+        cpu_physical_memory_write(addr, iplb, iplb_len);
+    }
+}
+
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
     bool valid;
@@ -164,11 +175,7 @@ out:
             return;
         }
 
-        if (!s390_is_pv()) {
-            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
-        } else {
-            s390_cpu_pv_mem_write(cpu, 0, iplb, be32_to_cpu(iplb->len));
-        }
+        diag_iplb_write(iplb, cpu, addr);
         env->regs[r1 + 1] = DIAG_308_RC_OK;
         return;
     case DIAG308_PV_START:
-- 
2.51.0

[PATCH 2/9] target/s390x: Factor diag_iplb_read() out

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/diag.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index ed320fc0c1f..c2fedc55213 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -73,6 +73,24 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
     return 0;
 }
 
+static bool diag_iplb_read(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
+{
+    if (s390_is_pv()) {
+        s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
+        if (!iplb_valid_len(iplb)) {
+            return false;
+        }
+        s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
+    } else {
+        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+        if (!iplb_valid_len(iplb)) {
+            return false;
+        }
+        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+    }
+    return true;
+}
+
 static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
 {
     const size_t iplb_len = be32_to_cpu(iplb->len);
@@ -125,23 +143,12 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
             return;
         }
         iplb = g_new0(IplParameterBlock, 1);
-        if (!s390_is_pv()) {
-            cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
-        } else {
-            s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
-        }
 
-        if (!iplb_valid_len(iplb)) {
+        if (!diag_iplb_read(iplb, cpu, addr)) {
             env->regs[r1 + 1] = DIAG_308_RC_INVALID;
             goto out;
         }
 
-        if (!s390_is_pv()) {
-            cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
-        } else {
-            s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
-        }
-
         valid = subcode == DIAG308_PV_SET ? iplb_valid_pv(iplb) : iplb_valid(iplb);
         if (!valid) {
             if (subcode == DIAG308_SET && iplb->pbt == S390_IPL_TYPE_QEMU_SCSI) {
-- 
2.51.0

[PATCH 3/9] target/s390x: Replace legacy cpu_physical_memory_read/write() calls

[Philippe Mathieu-Daudé]

cpu_physical_memory_read() and cpu_physical_memory_write() are
legacy (see commit b7ecba0f6f6), replace by address_space_read()
and address_space_write() respectively.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/diag.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index c2fedc55213..737c3bbc5be 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -17,6 +17,7 @@
 #include "s390x-internal.h"
 #include "hw/watchdog/wdt_diag288.h"
 #include "system/cpus.h"
+#include "system/memory.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "system/kvm.h"
@@ -82,11 +83,14 @@ static bool diag_iplb_read(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
         }
         s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
     } else {
-        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+        AddressSpace *as = CPU(cpu)->as;
+
+        address_space_read(as, addr, attrs, iplb, sizeof(iplb->len));
         if (!iplb_valid_len(iplb)) {
             return false;
         }
-        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+        address_space_read(as, addr, attrs, iplb, be32_to_cpu(iplb->len));
     }
     return true;
 }
@@ -98,7 +102,10 @@ static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr
     if (s390_is_pv()) {
         s390_cpu_pv_mem_write(cpu, 0, iplb, iplb_len);
     } else {
-        cpu_physical_memory_write(addr, iplb, iplb_len);
+        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+        AddressSpace *as = CPU(cpu)->as;
+
+        address_space_write(as, addr, attrs, iplb, iplb_len);
     }
 }
 
-- 
2.51.0

[PATCH 4/9] target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (1/3)

[Philippe Mathieu-Daudé]

Commit b7ecba0f6f6 ("docs/devel/loads-stores.rst: Document our
various load and store APIs") mentioned cpu_physical_memory_*()
methods are legacy, the replacement being address_space_*().

Replace the *_map() / *_unmap() methods in mchk_store_vregs().
No behavioral change expected.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/tcg/excp_helper.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index 4c7faeee82b..5e1d4dc9583 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -30,6 +30,7 @@
 #ifndef CONFIG_USER_ONLY
 #include "qemu/timer.h"
 #include "system/address-spaces.h"
+#include "system/memory.h"
 #include "hw/s390x/ioinst.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/boards.h"
@@ -418,16 +419,18 @@ QEMU_BUILD_BUG_ON(sizeof(MchkExtSaveArea) != 1024);
 
 static int mchk_store_vregs(CPUS390XState *env, uint64_t mcesao)
 {
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+    AddressSpace *as = env_cpu(env)->as;
     hwaddr len = sizeof(MchkExtSaveArea);
     MchkExtSaveArea *sa;
     int i;
 
-    sa = cpu_physical_memory_map(mcesao, &len, true);
+    sa = address_space_map(as, mcesao, &len, true, attrs);
     if (!sa) {
         return -EFAULT;
     }
     if (len != sizeof(MchkExtSaveArea)) {
-        cpu_physical_memory_unmap(sa, len, 1, 0);
+        address_space_unmap(as, sa, len, true, 0);
         return -EFAULT;
     }
 
@@ -436,7 +439,7 @@ static int mchk_store_vregs(CPUS390XState *env, uint64_t mcesao)
         sa->vregs[i][1] = cpu_to_be64(env->vregs[i][1]);
     }
 
-    cpu_physical_memory_unmap(sa, len, 1, len);
+    address_space_unmap(as, sa, len, true, len);
     return 0;
 }
 
-- 
2.51.0

[PATCH 5/9] target/s390x: Propagate CPUS390XState to cpu_unmap_lowcore()

[Philippe Mathieu-Daudé]

To be able to access the CPU state in cpu_unmap_lowcore()
in the next commit, propagate it as argument.
cpu_map_lowcore() already takes it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h  |  2 +-
 target/s390x/helper.c          |  4 ++--
 target/s390x/tcg/excp_helper.c | 10 +++++-----
 target/s390x/tcg/misc_helper.c |  4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 56cce2e7f50..1fb752aa1d6 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -327,7 +327,7 @@ hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
 int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch);
 int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len);
 LowCore *cpu_map_lowcore(CPUS390XState *env);
-void cpu_unmap_lowcore(LowCore *lowcore);
+void cpu_unmap_lowcore(CPUS390XState *env, LowCore *lowcore);
 #endif /* CONFIG_USER_ONLY */
 
 
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 5c127da1a6a..a6d4a1ce05d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -117,7 +117,7 @@ LowCore *cpu_map_lowcore(CPUS390XState *env)
     return lowcore;
 }
 
-void cpu_unmap_lowcore(LowCore *lowcore)
+void cpu_unmap_lowcore(CPUS390XState *env, LowCore *lowcore)
 {
     cpu_physical_memory_unmap(lowcore, sizeof(LowCore), 1, sizeof(LowCore));
 }
@@ -134,7 +134,7 @@ void do_restart_interrupt(CPUS390XState *env)
     mask = be64_to_cpu(lowcore->restart_new_psw.mask);
     addr = be64_to_cpu(lowcore->restart_new_psw.addr);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
     env->pending_int &= ~INTERRUPT_RESTART;
 
     s390_cpu_set_psw(env, mask, addr);
diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c
index 5e1d4dc9583..0ae4e266065 100644
--- a/target/s390x/tcg/excp_helper.c
+++ b/target/s390x/tcg/excp_helper.c
@@ -285,7 +285,7 @@ static void do_program_interrupt(CPUS390XState *env)
     addr = be64_to_cpu(lowcore->program_new_psw.addr);
     lowcore->per_breaking_event_addr = cpu_to_be64(env->gbea);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
 
     s390_cpu_set_psw(env, mask, addr);
 }
@@ -304,7 +304,7 @@ static void do_svc_interrupt(CPUS390XState *env)
     mask = be64_to_cpu(lowcore->svc_new_psw.mask);
     addr = be64_to_cpu(lowcore->svc_new_psw.addr);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
 
     s390_cpu_set_psw(env, mask, addr);
 
@@ -378,7 +378,7 @@ static void do_ext_interrupt(CPUS390XState *env)
     lowcore->external_old_psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(env));
     lowcore->external_old_psw.addr = cpu_to_be64(env->psw.addr);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
 
     s390_cpu_set_psw(env, mask, addr);
 }
@@ -405,7 +405,7 @@ static void do_io_interrupt(CPUS390XState *env)
     mask = be64_to_cpu(lowcore->io_new_psw.mask);
     addr = be64_to_cpu(lowcore->io_new_psw.addr);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
     g_free(io);
 
     s390_cpu_set_psw(env, mask, addr);
@@ -491,7 +491,7 @@ static void do_mchk_interrupt(CPUS390XState *env)
     mask = be64_to_cpu(lowcore->mcck_new_psw.mask);
     addr = be64_to_cpu(lowcore->mcck_new_psw.addr);
 
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
 
     s390_cpu_set_psw(env, mask, addr);
 }
diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
index f7101be5745..6d9d601d29a 100644
--- a/target/s390x/tcg/misc_helper.c
+++ b/target/s390x/tcg/misc_helper.c
@@ -570,7 +570,7 @@ uint32_t HELPER(tpi)(CPUS390XState *env, uint64_t addr)
         lowcore->subchannel_nr = cpu_to_be16(io->nr);
         lowcore->io_int_parm = cpu_to_be32(io->parm);
         lowcore->io_int_word = cpu_to_be32(io->word);
-        cpu_unmap_lowcore(lowcore);
+        cpu_unmap_lowcore(env, lowcore);
     }
 
     g_free(io);
@@ -700,7 +700,7 @@ void HELPER(stfl)(CPUS390XState *env)
     lowcore = cpu_map_lowcore(env);
     prepare_stfl();
     memcpy(&lowcore->stfl_fac_list, stfl_bytes, sizeof(lowcore->stfl_fac_list));
-    cpu_unmap_lowcore(lowcore);
+    cpu_unmap_lowcore(env, lowcore);
 }
 #endif
 
-- 
2.51.0

[PATCH 6/9] target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (2/3)

[Philippe Mathieu-Daudé]

Commit b7ecba0f6f6 ("docs/devel/loads-stores.rst: Document our
various load and store APIs") mentioned cpu_physical_memory_*()
methods are legacy, the replacement being address_space_*().

Replace the *_map() / *_unmap() methods in cpu_[un]map_lowcore().
No behavioral change expected.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/helper.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index a6d4a1ce05d..b01b7d9bbbf 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -26,6 +26,7 @@
 #include "hw/s390x/ioinst.h"
 #include "target/s390x/kvm/pv.h"
 #include "system/hw_accel.h"
+#include "system/memory.h"
 #include "system/runstate.h"
 #include "exec/target_page.h"
 #include "exec/watchpoint.h"
@@ -107,11 +108,13 @@ LowCore *cpu_map_lowcore(CPUS390XState *env)
 {
     LowCore *lowcore;
     hwaddr len = sizeof(LowCore);
+    CPUState *cs = env_cpu(env);
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
 
-    lowcore = cpu_physical_memory_map(env->psa, &len, true);
+    lowcore = address_space_map(cs->as, env->psa, &len, true, attrs);
 
     if (len < sizeof(LowCore)) {
-        cpu_abort(env_cpu(env), "Could not map lowcore\n");
+        cpu_abort(cs, "Could not map lowcore\n");
     }
 
     return lowcore;
@@ -119,7 +122,9 @@ LowCore *cpu_map_lowcore(CPUS390XState *env)
 
 void cpu_unmap_lowcore(CPUS390XState *env, LowCore *lowcore)
 {
-    cpu_physical_memory_unmap(lowcore, sizeof(LowCore), 1, sizeof(LowCore));
+    AddressSpace *as = env_cpu(env)->as;
+
+    address_space_unmap(as, lowcore, sizeof(LowCore), true, sizeof(LowCore));
 }
 
 void do_restart_interrupt(CPUS390XState *env)
-- 
2.51.0

[PATCH 7/9] target/s390x: Reduce s390_store_adtl_status() scope

[Philippe Mathieu-Daudé]

s390_store_adtl_status() is only called within sigp.c,
move it and the SigpAdtlSaveArea structure definition
there where it belongs, with other SIGP handling code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h |  1 -
 target/s390x/helper.c         | 40 -----------------------------------
 target/s390x/sigp.c           | 40 +++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 1fb752aa1d6..a49dca94a75 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -325,7 +325,6 @@ hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
 #define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
 int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch);
-int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len);
 LowCore *cpu_map_lowcore(CPUS390XState *env);
 void cpu_unmap_lowcore(CPUS390XState *env, LowCore *lowcore);
 #endif /* CONFIG_USER_ONLY */
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index b01b7d9bbbf..84321e1d68e 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -248,43 +248,3 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
 
     return 0;
 }
-
-typedef struct SigpAdtlSaveArea {
-    uint64_t    vregs[32][2];                     /* 0x0000 */
-    uint8_t     pad_0x0200[0x0400 - 0x0200];      /* 0x0200 */
-    uint64_t    gscb[4];                          /* 0x0400 */
-    uint8_t     pad_0x0420[0x1000 - 0x0420];      /* 0x0420 */
-} SigpAdtlSaveArea;
-QEMU_BUILD_BUG_ON(sizeof(SigpAdtlSaveArea) != 4096);
-
-#define ADTL_GS_MIN_SIZE 2048 /* minimal size of adtl save area for GS */
-int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
-{
-    SigpAdtlSaveArea *sa;
-    hwaddr save = len;
-    int i;
-
-    sa = cpu_physical_memory_map(addr, &save, true);
-    if (!sa) {
-        return -EFAULT;
-    }
-    if (save != len) {
-        cpu_physical_memory_unmap(sa, len, 1, 0);
-        return -EFAULT;
-    }
-
-    if (s390_has_feat(S390_FEAT_VECTOR)) {
-        for (i = 0; i < 32; i++) {
-            sa->vregs[i][0] = cpu_to_be64(cpu->env.vregs[i][0]);
-            sa->vregs[i][1] = cpu_to_be64(cpu->env.vregs[i][1]);
-        }
-    }
-    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE) && len >= ADTL_GS_MIN_SIZE) {
-        for (i = 0; i < 4; i++) {
-            sa->gscb[i] = cpu_to_be64(cpu->env.gscb[i]);
-        }
-    }
-
-    cpu_physical_memory_unmap(sa, len, 1, len);
-    return 0;
-}
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 5e95c4978f9..1464be76983 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -172,6 +172,46 @@ static void sigp_store_status_at_address(CPUState *cs, run_on_cpu_data arg)
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 }
 
+typedef struct SigpAdtlSaveArea {
+    uint64_t    vregs[32][2];                     /* 0x0000 */
+    uint8_t     pad_0x0200[0x0400 - 0x0200];      /* 0x0200 */
+    uint64_t    gscb[4];                          /* 0x0400 */
+    uint8_t     pad_0x0420[0x1000 - 0x0420];      /* 0x0420 */
+} SigpAdtlSaveArea;
+QEMU_BUILD_BUG_ON(sizeof(SigpAdtlSaveArea) != 4096);
+
+#define ADTL_GS_MIN_SIZE 2048 /* minimal size of adtl save area for GS */
+static int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
+{
+    SigpAdtlSaveArea *sa;
+    hwaddr save = len;
+    int i;
+
+    sa = cpu_physical_memory_map(addr, &save, true);
+    if (!sa) {
+        return -EFAULT;
+    }
+    if (save != len) {
+        cpu_physical_memory_unmap(sa, len, 1, 0);
+        return -EFAULT;
+    }
+
+    if (s390_has_feat(S390_FEAT_VECTOR)) {
+        for (i = 0; i < 32; i++) {
+            sa->vregs[i][0] = cpu_to_be64(cpu->env.vregs[i][0]);
+            sa->vregs[i][1] = cpu_to_be64(cpu->env.vregs[i][1]);
+        }
+    }
+    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE) && len >= ADTL_GS_MIN_SIZE) {
+        for (i = 0; i < 4; i++) {
+            sa->gscb[i] = cpu_to_be64(cpu->env.gscb[i]);
+        }
+    }
+
+    cpu_physical_memory_unmap(sa, len, 1, len);
+    return 0;
+}
+
 #define ADTL_SAVE_LC_MASK  0xfUL
 static void sigp_store_adtl_status(CPUState *cs, run_on_cpu_data arg)
 {
-- 
2.51.0

[PATCH 8/9] target/s390x: Reduce s390_store_status() scope

[Philippe Mathieu-Daudé]

s390_store_status() is only called within sigp.c,
move it and the SigpSaveArea structure definition
there where it belongs, with other SIGP handling
code.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h |  2 --
 target/s390x/helper.c         | 67 ----------------------------------
 target/s390x/sigp.c           | 68 +++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 69 deletions(-)

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index a49dca94a75..9691366ec91 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -323,8 +323,6 @@ void s390x_cpu_timer(void *opaque);
 void s390_handle_wait(S390CPU *cpu);
 hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
-#define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
-int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch);
 LowCore *cpu_map_lowcore(CPUS390XState *env);
 void cpu_unmap_lowcore(CPUS390XState *env, LowCore *lowcore);
 #endif /* CONFIG_USER_ONLY */
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 84321e1d68e..184428c6d9d 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -24,7 +24,6 @@
 #include "gdbstub/helpers.h"
 #include "qemu/timer.h"
 #include "hw/s390x/ioinst.h"
-#include "target/s390x/kvm/pv.h"
 #include "system/hw_accel.h"
 #include "system/memory.h"
 #include "system/runstate.h"
@@ -182,69 +181,3 @@ void s390_cpu_recompute_watchpoints(CPUState *cs)
                               wp_flags, NULL);
     }
 }
-
-typedef struct SigpSaveArea {
-    uint64_t    fprs[16];                       /* 0x0000 */
-    uint64_t    grs[16];                        /* 0x0080 */
-    PSW         psw;                            /* 0x0100 */
-    uint8_t     pad_0x0110[0x0118 - 0x0110];    /* 0x0110 */
-    uint32_t    prefix;                         /* 0x0118 */
-    uint32_t    fpc;                            /* 0x011c */
-    uint8_t     pad_0x0120[0x0124 - 0x0120];    /* 0x0120 */
-    uint32_t    todpr;                          /* 0x0124 */
-    uint64_t    cputm;                          /* 0x0128 */
-    uint64_t    ckc;                            /* 0x0130 */
-    uint8_t     pad_0x0138[0x0140 - 0x0138];    /* 0x0138 */
-    uint32_t    ars[16];                        /* 0x0140 */
-    uint64_t    crs[16];                        /* 0x0384 */
-} SigpSaveArea;
-QEMU_BUILD_BUG_ON(sizeof(SigpSaveArea) != 512);
-
-int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
-{
-    static const uint8_t ar_id = 1;
-    SigpSaveArea *sa;
-    hwaddr len = sizeof(*sa);
-    int i;
-
-    /* For PVMs storing will occur when this cpu enters SIE again */
-    if (s390_is_pv()) {
-        return 0;
-    }
-
-    sa = cpu_physical_memory_map(addr, &len, true);
-    if (!sa) {
-        return -EFAULT;
-    }
-    if (len != sizeof(*sa)) {
-        cpu_physical_memory_unmap(sa, len, 1, 0);
-        return -EFAULT;
-    }
-
-    if (store_arch) {
-        cpu_physical_memory_write(offsetof(LowCore, ar_access_id), &ar_id, 1);
-    }
-    for (i = 0; i < 16; ++i) {
-        sa->fprs[i] = cpu_to_be64(*get_freg(&cpu->env, i));
-    }
-    for (i = 0; i < 16; ++i) {
-        sa->grs[i] = cpu_to_be64(cpu->env.regs[i]);
-    }
-    sa->psw.addr = cpu_to_be64(cpu->env.psw.addr);
-    sa->psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(&cpu->env));
-    sa->prefix = cpu_to_be32(cpu->env.psa);
-    sa->fpc = cpu_to_be32(cpu->env.fpc);
-    sa->todpr = cpu_to_be32(cpu->env.todpr);
-    sa->cputm = cpu_to_be64(cpu->env.cputm);
-    sa->ckc = cpu_to_be64(cpu->env.ckc >> 8);
-    for (i = 0; i < 16; ++i) {
-        sa->ars[i] = cpu_to_be32(cpu->env.aregs[i]);
-    }
-    for (i = 0; i < 16; ++i) {
-        sa->crs[i] = cpu_to_be64(cpu->env.cregs[i]);
-    }
-
-    cpu_physical_memory_unmap(sa, len, 1, len);
-
-    return 0;
-}
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 1464be76983..2487acdb49e 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -19,6 +19,7 @@
 #include "system/tcg.h"
 #include "trace.h"
 #include "qapi/qapi-types-machine.h"
+#include "target/s390x/kvm/pv.h"
 
 QemuMutex qemu_sigp_mutex;
 
@@ -126,6 +127,73 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg)
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 }
 
+typedef struct SigpSaveArea {
+    uint64_t    fprs[16];                       /* 0x0000 */
+    uint64_t    grs[16];                        /* 0x0080 */
+    PSW         psw;                            /* 0x0100 */
+    uint8_t     pad_0x0110[0x0118 - 0x0110];    /* 0x0110 */
+    uint32_t    prefix;                         /* 0x0118 */
+    uint32_t    fpc;                            /* 0x011c */
+    uint8_t     pad_0x0120[0x0124 - 0x0120];    /* 0x0120 */
+    uint32_t    todpr;                          /* 0x0124 */
+    uint64_t    cputm;                          /* 0x0128 */
+    uint64_t    ckc;                            /* 0x0130 */
+    uint8_t     pad_0x0138[0x0140 - 0x0138];    /* 0x0138 */
+    uint32_t    ars[16];                        /* 0x0140 */
+    uint64_t    crs[16];                        /* 0x0384 */
+} SigpSaveArea;
+QEMU_BUILD_BUG_ON(sizeof(SigpSaveArea) != 512);
+
+#define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
+static int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
+{
+    static const uint8_t ar_id = 1;
+    SigpSaveArea *sa;
+    hwaddr len = sizeof(*sa);
+    int i;
+
+    /* For PVMs storing will occur when this cpu enters SIE again */
+    if (s390_is_pv()) {
+        return 0;
+    }
+
+    sa = cpu_physical_memory_map(addr, &len, true);
+    if (!sa) {
+        return -EFAULT;
+    }
+    if (len != sizeof(*sa)) {
+        cpu_physical_memory_unmap(sa, len, 1, 0);
+        return -EFAULT;
+    }
+
+    if (store_arch) {
+        cpu_physical_memory_write(offsetof(LowCore, ar_access_id), &ar_id, 1);
+    }
+    for (i = 0; i < 16; ++i) {
+        sa->fprs[i] = cpu_to_be64(*get_freg(&cpu->env, i));
+    }
+    for (i = 0; i < 16; ++i) {
+        sa->grs[i] = cpu_to_be64(cpu->env.regs[i]);
+    }
+    sa->psw.addr = cpu_to_be64(cpu->env.psw.addr);
+    sa->psw.mask = cpu_to_be64(s390_cpu_get_psw_mask(&cpu->env));
+    sa->prefix = cpu_to_be32(cpu->env.psa);
+    sa->fpc = cpu_to_be32(cpu->env.fpc);
+    sa->todpr = cpu_to_be32(cpu->env.todpr);
+    sa->cputm = cpu_to_be64(cpu->env.cputm);
+    sa->ckc = cpu_to_be64(cpu->env.ckc >> 8);
+    for (i = 0; i < 16; ++i) {
+        sa->ars[i] = cpu_to_be32(cpu->env.aregs[i]);
+    }
+    for (i = 0; i < 16; ++i) {
+        sa->crs[i] = cpu_to_be64(cpu->env.cregs[i]);
+    }
+
+    cpu_physical_memory_unmap(sa, len, 1, len);
+
+    return 0;
+}
+
 static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
 {
     S390CPU *cpu = S390_CPU(cs);
-- 
2.51.0

[PATCH 9/9] target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (3/3)

[Philippe Mathieu-Daudé]

Commit b7ecba0f6f6 ("docs/devel/loads-stores.rst: Document our
various load and store APIs") mentioned cpu_physical_memory_*()
methods are legacy, the replacement being address_space_*().

Replace the *_map() / *_unmap() methods in s390_store_status()
and s390_store_adtl_status(). In s390_store_status(), replace
cpu_physical_memory_write() by address_space_stb(), restricting
@ar_id scope. No behavioral change expected.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/sigp.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 2487acdb49e..f5d7bc0fa22 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -13,6 +13,7 @@
 #include "s390x-internal.h"
 #include "hw/boards.h"
 #include "system/hw_accel.h"
+#include "system/memory.h"
 #include "system/runstate.h"
 #include "system/address-spaces.h"
 #include "exec/cputlb.h"
@@ -147,7 +148,8 @@ QEMU_BUILD_BUG_ON(sizeof(SigpSaveArea) != 512);
 #define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
 static int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
 {
-    static const uint8_t ar_id = 1;
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+    AddressSpace *as = CPU(cpu)->as;
     SigpSaveArea *sa;
     hwaddr len = sizeof(*sa);
     int i;
@@ -157,17 +159,21 @@ static int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
         return 0;
     }
 
-    sa = cpu_physical_memory_map(addr, &len, true);
+    sa = address_space_map(as, addr, &len, true, attrs);
     if (!sa) {
         return -EFAULT;
     }
     if (len != sizeof(*sa)) {
-        cpu_physical_memory_unmap(sa, len, 1, 0);
+        address_space_unmap(as, sa, len, true, 0);
         return -EFAULT;
     }
 
     if (store_arch) {
-        cpu_physical_memory_write(offsetof(LowCore, ar_access_id), &ar_id, 1);
+        static const uint8_t ar_id = 1;
+
+        address_space_stb(as, offsetof(LowCore, ar_access_id),
+                          ar_id, attrs, NULL);
+
     }
     for (i = 0; i < 16; ++i) {
         sa->fprs[i] = cpu_to_be64(*get_freg(&cpu->env, i));
@@ -189,7 +195,7 @@ static int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
         sa->crs[i] = cpu_to_be64(cpu->env.cregs[i]);
     }
 
-    cpu_physical_memory_unmap(sa, len, 1, len);
+    address_space_unmap(as, sa, len, true, len);
 
     return 0;
 }
@@ -251,16 +257,18 @@ QEMU_BUILD_BUG_ON(sizeof(SigpAdtlSaveArea) != 4096);
 #define ADTL_GS_MIN_SIZE 2048 /* minimal size of adtl save area for GS */
 static int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
 {
+    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+    AddressSpace *as = CPU(cpu)->as;
     SigpAdtlSaveArea *sa;
     hwaddr save = len;
     int i;
 
-    sa = cpu_physical_memory_map(addr, &save, true);
+    sa = address_space_map(as, addr, &save, true, attrs);
     if (!sa) {
         return -EFAULT;
     }
     if (save != len) {
-        cpu_physical_memory_unmap(sa, len, 1, 0);
+        address_space_unmap(as, sa, len, true, 0);
         return -EFAULT;
     }
 
@@ -276,7 +284,8 @@ static int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
         }
     }
 
-    cpu_physical_memory_unmap(sa, len, 1, len);
+    address_space_unmap(as, sa, len, true, len);
+
     return 0;
 }
 
-- 
2.51.0

Re: [PATCH 1/9] target/s390x: Factor diag_iplb_write() out

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:

Can you please provide some reasoning in the patch description?
The code is only used in one place, so I fail to see the reason at the first 
glance.

  Thanks,
   Thomas


> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/diag.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index da44b0133ed..ed320fc0c1f 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -73,6 +73,17 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>       return 0;
>   }
>   
> +static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
> +{
> +    const size_t iplb_len = be32_to_cpu(iplb->len);
> +
> +    if (s390_is_pv()) {
> +        s390_cpu_pv_mem_write(cpu, 0, iplb, iplb_len);
> +    } else {
> +        cpu_physical_memory_write(addr, iplb, iplb_len);
> +    }
> +}
> +
>   void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>   {
>       bool valid;
> @@ -164,11 +175,7 @@ out:
>               return;
>           }
>   
> -        if (!s390_is_pv()) {
> -            cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
> -        } else {
> -            s390_cpu_pv_mem_write(cpu, 0, iplb, be32_to_cpu(iplb->len));
> -        }
> +        diag_iplb_write(iplb, cpu, addr);
>           env->regs[r1 + 1] = DIAG_308_RC_OK;
>           return;
>       case DIAG308_PV_START:

Re: [PATCH 2/9] target/s390x: Factor diag_iplb_read() out

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:

This patch is also missing a proper reasoning in the patch description.

  Thomas


> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/diag.c | 31 +++++++++++++++++++------------
>   1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index ed320fc0c1f..c2fedc55213 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -73,6 +73,24 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>       return 0;
>   }
>   
> +static bool diag_iplb_read(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
> +{
> +    if (s390_is_pv()) {
> +        s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
> +        if (!iplb_valid_len(iplb)) {
> +            return false;
> +        }
> +        s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
> +    } else {
> +        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
> +        if (!iplb_valid_len(iplb)) {
> +            return false;
> +        }
> +        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> +    }
> +    return true;
> +}
> +
>   static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
>   {
>       const size_t iplb_len = be32_to_cpu(iplb->len);
> @@ -125,23 +143,12 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>               return;
>           }
>           iplb = g_new0(IplParameterBlock, 1);
> -        if (!s390_is_pv()) {
> -            cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
> -        } else {
> -            s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
> -        }
>   
> -        if (!iplb_valid_len(iplb)) {
> +        if (!diag_iplb_read(iplb, cpu, addr)) {
>               env->regs[r1 + 1] = DIAG_308_RC_INVALID;
>               goto out;
>           }
>   
> -        if (!s390_is_pv()) {
> -            cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> -        } else {
> -            s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
> -        }
> -
>           valid = subcode == DIAG308_PV_SET ? iplb_valid_pv(iplb) : iplb_valid(iplb);
>           if (!valid) {
>               if (subcode == DIAG308_SET && iplb->pbt == S390_IPL_TYPE_QEMU_SCSI) {

Re: [PATCH 3/9] target/s390x: Replace legacy cpu_physical_memory_read/write() calls

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
> cpu_physical_memory_read() and cpu_physical_memory_write() are
> legacy (see commit b7ecba0f6f6), replace by address_space_read()
> and address_space_write() respectively.

I'm not sure whether this patch is a good idea in the current way it is done.

Commit b7ecba0f6f6 says: "there is likely to be behaviour you need to model 
correctly for a failed read or write operation" ... so if we switch to the 
address_space_* API, I think you should also implement the correct handling 
for the case where the memory transaction failed. Otherwise this is more or 
less just code churn, isn't it?

  Thomas

> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index c2fedc55213..737c3bbc5be 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -17,6 +17,7 @@
>   #include "s390x-internal.h"
>   #include "hw/watchdog/wdt_diag288.h"
>   #include "system/cpus.h"
> +#include "system/memory.h"
>   #include "hw/s390x/ipl.h"
>   #include "hw/s390x/s390-virtio-ccw.h"
>   #include "system/kvm.h"
> @@ -82,11 +83,14 @@ static bool diag_iplb_read(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr)
>           }
>           s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
>       } else {
> -        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
> +        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> +        AddressSpace *as = CPU(cpu)->as;
> +
> +        address_space_read(as, addr, attrs, iplb, sizeof(iplb->len));
>           if (!iplb_valid_len(iplb)) {
>               return false;
>           }
> -        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
> +        address_space_read(as, addr, attrs, iplb, be32_to_cpu(iplb->len));
>       }
>       return true;
>   }
> @@ -98,7 +102,10 @@ static void diag_iplb_write(IplParameterBlock *iplb, S390CPU *cpu, uint64_t addr
>       if (s390_is_pv()) {
>           s390_cpu_pv_mem_write(cpu, 0, iplb, iplb_len);
>       } else {
> -        cpu_physical_memory_write(addr, iplb, iplb_len);
> +        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> +        AddressSpace *as = CPU(cpu)->as;
> +
> +        address_space_write(as, addr, attrs, iplb, iplb_len);
>       }
>   }
>   

Re: [PATCH 5/9] target/s390x: Propagate CPUS390XState to cpu_unmap_lowcore()

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
> To be able to access the CPU state in cpu_unmap_lowcore()
> in the next commit, propagate it as argument.
> cpu_map_lowcore() already takes it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h  |  2 +-
>   target/s390x/helper.c          |  4 ++--
>   target/s390x/tcg/excp_helper.c | 10 +++++-----
>   target/s390x/tcg/misc_helper.c |  4 ++--
>   4 files changed, 10 insertions(+), 10 deletions(-)

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

Re: [PATCH 6/9] target/s390x: Replace legacy cpu_physical_memory_[un]map() calls (2/3)

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
> Commit b7ecba0f6f6 ("docs/devel/loads-stores.rst: Document our
> various load and store APIs") mentioned cpu_physical_memory_*()
> methods are legacy, the replacement being address_space_*().
> 
> Replace the *_map() / *_unmap() methods in cpu_[un]map_lowcore().
> No behavioral change expected.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/helper.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

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

Re: [PATCH 7/9] target/s390x: Reduce s390_store_adtl_status() scope

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
> s390_store_adtl_status() is only called within sigp.c,
> move it and the SigpAdtlSaveArea structure definition
> there where it belongs, with other SIGP handling code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h |  1 -
>   target/s390x/helper.c         | 40 -----------------------------------
>   target/s390x/sigp.c           | 40 +++++++++++++++++++++++++++++++++++
>   3 files changed, 40 insertions(+), 41 deletions(-)

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

Re: [PATCH 8/9] target/s390x: Reduce s390_store_status() scope

[Thomas Huth]

On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
> s390_store_status() is only called within sigp.c,
> move it and the SigpSaveArea structure definition
> there where it belongs, with other SIGP handling
> code.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h |  2 --
>   target/s390x/helper.c         | 67 ----------------------------------
>   target/s390x/sigp.c           | 68 +++++++++++++++++++++++++++++++++++
>   3 files changed, 68 insertions(+), 69 deletions(-)

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

Re: [PATCH 3/9] target/s390x: Replace legacy cpu_physical_memory_read/write() calls

[Philippe Mathieu-Daudé]

On 6/10/25 16:04, Thomas Huth wrote:
> On 02/10/2025 11.11, Philippe Mathieu-Daudé wrote:
>> cpu_physical_memory_read() and cpu_physical_memory_write() are
>> legacy (see commit b7ecba0f6f6), replace by address_space_read()
>> and address_space_write() respectively.
> 
> I'm not sure whether this patch is a good idea in the current way it is 
> done.
> 
> Commit b7ecba0f6f6 says: "there is likely to be behaviour you need to 
> model correctly for a failed read or write operation" ... so if we 
> switch to the address_space_* API, I think you should also implement the 
> correct handling for the case where the memory transaction failed. 
> Otherwise this is more or less just code churn, isn't it?

Yes and no :) The point is to trigger reviewers/maintainers to fill the
missing parts when they notice them, otherwise this is just a no-op.

It worked because now I know the error path you expect, which is
currently ignored.

> 
>   Thomas
> 
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index c2fedc55213..737c3bbc5be 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -17,6 +17,7 @@
>>   #include "s390x-internal.h"
>>   #include "hw/watchdog/wdt_diag288.h"
>>   #include "system/cpus.h"
>> +#include "system/memory.h"
>>   #include "hw/s390x/ipl.h"
>>   #include "hw/s390x/s390-virtio-ccw.h"
>>   #include "system/kvm.h"
>> @@ -82,11 +83,14 @@ static bool diag_iplb_read(IplParameterBlock 
>> *iplb, S390CPU *cpu, uint64_t addr)
>>           }
>>           s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
>>       } else {
>> -        cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
>> +        const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>> +        AddressSpace *as = CPU(cpu)->as;
>> +
>> +        address_space_read(as, addr, attrs, iplb, sizeof(iplb->len));
>>           if (!iplb_valid_len(iplb)) {
>>               return false;
>>           }
>> -        cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
>> +        address_space_read(as, addr, attrs, iplb, be32_to_cpu(iplb- 
>> >len));
>>       }
>>       return true;
>>   }