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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.03.2024 11:24, Julien Grall wrote:
> 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.

Hmm, I didn't even think in that direction, seeing that generally we aim
at moving from macros to inline functions. But yes, if converting to a
macro is acceptable, that'll be what I do in v2 then.

Jan