| Message ID | 20250108152317.335441-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: ffa: fix build with clang | expand |
Hi Stewart, Thanks a lot for the finding and the fix :-) > On 8 Jan 2025, at 16:23, Stewart Hildebrand <Stewart.Hildebrand@amd.com> wrote: > > Clang 16 reports: > > In file included from arch/arm/tee/ffa.c:72: > arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a non-definition declaration [-Werror,-Wignored-attributes] > extern uint32_t __ro_after_init ffa_fw_version; > ^ > > Remove the attribute from the header to resolve this. The attribute in > ffa.c is the one the matters anyway. > > Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support") > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > It appears the variable ffa_fw_version is only used in ffa.c. Was there > any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: > Fine granular call support")? If not, perhaps we ought to make it static > again and remove the declaration in the header. > --- > xen/arch/arm/tee/ffa_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h > index d441c0ca5598..05368d5a88d3 100644 > --- a/xen/arch/arm/tee/ffa_private.h > +++ b/xen/arch/arm/tee/ffa_private.h > @@ -326,7 +326,7 @@ extern void *ffa_rx; > extern void *ffa_tx; > extern spinlock_t ffa_rx_buffer_lock; > extern spinlock_t ffa_tx_buffer_lock; > -extern uint32_t __ro_after_init ffa_fw_version; > +extern uint32_t ffa_fw_version; > extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); > > bool ffa_shm_domain_destroy(struct domain *d); > > base-commit: 70f5a875becc9444a959830b10a361982c31a366 > -- > 2.47.1 >
Hi, On Wed Jan 8, 2025 at 3:23 PM GMT, Stewart Hildebrand wrote: > Clang 16 reports: > > In file included from arch/arm/tee/ffa.c:72: > arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a non-definition declaration [-Werror,-Wignored-attributes] > extern uint32_t __ro_after_init ffa_fw_version; > ^ > > Remove the attribute from the header to resolve this. The attribute in > ffa.c is the one the matters anyway. > > Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support") > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> > --- > It appears the variable ffa_fw_version is only used in ffa.c. Was there > any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: > Fine granular call support")? If not, perhaps we ought to make it static > again and remove the declaration in the header. The only reason I can think of is wanting to have it in the symbol table of the ELF file for some reason (livepatching?). But that's far fetched at best. > --- > xen/arch/arm/tee/ffa_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h > index d441c0ca5598..05368d5a88d3 100644 > --- a/xen/arch/arm/tee/ffa_private.h > +++ b/xen/arch/arm/tee/ffa_private.h > @@ -326,7 +326,7 @@ extern void *ffa_rx; > extern void *ffa_tx; > extern spinlock_t ffa_rx_buffer_lock; > extern spinlock_t ffa_tx_buffer_lock; > -extern uint32_t __ro_after_init ffa_fw_version; > +extern uint32_t ffa_fw_version; > extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); > > bool ffa_shm_domain_destroy(struct domain *d); > > base-commit: 70f5a875becc9444a959830b10a361982c31a366 IMO, it makes sense to make it static and remove this line altogether as you suggest. If it needs to be exported later on it can be adjusted as needed. Cheers, Alejandro
Hi, > On 8 Jan 2025, at 16:45, Alejandro Vallejo <alejandro.vallejo@cloud.com> wrote: > > Hi, > > On Wed Jan 8, 2025 at 3:23 PM GMT, Stewart Hildebrand wrote: >> Clang 16 reports: >> >> In file included from arch/arm/tee/ffa.c:72: >> arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a non-definition declaration [-Werror,-Wignored-attributes] >> extern uint32_t __ro_after_init ffa_fw_version; >> ^ >> >> Remove the attribute from the header to resolve this. The attribute in >> ffa.c is the one the matters anyway. >> >> Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support") >> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> >> --- >> It appears the variable ffa_fw_version is only used in ffa.c. Was there >> any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: >> Fine granular call support")? If not, perhaps we ought to make it static >> again and remove the declaration in the header. > > The only reason I can think of is wanting to have it in the symbol table of the > ELF file for some reason (livepatching?). But that's far fetched at best. > >> --- >> xen/arch/arm/tee/ffa_private.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >> index d441c0ca5598..05368d5a88d3 100644 >> --- a/xen/arch/arm/tee/ffa_private.h >> +++ b/xen/arch/arm/tee/ffa_private.h >> @@ -326,7 +326,7 @@ extern void *ffa_rx; >> extern void *ffa_tx; >> extern spinlock_t ffa_rx_buffer_lock; >> extern spinlock_t ffa_tx_buffer_lock; >> -extern uint32_t __ro_after_init ffa_fw_version; >> +extern uint32_t ffa_fw_version; >> extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); >> >> bool ffa_shm_domain_destroy(struct domain *d); >> >> base-commit: 70f5a875becc9444a959830b10a361982c31a366 > > IMO, it makes sense to make it static and remove this line altogether as you > suggest. If it needs to be exported later on it can be adjusted as needed. > Yes in fact it was made global as there was a use case at some point but this is not the case anymore so in fact we can completely remove this from the header and make it static for now. @stewart: are you happy to push a patch doing this instead ? Cheers Bertrand > Cheers, > Alejandro
On 1/8/25 11:34, Bertrand Marquis wrote: > Hi, > >> On 8 Jan 2025, at 16:45, Alejandro Vallejo <alejandro.vallejo@cloud.com> wrote: >> >> Hi, >> >> On Wed Jan 8, 2025 at 3:23 PM GMT, Stewart Hildebrand wrote: >>> Clang 16 reports: >>> >>> In file included from arch/arm/tee/ffa.c:72: >>> arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a non-definition declaration [-Werror,-Wignored-attributes] >>> extern uint32_t __ro_after_init ffa_fw_version; >>> ^ >>> >>> Remove the attribute from the header to resolve this. The attribute in >>> ffa.c is the one the matters anyway. >>> >>> Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support") >>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> >>> --- >>> It appears the variable ffa_fw_version is only used in ffa.c. Was there >>> any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: >>> Fine granular call support")? If not, perhaps we ought to make it static >>> again and remove the declaration in the header. >> >> The only reason I can think of is wanting to have it in the symbol table of the >> ELF file for some reason (livepatching?). But that's far fetched at best. >> >>> --- >>> xen/arch/arm/tee/ffa_private.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h >>> index d441c0ca5598..05368d5a88d3 100644 >>> --- a/xen/arch/arm/tee/ffa_private.h >>> +++ b/xen/arch/arm/tee/ffa_private.h >>> @@ -326,7 +326,7 @@ extern void *ffa_rx; >>> extern void *ffa_tx; >>> extern spinlock_t ffa_rx_buffer_lock; >>> extern spinlock_t ffa_tx_buffer_lock; >>> -extern uint32_t __ro_after_init ffa_fw_version; >>> +extern uint32_t ffa_fw_version; >>> extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); >>> >>> bool ffa_shm_domain_destroy(struct domain *d); >>> >>> base-commit: 70f5a875becc9444a959830b10a361982c31a366 >> >> IMO, it makes sense to make it static and remove this line altogether as you >> suggest. If it needs to be exported later on it can be adjusted as needed. >> > > Yes in fact it was made global as there was a use case at some point but this is not > the case anymore so in fact we can completely remove this from the header and make > it static for now. > > @stewart: are you happy to push a patch doing this instead ? Yes
On 08/01/2025 3:45 pm, Alejandro Vallejo wrote: > Hi, > > On Wed Jan 8, 2025 at 3:23 PM GMT, Stewart Hildebrand wrote: >> It appears the variable ffa_fw_version is only used in ffa.c. Was there >> any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: >> Fine granular call support")? If not, perhaps we ought to make it static >> again and remove the declaration in the header. > The only reason I can think of is wanting to have it in the symbol table of the > ELF file for some reason (livepatching?) Livepatching can work with static symbols just fine. Binary diffing is per TU, looking at all symbols in the object, not just global symbols. ~Andrew
diff --git a/xen/arch/arm/tee/ffa_private.h b/xen/arch/arm/tee/ffa_private.h index d441c0ca5598..05368d5a88d3 100644 --- a/xen/arch/arm/tee/ffa_private.h +++ b/xen/arch/arm/tee/ffa_private.h @@ -326,7 +326,7 @@ extern void *ffa_rx; extern void *ffa_tx; extern spinlock_t ffa_rx_buffer_lock; extern spinlock_t ffa_tx_buffer_lock; -extern uint32_t __ro_after_init ffa_fw_version; +extern uint32_t ffa_fw_version; extern DECLARE_BITMAP(ffa_fw_abi_supported, FFA_ABI_BITMAP_SIZE); bool ffa_shm_domain_destroy(struct domain *d);
Clang 16 reports: In file included from arch/arm/tee/ffa.c:72: arch/arm/tee/ffa_private.h:329:17: error: 'used' attribute ignored on a non-definition declaration [-Werror,-Wignored-attributes] extern uint32_t __ro_after_init ffa_fw_version; ^ Remove the attribute from the header to resolve this. The attribute in ffa.c is the one the matters anyway. Fixes: 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support") Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> --- It appears the variable ffa_fw_version is only used in ffa.c. Was there any rationale for exporting the symbol in 2f9f240a5e87 ("xen/arm: ffa: Fine granular call support")? If not, perhaps we ought to make it static again and remove the declaration in the header. --- xen/arch/arm/tee/ffa_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 70f5a875becc9444a959830b10a361982c31a366