[PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[Zenghui Yu]

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.

Fixes: efebeec13d07 ("target/arm: Skip AF and DB updates for AccessType_AT")
Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
---
 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 */
         }
 
         /*
-- 
2.53.0

Re: [PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[Peter Maydell]

On Tue, 17 Mar 2026 at 12:25, Zenghui Yu <zenghui.yu@linux.dev> 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: qemu-stable@nongnu.org

> Fixes: efebeec13d07 ("target/arm: Skip AF and DB updates for AccessType_AT")
> Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> ---
>  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.

Richard, what do you think?

thanks
-- PMM

Re: [PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[Zenghui Yu]

Hi Peter,

On 3/20/26 1:17 AM, Peter Maydell wrote:
> On Tue, 17 Mar 2026 at 12:25, Zenghui Yu <zenghui.yu@linux.dev> 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?

This was noticed when I was fixing the AT selftest on KVM side [*].

[*] https://lore.kernel.org/r/d58819b9-c745-4551-8ea4-e15af3fe63be@linux.dev

Thanks,
Zenghui

Re: [PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[Peter Maydell]

On Thu, 19 Mar 2026 at 17:17, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 17 Mar 2026 at 12:25, Zenghui Yu <zenghui.yu@linux.dev> 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: qemu-stable@nongnu.org
>
> > Fixes: efebeec13d07 ("target/arm: Skip AF and DB updates for AccessType_AT")
> > Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> > ---
> >  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

Re: [PATCH] target/arm: Don't skip access flag fault for AccessType_AT

[Zenghui Yu]

On 3/24/26 9:38 PM, Peter Maydell wrote:
> On Thu, 19 Mar 2026 at 17:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 17 Mar 2026 at 12:25, Zenghui Yu <zenghui.yu@linux.dev> 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: qemu-stable@nongnu.org
> >
> > > Fixes: efebeec13d07 ("target/arm: Skip AF and DB updates for AccessType_AT")
> > > Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
> > > ---
> > >  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