diff mbox series

[PATCH-for-8.0,v3] target/arm: Fix non-TCG build failure by inlining pauth_ptr_mask()

Message ID 20230328212516.29592-1-philmd@linaro.org
State New
Headers show
Series [PATCH-for-8.0,v3] target/arm: Fix non-TCG build failure by inlining pauth_ptr_mask() | expand

Commit Message

Philippe Mathieu-Daudé March 28, 2023, 9:25 p.m. UTC
aarch64_gdb_get_pauth_reg() -- although disabled since commit
5787d17a42 ("target/arm: Don't advertise aarch64-pauth.xml to
gdb") is still compiled in. It calls pauth_ptr_mask() which is
located in target/arm/tcg/pauth_helper.c, a TCG specific helper.

To avoid a linking error when TCG is not enabled:

  Undefined symbols for architecture arm64:
    "_pauth_ptr_mask", referenced from:
        _aarch64_gdb_get_pauth_reg in target_arm_gdbstub64.c.o
  ld: symbol(s) not found for architecture arm64
  clang: error: linker command failed with exit code 1 (use -v to see invocation)

- Inline pauth_ptr_mask() in aarch64_gdb_get_pauth_reg()
  (this is the single user),
- Rename pauth_ptr_mask_internal() as pauth_ptr_mask() and
  inline it in "internals.h",

Fixes: e995d5cce4 ("target/arm: Implement gdbstub pauth extension")
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Supersedes: <20230328133054.6553-1-philmd@linaro.org>

Since v2:
- Rebased (patch #1 merged)
- Addressed rth's comments
- Added R-b tags
---
 target/arm/internals.h        | 16 +++++++---------
 target/arm/gdbstub64.c        |  7 +++++--
 target/arm/tcg/pauth_helper.c | 18 +-----------------
 3 files changed, 13 insertions(+), 28 deletions(-)

Comments

Peter Maydell March 30, 2023, 10:31 a.m. UTC | #1
On Tue, 28 Mar 2023 at 22:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> aarch64_gdb_get_pauth_reg() -- although disabled since commit
> 5787d17a42 ("target/arm: Don't advertise aarch64-pauth.xml to
> gdb") is still compiled in. It calls pauth_ptr_mask() which is
> located in target/arm/tcg/pauth_helper.c, a TCG specific helper.
>
> To avoid a linking error when TCG is not enabled:
>
>   Undefined symbols for architecture arm64:
>     "_pauth_ptr_mask", referenced from:
>         _aarch64_gdb_get_pauth_reg in target_arm_gdbstub64.c.o
>   ld: symbol(s) not found for architecture arm64
>   clang: error: linker command failed with exit code 1 (use -v to see invocation)
>
> - Inline pauth_ptr_mask() in aarch64_gdb_get_pauth_reg()
>   (this is the single user),
> - Rename pauth_ptr_mask_internal() as pauth_ptr_mask() and
>   inline it in "internals.h",
>
> Fixes: e995d5cce4 ("target/arm: Implement gdbstub pauth extension")
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Supersedes: <20230328133054.6553-1-philmd@linaro.org>
>
> Since v2:
> - Rebased (patch #1 merged)
> - Addressed rth's comments
> - Added R-b tags
> ---
>  target/arm/internals.h        | 16 +++++++---------
>  target/arm/gdbstub64.c        |  7 +++++--
>  target/arm/tcg/pauth_helper.c | 18 +-----------------
>  3 files changed, 13 insertions(+), 28 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 673519a24a..71f4c6d8a2 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1389,15 +1389,13 @@ int exception_target_el(CPUARMState *env);
>  bool arm_singlestep_active(CPUARMState *env);
>  bool arm_generate_debug_exceptions(CPUARMState *env);
>
> -/**
> - * pauth_ptr_mask:
> - * @env: cpu context
> - * @ptr: selects between TTBR0 and TTBR1
> - * @data: selects between TBI and TBID
> - *
> - * Return a mask of the bits of @ptr that contain the authentication code.
> - */
> -uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data);
> +static inline uint64_t pauth_ptr_mask(ARMVAParameters param)
> +{
> +    int bot_pac_bit = 64 - param.tsz;
> +    int top_pac_bit = 64 - 8 * param.tbi;
> +
> +    return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
> +}

Any reason for deleting the doc comment ?

thanks
-- PMM
Peter Maydell March 30, 2023, 10:34 a.m. UTC | #2
On Thu, 30 Mar 2023 at 11:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 28 Mar 2023 at 22:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > aarch64_gdb_get_pauth_reg() -- although disabled since commit
> > 5787d17a42 ("target/arm: Don't advertise aarch64-pauth.xml to
> > gdb") is still compiled in. It calls pauth_ptr_mask() which is
> > located in target/arm/tcg/pauth_helper.c, a TCG specific helper.
> >
> > To avoid a linking error when TCG is not enabled:
> >
> >   Undefined symbols for architecture arm64:
> >     "_pauth_ptr_mask", referenced from:
> >         _aarch64_gdb_get_pauth_reg in target_arm_gdbstub64.c.o
> >   ld: symbol(s) not found for architecture arm64
> >   clang: error: linker command failed with exit code 1 (use -v to see invocation)
> >
> > - Inline pauth_ptr_mask() in aarch64_gdb_get_pauth_reg()
> >   (this is the single user),
> > - Rename pauth_ptr_mask_internal() as pauth_ptr_mask() and
> >   inline it in "internals.h",
> >
> > Fixes: e995d5cce4 ("target/arm: Implement gdbstub pauth extension")
> > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > Supersedes: <20230328133054.6553-1-philmd@linaro.org>
> >
> > Since v2:
> > - Rebased (patch #1 merged)
> > - Addressed rth's comments
> > - Added R-b tags
> > ---
> >  target/arm/internals.h        | 16 +++++++---------
> >  target/arm/gdbstub64.c        |  7 +++++--
> >  target/arm/tcg/pauth_helper.c | 18 +-----------------
> >  3 files changed, 13 insertions(+), 28 deletions(-)
> >
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index 673519a24a..71f4c6d8a2 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1389,15 +1389,13 @@ int exception_target_el(CPUARMState *env);
> >  bool arm_singlestep_active(CPUARMState *env);
> >  bool arm_generate_debug_exceptions(CPUARMState *env);
> >
> > -/**
> > - * pauth_ptr_mask:
> > - * @env: cpu context
> > - * @ptr: selects between TTBR0 and TTBR1
> > - * @data: selects between TBI and TBID
> > - *
> > - * Return a mask of the bits of @ptr that contain the authentication code.
> > - */
> > -uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data);
> > +static inline uint64_t pauth_ptr_mask(ARMVAParameters param)
> > +{
> > +    int bot_pac_bit = 64 - param.tsz;
> > +    int top_pac_bit = 64 - 8 * param.tbi;
> > +
> > +    return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
> > +}
>
> Any reason for deleting the doc comment ?

Applied to target-arm.next with a doc comment:

/**
 * pauth_ptr_mask:
 * @param: parameters defining the MMU setup
 *
 * Return a mask of the address bits that contain the authentication code,
 * given the MMU config defined by @param.
 */


-- PMM
Philippe Mathieu-Daudé March 30, 2023, 1 p.m. UTC | #3
On 30/3/23 12:34, Peter Maydell wrote:
> On Thu, 30 Mar 2023 at 11:31, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 28 Mar 2023 at 22:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>
>>> aarch64_gdb_get_pauth_reg() -- although disabled since commit
>>> 5787d17a42 ("target/arm: Don't advertise aarch64-pauth.xml to
>>> gdb") is still compiled in. It calls pauth_ptr_mask() which is
>>> located in target/arm/tcg/pauth_helper.c, a TCG specific helper.
>>>
>>> To avoid a linking error when TCG is not enabled:
>>>
>>>    Undefined symbols for architecture arm64:
>>>      "_pauth_ptr_mask", referenced from:
>>>          _aarch64_gdb_get_pauth_reg in target_arm_gdbstub64.c.o
>>>    ld: symbol(s) not found for architecture arm64
>>>    clang: error: linker command failed with exit code 1 (use -v to see invocation)
>>>
>>> - Inline pauth_ptr_mask() in aarch64_gdb_get_pauth_reg()
>>>    (this is the single user),
>>> - Rename pauth_ptr_mask_internal() as pauth_ptr_mask() and
>>>    inline it in "internals.h",
>>>
>>> Fixes: e995d5cce4 ("target/arm: Implement gdbstub pauth extension")
>>> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>>> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> Supersedes: <20230328133054.6553-1-philmd@linaro.org>
>>>
>>> Since v2:
>>> - Rebased (patch #1 merged)
>>> - Addressed rth's comments
>>> - Added R-b tags
>>> ---
>>>   target/arm/internals.h        | 16 +++++++---------
>>>   target/arm/gdbstub64.c        |  7 +++++--
>>>   target/arm/tcg/pauth_helper.c | 18 +-----------------
>>>   3 files changed, 13 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>> index 673519a24a..71f4c6d8a2 100644
>>> --- a/target/arm/internals.h
>>> +++ b/target/arm/internals.h
>>> @@ -1389,15 +1389,13 @@ int exception_target_el(CPUARMState *env);
>>>   bool arm_singlestep_active(CPUARMState *env);
>>>   bool arm_generate_debug_exceptions(CPUARMState *env);
>>>
>>> -/**
>>> - * pauth_ptr_mask:
>>> - * @env: cpu context
>>> - * @ptr: selects between TTBR0 and TTBR1
>>> - * @data: selects between TBI and TBID
>>> - *
>>> - * Return a mask of the bits of @ptr that contain the authentication code.
>>> - */
>>> -uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data);
>>> +static inline uint64_t pauth_ptr_mask(ARMVAParameters param)
>>> +{
>>> +    int bot_pac_bit = 64 - param.tsz;
>>> +    int top_pac_bit = 64 - 8 * param.tbi;
>>> +
>>> +    return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
>>> +}
>>
>> Any reason for deleting the doc comment ?
> 
> Applied to target-arm.next with a doc comment:
> 
> /**
>   * pauth_ptr_mask:
>   * @param: parameters defining the MMU setup
>   *
>   * Return a mask of the address bits that contain the authentication code,
>   * given the MMU config defined by @param.
>   */

Thank you!
diff mbox series

Patch

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 673519a24a..71f4c6d8a2 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1389,15 +1389,13 @@  int exception_target_el(CPUARMState *env);
 bool arm_singlestep_active(CPUARMState *env);
 bool arm_generate_debug_exceptions(CPUARMState *env);
 
-/**
- * pauth_ptr_mask:
- * @env: cpu context
- * @ptr: selects between TTBR0 and TTBR1
- * @data: selects between TBI and TBID
- *
- * Return a mask of the bits of @ptr that contain the authentication code.
- */
-uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data);
+static inline uint64_t pauth_ptr_mask(ARMVAParameters param)
+{
+    int bot_pac_bit = 64 - param.tsz;
+    int top_pac_bit = 64 - 8 * param.tbi;
+
+    return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
+}
 
 /* Add the cpreg definitions for debug related system registers */
 void define_debug_regs(ARMCPU *cpu);
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index ec1e07f139..c1f7e8c934 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -230,8 +230,11 @@  int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg)
         {
             bool is_data = !(reg & 1);
             bool is_high = reg & 2;
-            uint64_t mask = pauth_ptr_mask(env, -is_high, is_data);
-            return gdb_get_reg64(buf, mask);
+            ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
+            ARMVAParameters param;
+
+            param = aa64_va_parameters(env, -is_high, mmu_idx, is_data);
+            return gdb_get_reg64(buf, pauth_ptr_mask(param));
         }
     default:
         return 0;
diff --git a/target/arm/tcg/pauth_helper.c b/target/arm/tcg/pauth_helper.c
index 20f347332d..de067fa716 100644
--- a/target/arm/tcg/pauth_helper.c
+++ b/target/arm/tcg/pauth_helper.c
@@ -339,17 +339,9 @@  static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
     return pac | ext | ptr;
 }
 
-static uint64_t pauth_ptr_mask_internal(ARMVAParameters param)
-{
-    int bot_pac_bit = 64 - param.tsz;
-    int top_pac_bit = 64 - 8 * param.tbi;
-
-    return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
-}
-
 static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
 {
-    uint64_t mask = pauth_ptr_mask_internal(param);
+    uint64_t mask = pauth_ptr_mask(param);
 
     /* Note that bit 55 is used whether or not the regime has 2 ranges. */
     if (extract64(ptr, 55, 1)) {
@@ -359,14 +351,6 @@  static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
     }
 }
 
-uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data)
-{
-    ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
-    ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
-
-    return pauth_ptr_mask_internal(param);
-}
-
 static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
                            ARMPACKey *key, bool data, int keynumber)
 {