The translate_for_debug method is supposed to return attributes
that include the debug flag being set. We forgot this when
implementing the method for Arm.

Fixes: abefca8e7f957 ("target/arm: Implement translate_for_debug")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8706dd59dd..0693d2867e 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3961,6 +3961,7 @@ static bool arm_cpu_get_phys_addr(CPUARMState *env, vaddr addr,
         /* translation succeeded */
         result->physaddr = res.f.phys_addr;
         result->attrs = res.f.attrs;
+        result->attrs.debug = 1;
         result->lg_page_size = res.f.lg_page_size;
     }
     return fault;
-- 
2.43.0

On 5/15/26 06:12, Peter Maydell wrote:
> The translate_for_debug method is supposed to return attributes
> that include the debug flag being set. We forgot this when
> implementing the method for Arm.
> 
> Fixes: abefca8e7f957 ("target/arm: Implement translate_for_debug")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 8706dd59dd..0693d2867e 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -3961,6 +3961,7 @@ static bool arm_cpu_get_phys_addr(CPUARMState *env, vaddr addr,
>           /* translation succeeded */
>           result->physaddr = res.f.phys_addr;
>           result->attrs = res.f.attrs;
> +        result->attrs.debug = 1;
>           result->lg_page_size = res.f.lg_page_size;
>       }
>       return fault;

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

r~

On 15/5/26 15:12, Peter Maydell wrote:
> The translate_for_debug method is supposed to return attributes
> that include the debug flag being set. We forgot this when
> implementing the method for Arm.
> 
> Fixes: abefca8e7f957 ("target/arm: Implement translate_for_debug")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/ptw.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 8706dd59dd..0693d2867e 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -3961,6 +3961,7 @@ static bool arm_cpu_get_phys_addr(CPUARMState *env, vaddr addr,
>           /* translation succeeded */
>           result->physaddr = res.f.phys_addr;
>           result->attrs = res.f.attrs;
> +        result->attrs.debug = 1;

I missed that because thought we'd set it on success in
cpu_translate_for_debug(), but we only do it for the fallback
case. Shouldn't it be safer to set it there instead?

Otherwise I'd expect the bit set in arm_cpu_translate_for_debug().
not in arm_cpu_get_phys_addr(). WDYT?

>           result->lg_page_size = res.f.lg_page_size;
>       }
>       return fault;

On Fri, 15 May 2026 at 15:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 15/5/26 15:12, Peter Maydell wrote:
> > The translate_for_debug method is supposed to return attributes
> > that include the debug flag being set. We forgot this when
> > implementing the method for Arm.
> >
> > Fixes: abefca8e7f957 ("target/arm: Implement translate_for_debug")
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   target/arm/ptw.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> > index 8706dd59dd..0693d2867e 100644
> > --- a/target/arm/ptw.c
> > +++ b/target/arm/ptw.c
> > @@ -3961,6 +3961,7 @@ static bool arm_cpu_get_phys_addr(CPUARMState *env, vaddr addr,
> >           /* translation succeeded */
> >           result->physaddr = res.f.phys_addr;
> >           result->attrs = res.f.attrs;
> > +        result->attrs.debug = 1;
>
> I missed that because thought we'd set it on success in
> cpu_translate_for_debug(), but we only do it for the fallback
> case. Shouldn't it be safer to set it there instead?

Mmm. The idea of the method was "the implementation
should set the attributes entirely, not set them partially
and then have them adjusted by the wrapper function"; also
it's a bit confusing if the semantics of the wrapper function
aren't the same as the semantics of the method proper. On the
other hand, it is a bit of an easy mistake to make, so the
API as it stands is a bit bug-prone.

> Otherwise I'd expect the bit set in arm_cpu_translate_for_debug().
> not in arm_cpu_get_phys_addr(). WDYT?

I went for arm_cpu_get_phys_addr() because it's this
function that sets ".in_debug = true" in the S1Translate
struct it uses, so it matches that it also sets the
attributes to match that.

The arm_cpu_get_phys_addr() function is a bit misnamed
by this point -- it would probably be clearer to name it
translate_for_debug_for_mmuidx() or something similar,
since it's just a helper function that has the same API
as translate_for_debug() but takes the mmuidx to use.

-- PMM