On Fri, 15 May 2026 at 15:17, Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]> > > --- > > 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
