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

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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@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.)
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)