| Message ID | 20240729142421.137283-2-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Stack checking on Arm | expand |
On 29.07.2024 16:24, Stewart Hildebrand wrote: > In preparation for using -finstrument-functions option, we need to tag a > few functions that don't work well with such instrumentation. If we > don't intervene, we would end up with linker errors such as undefined > reference to __bad_cmpxchg. I can't spot mention of such a side effect from the documentation. Talk there is of function calls being added at function entry and exit. Nothing is being said that calls to other functions would also be affected. > --- a/xen/arch/arm/include/asm/arm64/cmpxchg.h > +++ b/xen/arch/arm/include/asm/arm64/cmpxchg.h > @@ -5,6 +5,7 @@ > > extern void __bad_xchg(volatile void *ptr, int size); > > +__attribute__((no_instrument_function)) I guess it would be nice to have #define no_instrument __attribute__((no_instrument_function)) in xen/compiler.h. I also don't think these annotations want placing ahead of the entire declaration, but rather where other similar annotations would also go. Jan
On 7/29/24 10:55, Jan Beulich wrote: > On 29.07.2024 16:24, Stewart Hildebrand wrote: >> In preparation for using -finstrument-functions option, we need to tag a >> few functions that don't work well with such instrumentation. If we >> don't intervene, we would end up with linker errors such as undefined >> reference to __bad_cmpxchg. > > I can't spot mention of such a side effect from the documentation. Talk > there is of function calls being added at function entry and exit. > Nothing is being said that calls to other functions would also be > affected. Oddly, it seems the compiler fails remove the implementations of the affected functions from the object files even though they are not called in the generated code (because they were inlined). I can mention this in the commit message. For example, if no_instrument is omitted from __int_cmpxchg, feeding one of the object files through aarch64-none-linux-gnu-objdump -d yields for example: 0000000000000048 <__int_cmpxchg>: <snip> 16c: 94000000 bl 0 <__bad_cmpxchg> <snip> Yet, __int_cmpxchg is not called by anything. Even more oddly, this appears to be the case for both clang and gcc. >> --- a/xen/arch/arm/include/asm/arm64/cmpxchg.h >> +++ b/xen/arch/arm/include/asm/arm64/cmpxchg.h >> @@ -5,6 +5,7 @@ >> >> extern void __bad_xchg(volatile void *ptr, int size); >> >> +__attribute__((no_instrument_function)) > > I guess it would be nice to have > > #define no_instrument __attribute__((no_instrument_function)) > > in xen/compiler.h. I also don't think these annotations want placing > ahead of the entire declaration, but rather where other similar > annotations would also go. Will do, thanks for the suggestion.
On 29.07.2024 20:56, Stewart Hildebrand wrote: > On 7/29/24 10:55, Jan Beulich wrote: >> On 29.07.2024 16:24, Stewart Hildebrand wrote: >>> In preparation for using -finstrument-functions option, we need to tag a >>> few functions that don't work well with such instrumentation. If we >>> don't intervene, we would end up with linker errors such as undefined >>> reference to __bad_cmpxchg. >> >> I can't spot mention of such a side effect from the documentation. Talk >> there is of function calls being added at function entry and exit. >> Nothing is being said that calls to other functions would also be >> affected. > > Oddly, it seems the compiler fails remove the implementations of the > affected functions from the object files even though they are not called > in the generated code (because they were inlined). I can mention this in > the commit message. > > For example, if no_instrument is omitted from __int_cmpxchg, feeding one > of the object files through aarch64-none-linux-gnu-objdump -d yields for > example: > > 0000000000000048 <__int_cmpxchg>: > <snip> > 16c: 94000000 bl 0 <__bad_cmpxchg> > <snip> > > Yet, __int_cmpxchg is not called by anything. > > Even more oddly, this appears to be the case for both clang and gcc. Upon a 2nd, closer look this doesn't appear odd at all. The 1st argument to the two instrumentation functions is a pointer to the function, thus requiring instantiation somewhere. Question then is whether adding the attribute is an appropriate approach to the issue. Gcc doc kind of suggests to possibly use extern inline instead. Jan
diff --git a/xen/arch/arm/include/asm/arm64/cmpxchg.h b/xen/arch/arm/include/asm/arm64/cmpxchg.h index f160e8e7bcf9..acabd42ca986 100644 --- a/xen/arch/arm/include/asm/arm64/cmpxchg.h +++ b/xen/arch/arm/include/asm/arm64/cmpxchg.h @@ -5,6 +5,7 @@ extern void __bad_xchg(volatile void *ptr, int size); +__attribute__((no_instrument_function)) static inline unsigned long __xchg(unsigned long x, volatile void *ptr, int size) { unsigned long ret, tmp; @@ -102,6 +103,7 @@ __CMPXCHG_CASE(w, h, 2) __CMPXCHG_CASE(w, , 4) __CMPXCHG_CASE( , , 8) +__attribute__((no_instrument_function)) static always_inline bool __int_cmpxchg(volatile void *ptr, unsigned long *old, unsigned long new, int size, bool timeout, unsigned int max_try) @@ -122,6 +124,7 @@ static always_inline bool __int_cmpxchg(volatile void *ptr, unsigned long *old, ASSERT_UNREACHABLE(); } +__attribute__((no_instrument_function)) static always_inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old, unsigned long new, @@ -145,6 +148,7 @@ static always_inline unsigned long __cmpxchg(volatile void *ptr, * The helper will return true when the update has succeeded (i.e no * timeout) and false if the update has failed. */ +__attribute__((no_instrument_function)) static always_inline bool __cmpxchg_timeout(volatile void *ptr, unsigned long *old, unsigned long new, diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h index a71d6a295dcc..3090e1cb784b 100644 --- a/xen/arch/arm/include/asm/arm64/sve.h +++ b/xen/arch/arm/include/asm/arm64/sve.h @@ -48,6 +48,7 @@ unsigned int get_sys_vl_len(void); #define opt_dom0_sve 0 +__attribute__((no_instrument_function)) static inline bool is_sve_domain(const struct domain *d) { return false; diff --git a/xen/arch/arm/include/asm/atomic.h b/xen/arch/arm/include/asm/atomic.h index 517216d2a846..31c3b3c3745b 100644 --- a/xen/arch/arm/include/asm/atomic.h +++ b/xen/arch/arm/include/asm/atomic.h @@ -74,6 +74,7 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t) void __bad_atomic_read(const volatile void *p, void *res); void __bad_atomic_size(void); +__attribute__((no_instrument_function)) static always_inline void read_atomic_size(const volatile void *p, void *res, unsigned int size) @@ -99,6 +100,7 @@ static always_inline void read_atomic_size(const volatile void *p, } } +__attribute__((no_instrument_function)) static always_inline void write_atomic_size(volatile void *p, void *val, unsigned int size) diff --git a/xen/arch/arm/include/asm/guest_atomics.h b/xen/arch/arm/include/asm/guest_atomics.h index 8893eb9a55d7..f5f4ba0df0b1 100644 --- a/xen/arch/arm/include/asm/guest_atomics.h +++ b/xen/arch/arm/include/asm/guest_atomics.h @@ -86,6 +86,7 @@ static inline void guest_clear_mask16(struct domain *d, uint16_t mask, domain_unpause(d); } +__attribute__((no_instrument_function)) static always_inline unsigned long __guest_cmpxchg(struct domain *d, volatile void *ptr, unsigned long old, diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 00d2cbebf25a..7b42f8faa61c 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -75,6 +75,7 @@ void __xsm_action_mismatch_detected(void); #endif /* CONFIG_XSM */ +__attribute__((no_instrument_function)) static always_inline int xsm_default_action( xsm_default_t action, struct domain *src, struct domain *target) {
In preparation for using -finstrument-functions option, we need to tag a few functions that don't work well with such instrumentation. If we don't intervene, we would end up with linker errors such as undefined reference to __bad_cmpxchg. Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- xen/arch/arm/include/asm/arm64/cmpxchg.h | 4 ++++ xen/arch/arm/include/asm/arm64/sve.h | 1 + xen/arch/arm/include/asm/atomic.h | 2 ++ xen/arch/arm/include/asm/guest_atomics.h | 1 + xen/include/xsm/dummy.h | 1 + 5 files changed, 9 insertions(+)