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

[Thomas Huth <thuth@redhat.com>]

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);
>       }
>   }
>