diff mbox series

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

Message ID 6f3538a91f719611e719066237568163ae90c95e.1695827160.git.federico.serafini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series [XEN] x86/domain_page: address violations of MISRA C:2012 Rule 8.3 | expand

Commit Message

Federico Serafini Sept. 27, 2023, 3:09 p.m. UTC 
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(-)

Comments

Jan Beulich Sept. 28, 2023, 6:25 a.m. UTC | #1 
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
Federico Serafini Sept. 28, 2023, 10:46 a.m. UTC | #2 
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).
diff mbox series

Patch

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)]);
 }