Re: [PATCH v1] gdb/aarch64: Add support for FEAT_S1POE POR_EL0 register

[Luis <luis.machado.foss@gmail.com>]

On Wed, Jun 17, 2026 at 05:00:03PM +0000, srinath.parvathaneni@arm.com 
wrote:
 > From: Srinath Parvathaneni <srinath.parvathaneni@arm.com>
 >
 > Hi,
 >
 > FEAT_S1POE introduces the POR_EL0 register. This patch adds
 > support for POR_EL0 in both native GDB and gdbserver using
 > NT_ARM_POE and HWCAP2_POE detection.

Hi Srinath,

Thanks for the patch. A few comments below (some might overlap with what 
Ezra and Matthieu already raised).

 > +/* Feature check for Permission Overlay Extension.  */

Nit: FEAT_S1POE stands for Stage 1 Permission Overlay Enhancement,
not Extension. Please fix the comment.

 > +#ifndef HWCAP2_POE
 > +#define AARCH64_HWCAP2_POE (1ULL << 63)
 > +#endif

The #ifndef guard checks HWCAP2_POE but the macro being defined is
AARCH64_HWCAP2_POE. If a system header has already provided
HWCAP2_POE, AARCH64_HWCAP2_POE will silently remain undefined.

   #ifndef AARCH64_HWCAP2_POE
   #define AARCH64_HWCAP2_POE (1ULL << 63)
   #endif

 > +#ifndef AARCH64_SEGV_PKUERR
 > +#define AARCH64_SEGV_PKUERR 4  /* Protection overlay violation.  */
 > +#endif

Make it more informative:

   /* Data or instruction abort caused by a POE violation.  */

 > +/* The POE regset consists of 1 64-bit register.  */
 > +#define AARCH64_LINUX_SIZEOF_POE_REGSET (8)

Drop the parenthesis.

 > +#endif /* GDB_ARCH_AARCH64_POE_H */

Once the file is renamed the guard should become
GDB_ARCH_AARCH64_POE_LINUX_H.

 > diff --git a/gdb/nat/aarch64-poe.h b/gdb/nat/aarch64-poe.h
 > new file mode 100644
 > @@ -0,0 +1,49 @@
 > +#ifndef HWCAP2_POE
 > +#define HWCAP2_POE (1ULL << 63)
 > +#endif

<asm/hwcap.h> defines this on kernels >= 6.9. Is this fallback needed, 
and if so, why?

 > +#ifndef NT_ARM_POE
 > +#define NT_ARM_POE 0x40f
 > +#endif

Same question. This is in <uapi/linux/elf.h> on recent kernels.

 > +struct user_poe
 > +{
 > +  uint64_t por_el0;
 > +};

Why a struct for a single scalar? A bare uint64_t works fine here.

 > diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
 > @@ -208,6 +208,16 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
 >       return gcs_linux_reg_base != -1;
 >     }
 >
 > +  /* First POE register.  This is -1 if no POE feature is available.  */
 > +  int poe_regnum = -1;

"First POE register" implies there may be more than one. Since there
is exactly one, please use:

   /* Index of the POE register.  This is -1 if POE is not supported.  */

Upstream master now has comments for every other boolean in this
struct. Please add a comment as well.

 > diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
 > @@ -35,6 +35,7 @@ struct aarch64_features
 >       bool pauth = false;
 >       bool mte = false;
 >       bool fpmr = false;
 > +  bool poe = false;

Missing comment. Please add:

   /* Whether the Permission Overlay Extension (FEAT_S1POE) is
      supported.  */

 > diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
 > +/* Fill GDB's register array with the POE register value from the 
current
 > +   thread.  */
 > +
 > +static void
 > +fetch_por_el0_from_thread (regcache *regcache)

The convention for fetch functions in this file is to name them after
the feature or register group, not the specific register:
   fetch_fpmr_from_thread
   fetch_gcsregs_from_thread
   fetch_tlsregs_from_thread

Please rename to fetch_poereg_from_thread.

 > +{
 > +  aarch64_gdbarch_tdep *tdep
 > +      = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
 > +
 > +  gdb_assert (tdep->poe_regnum != -1);

Please use:

   gdb_assert (tdep->has_poe ());

 > +  user_poe user_poe;
 > +  iovec iovec;

Letś not use a variable with the same name as the type.

 > +  int tid = get_ptrace_pid (regcache->ptid ());

get_ptrace_pid here looks correct, as we have the regcache information.

 > diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
 > +/* Convert a POR_EL0 Perm<m> overlay permission encoding into 
rwx-style string.
 > +   For example:
 > +      0b0011 -> "r-x" (3)
 > +      0b0101 -> "rw-" (5)
 > +      0b0111 -> "rwx" (7)
 > +      0b0000 -> "---" (0)
 > +   Reserved encodings (0b1xxx) are returned as "???".  */
 > +
 > +static const char*
 > +aarch64_perm_overlay_decode (unsigned int perm)

Missing space before *.

 > +/* For POE SEGSEGV, show additional information.  */

Typo: SEGSEGV should be SIGSEGV.

 > +  else if (si_code == AARCH64_SEGV_PKUERR && si_errno == 0)
 > +    meaning = _("Protection overlay violation");

The si_errno == 0 condition is not meaningful for SEGV_PKUERR. si_errno
is not used to qualify this signal code, is it?

If so...

   else if (si_code == AARCH64_SEGV_PKUERR)
     meaning = _("Protection overlay violation");

 > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
 > +  if (features.poe)
 > +    regnum = create_feature_aarch64_poe (tdesc.get (), regnum);

Is identation off here?

 > diff --git a/gdbserver/linux-aarch64-low.cc 
b/gdbserver/linux-aarch64-low.cc
 > +static void
 > +aarch64_store_por_el0_regset (struct regcache *regcache, const void 
*buf)
 > +{
 > +  const user_poe *regset = (const user_poe *) buf;
 > +  int poe_regnum  = find_regno (regcache->tdesc, "por_el0");

Single space before =.

 > +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_ARM_POE,
 > +    0, OPTIONAL_REGS,
 > +    nullptr, aarch64_store_por_el0_regset },

Document/Add comment that we don´t support writing for now.

Also document/add comment stating we lack core file support as well.

Reviewed-By: Luis Machado <luis.machado.foss@gmail.com>