Re: [XEN PATCH] x86/domain_page: address violations of MISRA C:2012 Rule 8.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2023 17:09, Federico Serafini wrote:
> Make function declarations and definitions consistent.
> No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> ---
>  xen/arch/x86/domain_page.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
> index eac5e3304f..1cfa992a02 100644
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -173,18 +173,18 @@ void *map_domain_page(mfn_t mfn)
>      return (void *)MAPCACHE_VIRT_START + pfn_to_paddr(idx);
>  }
>  
> -void unmap_domain_page(const void *ptr)
> +void unmap_domain_page(const void *va)
>  {
>      unsigned int idx;
>      struct vcpu *v;
>      struct mapcache_domain *dcache;
> -    unsigned long va = (unsigned long)ptr, mfn, flags;
> +    unsigned long addr = (unsigned long)va, mfn, flags;
>      struct vcpu_maphash_entry *hashent;
>  
> -    if ( !va || va >= DIRECTMAP_VIRT_START )
> +    if ( !addr || addr >= DIRECTMAP_VIRT_START )
>          return;
>  
> -    ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
> +    ASSERT(addr >= MAPCACHE_VIRT_START && addr < MAPCACHE_VIRT_END);
>  
>      v = mapcache_current_vcpu();
>      ASSERT(v && is_pv_vcpu(v));
> @@ -192,7 +192,7 @@ void unmap_domain_page(const void *ptr)
>      dcache = &v->domain->arch.pv.mapcache;
>      ASSERT(dcache->inuse);
>  
> -    idx = PFN_DOWN(va - MAPCACHE_VIRT_START);
> +    idx = PFN_DOWN(addr - MAPCACHE_VIRT_START);
>      mfn = l1e_get_pfn(MAPCACHE_L1ENT(idx));
>      hashent = &v->arch.pv.mapcache.hash[MAPHASH_HASHFN(mfn)];
>  
> @@ -317,30 +317,30 @@ void *map_domain_page_global(mfn_t mfn)
>      return vmap(&mfn, 1);
>  }
>  
> -void unmap_domain_page_global(const void *ptr)
> +void unmap_domain_page_global(const void *va)
>  {
> -    unsigned long va = (unsigned long)ptr;
> +    unsigned long addr = (unsigned long)va;
>  
> -    if ( va >= DIRECTMAP_VIRT_START )
> +    if ( addr >= DIRECTMAP_VIRT_START )
>          return;
>  
> -    ASSERT(va >= VMAP_VIRT_START && va < VMAP_VIRT_END);
> +    ASSERT(addr >= VMAP_VIRT_START && addr < VMAP_VIRT_END);
>  
> -    vunmap(ptr);
> +    vunmap(va);
>  }

Up to here a statement in the description is needed why this apparently
much heavier code churn (compared to changing the declaration) is still
the (likely) better approach. (It may still be worthwhile to consider
changing declaration and Arm code, for "ptr" being the imo better name
for a const void * parameter, and "va" being more specific than just
"addr" as a local variable.)

>  /* Translate a map-domain-page'd address to the underlying MFN */
> -mfn_t domain_page_map_to_mfn(const void *ptr)
> +mfn_t domain_page_map_to_mfn(const void *va)
>  {
> -    unsigned long va = (unsigned long)ptr;
> +    unsigned long addr = (unsigned long)va;
>  
> -    if ( va >= DIRECTMAP_VIRT_START )
> -        return _mfn(virt_to_mfn(ptr));
> +    if ( addr >= DIRECTMAP_VIRT_START )
> +        return _mfn(virt_to_mfn(va));
>  
> -    if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
> -        return vmap_to_mfn(va);
> +    if ( addr >= VMAP_VIRT_START && addr < VMAP_VIRT_END )
> +        return vmap_to_mfn(addr);
>  
> -    ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
> +    ASSERT(addr >= MAPCACHE_VIRT_START && addr < MAPCACHE_VIRT_END);
>  
> -    return l1e_get_mfn(__linear_l1_table[l1_linear_offset(va)]);
> +    return l1e_get_mfn(__linear_l1_table[l1_linear_offset(addr)]);
>  }

This change, otoh, moves the violation from x86 to Arm, afaict. IOW
this likely wants taking care of by changing the declaration. Then,
for consistency, the consideration above gains one more supporting
factor.

Jan