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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan