Re: [PATCH] x86+Arm: drop (rename) __virt_to_maddr() / __maddr_to_virt()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 06/03/2024 09:59, Jan Beulich wrote:
> On 06.03.2024 10:44, Julien Grall wrote:
> > On 06/03/2024 07:22, Jan Beulich wrote:
> > > On 05.03.2024 20:24, Julien Grall wrote:
> > > > The title is quite confusing. I would have expected the macro...
> > > >
> > > > On 05/03/2024 08:33, Jan Beulich wrote:
> > > > > There's no use of them anymore except in the definitions of the non-
> > > > > underscore-prefixed aliases. Rename the inline functions, adjust the
> > > > > virt_to_maddr() #define-e, and purge the (x86-only) maddr_to_virt() one,
> > > > > thus eliminating a bogus cast which would have allowed the passing of a
> > > > > pointer type variable into maddr_to_virt() to go silently.
> > > > >
> > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > >
> > > > > --- a/xen/arch/arm/include/asm/mm.h
> > > > > +++ b/xen/arch/arm/include/asm/mm.h
> > > > > @@ -256,12 +256,12 @@ static inline void __iomem *ioremap_wc(p
> > > > >     /* Page-align address and convert to frame number format */
> > > > >     #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
> > > > >     
> > > > > -static inline paddr_t __virt_to_maddr(vaddr_t va)
> > > > > +static inline paddr_t virt_to_maddr(vaddr_t va)
> > > > >     {
> > > > >         uint64_t par = va_to_par(va);
> > > > >         return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
> > > > >     }
> > > > > -#define virt_to_maddr(va)   __virt_to_maddr((vaddr_t)(va))
> > > > > +#define virt_to_maddr(va) virt_to_maddr((vaddr_t)(va))
> > > > ... to be removed. But you keep it and just overload the name. I know it
> > > > is not possible to remove the macro because some callers are using
> > > > pointers (?).
> > > Indeed. I actually tried without, but the build fails miserably.
> > >
> > > > So I would rather prefer if we keep the name distinct on Arm.
> > > >
> > > > Let see what the other Arm maintainers think.
> > > Well, okay. I'm a little surprised though; I was under the impression
> > > that a goal would be to, eventually, get rid of all the __-prefixed
> > > secondary variants of this kind of helpers.
> > Because of MISRA? If so, you would be replacing one violation by another
> > one (duplicated name). IIRC we decided to deviate it, yet I don't
> > particular want to use the pattern in Arm headers when there is no need.
> >
> > If you are trying to solve MISRA, then I think we want to either remove
> > the macro (not possible here) or suffix with the double-underscore the
> > static inline helper.
> No, Misra is only secondary here. Many of these helpers come in two flavors
> such than one can be overridden in individual source files. That's mainly
> connected to type-safety being generally wanted, but not always easy to
> achieve without a lot of code churn. We've made quite a bit of progress
> there, and imo ultimately the need for two flavors of doing the same thing
> ought to disappear.
What about converting the static inline to a macro? As we cast 'va', I 
don't think we have any benefits to keep the static inline helper and 
provide a thin wrapper with just a cast.
This would address my concern and possibly address your wish to remove 
the double-underscore version.
Cheers,


--
Julien Grall