| Message ID | 20221208161152.28976-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,PATCH-for-8.0] hw: Avoid using inlined functions with external linkage | expand |
On Thu, 8 Dec 2022 at 16:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > When using Clang ("Apple clang version 14.0.0 (clang-1400.0.29.202)") > and building with -Wall we get: > > hw/arm/smmu-common.c:173:33: warning: static function 'smmu_hash_remove_by_asid_iova' is used in an inline function with external linkage [-Wstatic-in-inline] > hw/arm/smmu-common.h:170:1: note: use 'static' to give inline function 'smmu_iotlb_inv_iova' internal linkage > void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > ^ > static > > None of our code base require / use inlined functions with external > linkage. Some places use internal inlining in the hot path. These > two functions are certainly not in any hot path and don't justify > any inlining. > > Reported-by: Stefan Weil <sw@weilnetz.de> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > RFC: Any better justification? I don't really understand what the warning is trying to warn about, and googling didn't enlighten me. Does anybody understand it? In any case, it does seem weird to define a function inline and also have it be defined in a C file rather than as a 'static inline' in a header file, so these are likely oversights rather than intentional. > --- > hw/arm/smmu-common.c | 10 +++++----- > hw/i386/x86.c | 3 +-- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index e09b9c13b7..298e021cd3 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -116,7 +116,7 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new) > g_hash_table_insert(bs->iotlb, key, new); > } > > -inline void smmu_iotlb_inv_all(SMMUState *s) > +void smmu_iotlb_inv_all(SMMUState *s) > { > trace_smmu_iotlb_inv_all(); > g_hash_table_remove_all(s->iotlb); > @@ -146,7 +146,7 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, > ((entry->iova & ~info->mask) == info->iova); > } > > -inline void > +void > smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > uint8_t tg, uint64_t num_pages, uint8_t ttl) While we're changing this, can we put the "void" on the same line as the rest of the function prototype, to match the style of these other functions ? > { > @@ -174,7 +174,7 @@ smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, > &info); > } > > -inline void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid) > +void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid) > { > trace_smmu_iotlb_inv_asid(asid); > g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid); > @@ -374,7 +374,7 @@ error: > * > * return 0 on success > */ > -inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, > +int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, > SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info) This second line now needs re-indenting. > { > if (!cfg->aa64) { > @@ -483,7 +483,7 @@ static void smmu_unmap_notifier_range(IOMMUNotifier *n) > } > > /* Unmap all notifiers attached to @mr */ > -inline void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr) > +void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr) > { > IOMMUNotifier *n; > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 78cc131926..9ac1680180 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -64,8 +64,7 @@ > /* Physical Address of PVH entry point read from kernel ELF NOTE */ > static size_t pvh_start_addr; > > -inline void init_topo_info(X86CPUTopoInfo *topo_info, > - const X86MachineState *x86ms) > +void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms) > { > MachineState *ms = MACHINE(x86ms); This function is not used anywhere outside this file, so we can delete the prototype from include/hw/i386/x86.h and make the function "static void". With those changes, Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 8/12/22 17:27, Peter Maydell wrote: > On Thu, 8 Dec 2022 at 16:11, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> When using Clang ("Apple clang version 14.0.0 (clang-1400.0.29.202)") >> and building with -Wall we get: >> >> hw/arm/smmu-common.c:173:33: warning: static function 'smmu_hash_remove_by_asid_iova' is used in an inline function with external linkage [-Wstatic-in-inline] >> hw/arm/smmu-common.h:170:1: note: use 'static' to give inline function 'smmu_iotlb_inv_iova' internal linkage >> void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, >> ^ >> static >> >> None of our code base require / use inlined functions with external >> linkage. Some places use internal inlining in the hot path. These >> two functions are certainly not in any hot path and don't justify >> any inlining. >> >> Reported-by: Stefan Weil <sw@weilnetz.de> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> RFC: Any better justification? > > I don't really understand what the warning is trying to warn > about, and googling didn't enlighten me. Does anybody understand it? > > In any case, it does seem weird to define a function inline and > also have it be defined in a C file rather than as a 'static inline' > in a header file, so these are likely oversights rather than > intentional. > >> --- >> hw/arm/smmu-common.c | 10 +++++----- >> hw/i386/x86.c | 3 +-- >> 2 files changed, 6 insertions(+), 7 deletions(-) >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index 78cc131926..9ac1680180 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -64,8 +64,7 @@ >> /* Physical Address of PVH entry point read from kernel ELF NOTE */ >> static size_t pvh_start_addr; >> >> -inline void init_topo_info(X86CPUTopoInfo *topo_info, >> - const X86MachineState *x86ms) >> +void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms) >> { >> MachineState *ms = MACHINE(x86ms); > > This function is not used anywhere outside this file, so we > can delete the prototype from include/hw/i386/x86.h and > make the function "static void". Good idea. > With those changes, > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Thanks!
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index e09b9c13b7..298e021cd3 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -116,7 +116,7 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new) g_hash_table_insert(bs->iotlb, key, new); } -inline void smmu_iotlb_inv_all(SMMUState *s) +void smmu_iotlb_inv_all(SMMUState *s) { trace_smmu_iotlb_inv_all(); g_hash_table_remove_all(s->iotlb); @@ -146,7 +146,7 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value, ((entry->iova & ~info->mask) == info->iova); } -inline void +void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, uint8_t tg, uint64_t num_pages, uint8_t ttl) { @@ -174,7 +174,7 @@ smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, &info); } -inline void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid) +void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid) { trace_smmu_iotlb_inv_asid(asid); g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid); @@ -374,7 +374,7 @@ error: * * return 0 on success */ -inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, +int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm, SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info) { if (!cfg->aa64) { @@ -483,7 +483,7 @@ static void smmu_unmap_notifier_range(IOMMUNotifier *n) } /* Unmap all notifiers attached to @mr */ -inline void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr) +void smmu_inv_notifiers_mr(IOMMUMemoryRegion *mr) { IOMMUNotifier *n; diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 78cc131926..9ac1680180 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -64,8 +64,7 @@ /* Physical Address of PVH entry point read from kernel ELF NOTE */ static size_t pvh_start_addr; -inline void init_topo_info(X86CPUTopoInfo *topo_info, - const X86MachineState *x86ms) +void init_topo_info(X86CPUTopoInfo *topo_info, const X86MachineState *x86ms) { MachineState *ms = MACHINE(x86ms);
When using Clang ("Apple clang version 14.0.0 (clang-1400.0.29.202)") and building with -Wall we get: hw/arm/smmu-common.c:173:33: warning: static function 'smmu_hash_remove_by_asid_iova' is used in an inline function with external linkage [-Wstatic-in-inline] hw/arm/smmu-common.h:170:1: note: use 'static' to give inline function 'smmu_iotlb_inv_iova' internal linkage void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova, ^ static None of our code base require / use inlined functions with external linkage. Some places use internal inlining in the hot path. These two functions are certainly not in any hot path and don't justify any inlining. Reported-by: Stefan Weil <sw@weilnetz.de> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- RFC: Any better justification? --- hw/arm/smmu-common.c | 10 +++++----- hw/i386/x86.c | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-)