Re: [RFC PATCH-for-8.0] hw: Avoid using inlined functions with external linkage

[Peter Maydell]

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