On 3/24/26 9:38 PM, Peter Maydell wrote: > 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?
Sure, sent out now. Thanks for your suggestion! Zenghui
