Re: [PATCH v2 2/2] hw/arm/smmu-common: Avoid using inlined functions with external linkage

[Eric Auger]

On 12/16/22 22:49, Philippe Mathieu-Daudé 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, so these are likely oversights rather than intentional.
>
> Reported-by: Stefan Weil <sw@weilnetz.de>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks!

Eric
> ---
>  hw/arm/smmu-common.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 9f196625a2..54186f31cb 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,9 +146,8 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer 
> key, gpointer value,
>             ((entry->iova & ~info->mask) == info->iova);
>  }
>  
> -inline void
> -smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> -                    uint8_t tg, uint64_t num_pages, uint8_t ttl)
> +void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> +                         uint8_t tg, uint64_t num_pages, uint8_t ttl)
>  {
>      /* if tg is not set we use 4KB range invalidation */
>      uint8_t granule = tg ? tg * 2 + 10 : 12;
> @@ -174,7 +173,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,8 +373,8 @@ error:
>   *
>   * return 0 on success
>   */
> -inline int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags 
> perm,
> -                    SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> +             SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>  {
>      if (!cfg->aa64) {
>          /*