Commit 500131154d6 ("exec.c: Add new address_space_ld*/st*
functions") added a new API to fix a shortcoming of the
ld/st*_phys() API, which does blind bus access, not reporting
failure (and it also allow to provide transaction attributes).

Later commit 42874d3a8c6 ("Switch non-CPU callers from ld/st*_phys
to address_space_ld/st*") automatically converted the legacy uses
to the new API, not precising transaction attributes
(MEMTXATTRS_UNSPECIFIED) and ignoring the transation result (passing
NULL pointer as MemTxResult).

While this is a faithful replacement, without any logical change,
we later realized better is to not use MEMTXATTRS_UNSPECIFIED or
NULL MemTxResult, and adapt each call site on a pair basis, looking
at the device model datasheet to do the correct behavior (which is
unlikely to ignore transaction failures).

Since this is quite some work, we defer that to device model
maintainers. Meanwhile we introduce a definition, to allow a
target which removed all legacy API call to prohibit further
legacy API uses, named "TARGET_NOT_USING_LEGACY_LDST_PHYS_API".

Since all targets should be able to check this definition, we
take care to not poison it.

Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/system/memory.h       | 2 ++
 scripts/make-config-poison.sh | 1 +
 2 files changed, 3 insertions(+)

diff --git a/include/system/memory.h b/include/system/memory.h
index e69171de05a..d5c248f1794 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -2850,10 +2850,12 @@ MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
 #define ARG1_DECL    AddressSpace *as
 #include "exec/memory_ldst.h.inc"
 
+#ifndef TARGET_NOT_USING_LEGACY_LDST_PHYS_API
 #define SUFFIX
 #define ARG1         as
 #define ARG1_DECL    AddressSpace *as
 #include "exec/memory_ldst_phys.h.inc"
+#endif
 
 struct MemoryRegionCache {
     uint8_t *ptr;
diff --git a/scripts/make-config-poison.sh b/scripts/make-config-poison.sh
index 2b36907e239..937357b3531 100755
--- a/scripts/make-config-poison.sh
+++ b/scripts/make-config-poison.sh
@@ -10,6 +10,7 @@ exec sed -n \
   -e' /CONFIG_TCG/d' \
   -e '/CONFIG_USER_ONLY/d' \
   -e '/CONFIG_SOFTMMU/d' \
+  -e '/TARGET_NOT_USING_LEGACY_LDST_PHYS_API/d' \
   -e '/^#define / {' \
   -e    's///' \
   -e    's/ .*//' \
-- 
2.52.0

On 12/25/25 02:13, Philippe Mathieu-Daudé wrote:
> Commit 500131154d6 ("exec.c: Add new address_space_ld*/st*
> functions") added a new API to fix a shortcoming of the
> ld/st*_phys() API, which does blind bus access, not reporting
> failure (and it also allow to provide transaction attributes).
> 
> Later commit 42874d3a8c6 ("Switch non-CPU callers from ld/st*_phys
> to address_space_ld/st*") automatically converted the legacy uses
> to the new API, not precising transaction attributes
> (MEMTXATTRS_UNSPECIFIED) and ignoring the transation result (passing
> NULL pointer as MemTxResult).
> 
> While this is a faithful replacement, without any logical change,
> we later realized better is to not use MEMTXATTRS_UNSPECIFIED or
> NULL MemTxResult, and adapt each call site on a pair basis, looking
> at the device model datasheet to do the correct behavior (which is
> unlikely to ignore transaction failures).
> 
> Since this is quite some work, we defer that to device model
> maintainers. Meanwhile we introduce a definition, to allow a
> target which removed all legacy API call to prohibit further
> legacy API uses, named "TARGET_NOT_USING_LEGACY_LDST_PHYS_API".
> 
> Since all targets should be able to check this definition, we
> take care to not poison it.
> 
> Suggested-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/system/memory.h       | 2 ++
>   scripts/make-config-poison.sh | 1 +
>   2 files changed, 3 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

On Wed, Dec 24, 2025 at 5:14 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Commit 500131154d6 ("exec.c: Add new address_space_ld*/st*
> functions") added a new API to fix a shortcoming of the
> ld/st*_phys() API, which does blind bus access, not reporting
> failure (and it also allow to provide transaction attributes).
>
> Later commit 42874d3a8c6 ("Switch non-CPU callers from ld/st*_phys
> to address_space_ld/st*") automatically converted the legacy uses
> to the new API, not precising transaction attributes
> (MEMTXATTRS_UNSPECIFIED) and ignoring the transation result (passing
> NULL pointer as MemTxResult).
>
> While this is a faithful replacement, without any logical change,
> we later realized better is to not use MEMTXATTRS_UNSPECIFIED or
> NULL MemTxResult, and adapt each call site on a pair basis, looking
> at the device model datasheet to do the correct behavior (which is
> unlikely to ignore transaction failures).
>
> Since this is quite some work, we defer that to device model
> maintainers. Meanwhile we introduce a definition, to allow a
> target which removed all legacy API call to prohibit further
> legacy API uses, named "TARGET_NOT_USING_LEGACY_LDST_PHYS_API".
>

Personally the negation (i.e. TARGET_USING_LEGACY_LDST_PHYS_API) would
make more sense since you're opting-in an API but that must be more
complex to introduce I guess?

In any case:

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>


> Since all targets should be able to check this definition, we
> take care to not poison it.
>
> Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/system/memory.h       | 2 ++
>  scripts/make-config-poison.sh | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/include/system/memory.h b/include/system/memory.h
> index e69171de05a..d5c248f1794 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -2850,10 +2850,12 @@ MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr,
>  #define ARG1_DECL    AddressSpace *as
>  #include "exec/memory_ldst.h.inc"
>
> +#ifndef TARGET_NOT_USING_LEGACY_LDST_PHYS_API
>  #define SUFFIX
>  #define ARG1         as
>  #define ARG1_DECL    AddressSpace *as
>  #include "exec/memory_ldst_phys.h.inc"
> +#endif
>
>  struct MemoryRegionCache {
>      uint8_t *ptr;
> diff --git a/scripts/make-config-poison.sh b/scripts/make-config-poison.sh
> index 2b36907e239..937357b3531 100755
> --- a/scripts/make-config-poison.sh
> +++ b/scripts/make-config-poison.sh
> @@ -10,6 +10,7 @@ exec sed -n \
>    -e' /CONFIG_TCG/d' \
>    -e '/CONFIG_USER_ONLY/d' \
>    -e '/CONFIG_SOFTMMU/d' \
> +  -e '/TARGET_NOT_USING_LEGACY_LDST_PHYS_API/d' \
>    -e '/^#define / {' \
>    -e    's///' \
>    -e    's/ .*//' \
> --
> 2.52.0
>

On 24/12/25 19:34, Manos Pitsidianakis wrote:
> On Wed, Dec 24, 2025 at 5:14 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Commit 500131154d6 ("exec.c: Add new address_space_ld*/st*
>> functions") added a new API to fix a shortcoming of the
>> ld/st*_phys() API, which does blind bus access, not reporting
>> failure (and it also allow to provide transaction attributes).
>>
>> Later commit 42874d3a8c6 ("Switch non-CPU callers from ld/st*_phys
>> to address_space_ld/st*") automatically converted the legacy uses
>> to the new API, not precising transaction attributes
>> (MEMTXATTRS_UNSPECIFIED) and ignoring the transation result (passing
>> NULL pointer as MemTxResult).
>>
>> While this is a faithful replacement, without any logical change,
>> we later realized better is to not use MEMTXATTRS_UNSPECIFIED or
>> NULL MemTxResult, and adapt each call site on a pair basis, looking
>> at the device model datasheet to do the correct behavior (which is
>> unlikely to ignore transaction failures).
>>
>> Since this is quite some work, we defer that to device model
>> maintainers. Meanwhile we introduce a definition, to allow a
>> target which removed all legacy API call to prohibit further
>> legacy API uses, named "TARGET_NOT_USING_LEGACY_LDST_PHYS_API".
>>
> 
> Personally the negation (i.e. TARGET_USING_LEGACY_LDST_PHYS_API) would
> make more sense since you're opting-in an API but that must be more
> complex to introduce I guess?

Not much more complex, this was the previous approach (allow
all then opt out):
https://lore.kernel.org/qemu-devel/20251218212814.61445-5-philmd@linaro.org/

But Pierrick suggested the less aggressive approach, opting
in, disturbing maintainers a bit less. In term of code churn
I expect the same lines of code amount to change.

> 
> In any case:
> 
> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Thanks!