1. Patchew
  2. Xen
  3. View series

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

Federico Serafini posted 1 patch 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/6f3538a91f719611e719066237568163ae90c95e.1695827160.git.federico.serafini@bugseng.com
xen/arch/x86/domain_page.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
[XEN PATCH] x86/domain_page: address violations of MISRA C:2012 Rule 8.3
Posted by Federico Serafini 7 months ago 
Make function declarations and definitions consistent.
No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 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);
 }
 
 /* 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)]);
 }
-- 
2.34.1
Re: [XEN PATCH] x86/domain_page: address violations of MISRA C:2012 Rule 8.3
Posted by Jan Beulich 7 months ago 
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@bugseng.com>
> ---
>  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
Re: [XEN PATCH] x86/domain_page: address violations of MISRA C:2012 Rule 8.3
Posted by Federico Serafini 7 months ago 
On 28/09/23 08:25, Jan Beulich wrote:
> 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@bugseng.com>
>> ---
>>   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.)

I thought keeping "va" would be better because of the comments on the
declaration side, which focus on taking a VA as parameter.
However, I will follow your naming conventions.

> 
>>   /* 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
> 

Thanks, I missed this (it is a violation related to Arm 32
that is not under the ECLAIR analysis).

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.