In do_ats_write() we try to assert that the cacheattrs from
get_phys_addr_for_at() are in the form we expect:

    /*
     * ATS operations only do S1 or S1+S2 translations, so we never
     * have to deal with the ARMCacheAttrs format for S2 only.
     */
    assert(!res.cacheattrs.is_s2_format);

However, the GetPhysAddrResult struct documents that its fields are
only valid when the page table walk succeeded.  For a two stage page
table walk which fails during stage two, we will return early from
get_phys_addr_twostage() and depending on the fault type the
res.cacheattrs may have been initialized with the stage 2 cache attr
information in stage 2 format.  In this case we will incorrectly
assert here.

Fix the assertion to not look at the res fields if the lookup failed.

Note for stable backports: the do_ats_write() function is in
target/arm/helper.c in older QEMU versions, but the change to the
assert line is the same.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3328
Fixes: 9f225e607f21 ("target/arm: Postpone interpretation of stage 2 descriptor attribute bits")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Not a regression, but it's a very safe fix, so this will probably
get into 11.0.

 target/arm/tcg/cpregs-at.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/tcg/cpregs-at.c b/target/arm/tcg/cpregs-at.c
index 0e8f229aa7..53dd67375d 100644
--- a/target/arm/tcg/cpregs-at.c
+++ b/target/arm/tcg/cpregs-at.c
@@ -37,8 +37,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     /*
      * ATS operations only do S1 or S1+S2 translations, so we never
      * have to deal with the ARMCacheAttrs format for S2 only.
+     * (Note that res fields are only valid on ptw success.)
      */
-    assert(!res.cacheattrs.is_s2_format);
+    assert(ret || !res.cacheattrs.is_s2_format);
 
     if (ret) {
         /*
-- 
2.43.0

On 3/31/26 19:23, Peter Maydell wrote:
> In do_ats_write() we try to assert that the cacheattrs from
> get_phys_addr_for_at() are in the form we expect:
> 
>      /*
>       * ATS operations only do S1 or S1+S2 translations, so we never
>       * have to deal with the ARMCacheAttrs format for S2 only.
>       */
>      assert(!res.cacheattrs.is_s2_format);
> 
> However, the GetPhysAddrResult struct documents that its fields are
> only valid when the page table walk succeeded.  For a two stage page
> table walk which fails during stage two, we will return early from
> get_phys_addr_twostage() and depending on the fault type the
> res.cacheattrs may have been initialized with the stage 2 cache attr
> information in stage 2 format.  In this case we will incorrectly
> assert here.
> 
> Fix the assertion to not look at the res fields if the lookup failed.
> 
> Note for stable backports: the do_ats_write() function is in
> target/arm/helper.c in older QEMU versions, but the change to the
> assert line is the same.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3328
> Fixes: 9f225e607f21 ("target/arm: Postpone interpretation of stage 2 descriptor attribute bits")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Not a regression, but it's a very safe fix, so this will probably
> get into 11.0.
> 
>   target/arm/tcg/cpregs-at.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/tcg/cpregs-at.c b/target/arm/tcg/cpregs-at.c
> index 0e8f229aa7..53dd67375d 100644
> --- a/target/arm/tcg/cpregs-at.c
> +++ b/target/arm/tcg/cpregs-at.c
> @@ -37,8 +37,9 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
>       /*
>        * ATS operations only do S1 or S1+S2 translations, so we never
>        * have to deal with the ARMCacheAttrs format for S2 only.
> +     * (Note that res fields are only valid on ptw success.)
>        */
> -    assert(!res.cacheattrs.is_s2_format);
> +    assert(ret || !res.cacheattrs.is_s2_format);
>   
>       if (ret) {
>           /*

The assert could plausibly be moved somewhere !ret is already tested.  E.g. just before 
the only use of cacheattrs.  That said,

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

r~

On 31/3/26 11:23, Peter Maydell wrote:
> In do_ats_write() we try to assert that the cacheattrs from
> get_phys_addr_for_at() are in the form we expect:
> 
>      /*
>       * ATS operations only do S1 or S1+S2 translations, so we never
>       * have to deal with the ARMCacheAttrs format for S2 only.
>       */
>      assert(!res.cacheattrs.is_s2_format);
> 
> However, the GetPhysAddrResult struct documents that its fields are
> only valid when the page table walk succeeded.  For a two stage page
> table walk which fails during stage two, we will return early from
> get_phys_addr_twostage() and depending on the fault type the
> res.cacheattrs may have been initialized with the stage 2 cache attr
> information in stage 2 format.  In this case we will incorrectly
> assert here.
> 
> Fix the assertion to not look at the res fields if the lookup failed.
> 
> Note for stable backports: the do_ats_write() function is in
> target/arm/helper.c in older QEMU versions, but the change to the
> assert line is the same.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3328
> Fixes: 9f225e607f21 ("target/arm: Postpone interpretation of stage 2 descriptor attribute bits")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Not a regression, but it's a very safe fix, so this will probably
> get into 11.0.
> 
>   target/arm/tcg/cpregs-at.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>