[PATCH v2 0/2] target/s390x/mmu_helper: Do not ignore address_space_rw() errors

[PATCH v2 0/2] target/s390x/mmu_helper: Do not ignore address_space_rw() errors

[Philippe Mathieu-Daudé]

v2: Addressed Thomas' review comment

Philippe Mathieu-Daudé (2):
  target/s390x/mmu_helper: Simplify s390_cpu_virt_mem_rw() logic
  target/s390x/mmu_helper: Do not ignore address_space_rw() errors

 target/s390x/mmu_helper.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.51.0

[PATCH v2 1/2] target/s390x/mmu_helper: Simplify s390_cpu_virt_mem_rw() logic

[Philippe Mathieu-Daudé]

In order to simplify the next commit, move the
trigger_access_exception() call after the address_space_rw()
calls. No logical change intended.

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

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 487c41bf933..22d3d4a97df 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -541,9 +541,7 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
     pages = g_malloc(nr_pages * sizeof(*pages));
 
     ret = translate_pages(cpu, laddr, nr_pages, pages, is_write, &tec);
-    if (ret) {
-        trigger_access_exception(&cpu->env, ret, tec);
-    } else if (hostbuf != NULL) {
+    if (ret == 0 && hostbuf != NULL) {
         AddressSpace *as = CPU(cpu)->as;
 
         /* Copy data by stepping through the area page by page */
@@ -556,6 +554,9 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
             len -= currlen;
         }
     }
+    if (ret) {
+        trigger_access_exception(&cpu->env, ret, tec);
+    }
 
     g_free(pages);
     return ret;
-- 
2.51.0

[PATCH v2 2/2] target/s390x/mmu_helper: Do not ignore address_space_rw() errors

[Philippe Mathieu-Daudé]

If a address_space_rw() call ever fails, break the loop and
return the PGM_ADDRESSING error (after triggerring an access
exception).

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

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index 22d3d4a97df..3b1e75f7833 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -546,9 +546,15 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
 
         /* Copy data by stepping through the area page by page */
         for (i = 0; i < nr_pages; i++) {
+            MemTxResult res;
+
             currlen = MIN(len, TARGET_PAGE_SIZE - (laddr % TARGET_PAGE_SIZE));
-            address_space_rw(as, pages[i] | (laddr & ~TARGET_PAGE_MASK),
-                             attrs, hostbuf, currlen, is_write);
+            res = address_space_rw(as, pages[i] | (laddr & ~TARGET_PAGE_MASK),
+                                   attrs, hostbuf, currlen, is_write);
+            if (res != MEMTX_OK) {
+                ret = PGM_ADDRESSING;
+                break;
+            }
             laddr += currlen;
             hostbuf += currlen;
             len -= currlen;
-- 
2.51.0

Re: [PATCH v2 1/2] target/s390x/mmu_helper: Simplify s390_cpu_virt_mem_rw() logic

[Thomas Huth]

On 08/10/2025 16.14, Philippe Mathieu-Daudé wrote:
> In order to simplify the next commit, move the
> trigger_access_exception() call after the address_space_rw()
> calls. No logical change intended.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/mmu_helper.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 487c41bf933..22d3d4a97df 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -541,9 +541,7 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
>       pages = g_malloc(nr_pages * sizeof(*pages));
>   
>       ret = translate_pages(cpu, laddr, nr_pages, pages, is_write, &tec);
> -    if (ret) {
> -        trigger_access_exception(&cpu->env, ret, tec);
> -    } else if (hostbuf != NULL) {
> +    if (ret == 0 && hostbuf != NULL) {
>           AddressSpace *as = CPU(cpu)->as;
>   
>           /* Copy data by stepping through the area page by page */
> @@ -556,6 +554,9 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
>               len -= currlen;
>           }
>       }
> +    if (ret) {
> +        trigger_access_exception(&cpu->env, ret, tec);
> +    }
>   
>       g_free(pages);
>       return ret;

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

Re: [PATCH v2 2/2] target/s390x/mmu_helper: Do not ignore address_space_rw() errors

[Thomas Huth]

On 08/10/2025 16.14, Philippe Mathieu-Daudé wrote:
> If a address_space_rw() call ever fails, break the loop and
> return the PGM_ADDRESSING error (after triggerring an access
> exception).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/mmu_helper.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index 22d3d4a97df..3b1e75f7833 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -546,9 +546,15 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
>   
>           /* Copy data by stepping through the area page by page */
>           for (i = 0; i < nr_pages; i++) {
> +            MemTxResult res;
> +
>               currlen = MIN(len, TARGET_PAGE_SIZE - (laddr % TARGET_PAGE_SIZE));
> -            address_space_rw(as, pages[i] | (laddr & ~TARGET_PAGE_MASK),
> -                             attrs, hostbuf, currlen, is_write);
> +            res = address_space_rw(as, pages[i] | (laddr & ~TARGET_PAGE_MASK),
> +                                   attrs, hostbuf, currlen, is_write);
> +            if (res != MEMTX_OK) {
> +                ret = PGM_ADDRESSING;
> +                break;
> +            }
>               laddr += currlen;
>               hostbuf += currlen;
>               len -= currlen;

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