On Thu, 19 Mar 2026 at 17:17, Peter Maydell <[email protected]> wrote: > > On Tue, 17 Mar 2026 at 12:25, Zenghui Yu <[email protected]> wrote: > > > > As per the pseudo code from DDI0487 M.a.a (on J1-16021) AArch64.S1Walk(): > > > > // Check descriptor AF bit > > elsif (descriptor<10> == '0' && walkparams.ha == '0' && > > (!accdesc.acctype IN {AccessType_DC, AccessType_IC} || > > boolean IMPLEMENTATION_DEFINED "Generate access flag fault on > > IC/DC operations")) then > > fault.statuscode = Fault_AccessFlag; > > > > an access flag fault should be generated for AccessType_AT, if the AF bit > > is 0 and !param.ha. > > Did you find this by code inspection, or because it caused > a guest to go wrong? > > We should probably > > Cc: [email protected] > > > Fixes: efebeec13d07 ("target/arm: Skip AF and DB updates for AccessType_AT") > > Signed-off-by: Zenghui Yu <[email protected]> > > --- > > target/arm/ptw.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > > index 8b8dc09e72..572048d560 100644 > > --- a/target/arm/ptw.c > > +++ b/target/arm/ptw.c > > @@ -2118,6 +2118,12 @@ static bool get_phys_addr_lpae(CPUARMState *env, > > S1Translate *ptw, > > descaddr &= ~(hwaddr)(page_size - 1); > > descaddr |= (address & (page_size - 1)); > > > > + /* Check descriptor AF bit */ > > + if (!(descriptor & (1 << 10)) && !param.ha) { > > + fi->type = ARMFault_AccessFlag; > > + goto do_fault; > > + } > > + > > /* > > * For AccessType_AT, DB is not updated (AArch64.SetDirtyFlag), > > * and it is IMPLEMENTATION DEFINED whether AF is updated > > @@ -2127,15 +2133,9 @@ static bool get_phys_addr_lpae(CPUARMState *env, > > S1Translate *ptw, > > /* > > * Access flag. > > * If HA is enabled, prepare to update the descriptor below. > > - * Otherwise, pass the access fault on to software. > > */ > > - if (!(descriptor & (1 << 10))) { > > - if (param.ha) { > > - new_descriptor |= 1 << 10; /* AF */ > > - } else { > > - fi->type = ARMFault_AccessFlag; > > - goto do_fault; > > - } > > + if (!(descriptor & (1 << 10)) && param.ha) { > > + new_descriptor |= 1 << 10; /* AF */ > > } > > This is definitely correct behaviour for AT insns. The only > thing I'm wondering about is what we should do for the > ptw->in_debug == true case. Before commit efebeec13d07 we > were checking !ptw->in_debug to skip both the testing and > the updating of the access flag; now we will skip the update > (definitely correct) but will fail debug accesses if the > access flag is not set. > > There's an argument that if you're doing a debugger access > it's not very helpful for it to not work just because the OS > happens to have cleared the access flag on the page table entry > (we also ignore permissions on debug accesses, though there > are some code paths where we do enforce permissions, not > really intentionally). > > On the other hand, we don't and I think have never suppressed > AccessFlag faults for the get_phys_addr_v6 short-descriptor > tables, so honouring them here too is at least consistent.
Having thought about this a little more, I think we should continue to not raise the AccessFlag fault for in_debug = true: it is what we've been doing previously for LPAE, at least, and it is what intention of the debugger access codepath is. Could you do a v2 of this patch that handles in_debug = true, please? thanks -- PMM
