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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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