Re: [Xen-devel] [PATCH v2 9/9] xen: Convert __page_to_mfn and __mfn_to_page to use typesafe MFN

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 15:48, <julien.grall@xxxxxxx> wrote:
> On 09/10/17 14:40, Jan Beulich wrote:
>>>>> On 09.10.17 at 14:20, <julien.grall@xxxxxxx> wrote:
>>> On 09/10/17 12:42, Jan Beulich wrote:
>>>>>>> On 05.10.17 at 19:42, <julien.grall@xxxxxxxxxx> wrote:
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -50,8 +50,6 @@ struct map_range_data
>>>>>    /* Override macros from asm/page.h to make them work with mfn_t */
>>>>>    #undef virt_to_mfn
>>>>>    #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>>>>> -#undef page_to_mfn
>>>>> -#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
>>>>
>>>> With the patch dropping (I assume) all overrides of this kind, what
>>>> is the difference between the double-underscore-prefixed versions
>>>> of the two constructs you convert here and the plain ones? If
>>>> there's none (which I think is what the result here is meant to be),
>>>> then ideally the patch would drop the former altogether. In case
>>>> this means touching a lot more code, then at least I'd expect you
>>>> to convert all instances you touch anyway, and that you in
>>>> particular don't introduce any new ones.
>>>>
>>>> But wait - the patch even introduces new overrides (doing the
>>>> inverse). What's the deal here? If that's again to limit patch size,
>>>> then I'd still prefer the global aliases to go away, and local (per
>>>> file) aliases to be retained as needed.
>>>
>>> It introduces new overrides because some of the code is not trivial to
>>> convert to use mfn_t. It needs more effort to see whether it is worth it
>>> to convert them and I think is out of scope of this series.
>>>
>>> This series is meant to reduce the number of place we override
>>> page_to_mfn to an handful numbers whilst still enforcing the use of
>>> mfn_t by default.
>>>
>>> But I am not entirely sure what you are suggesting here. Are you saying
>>> we could define page_to_mfn/mfn_to_page on every file?
>> 
>> Actually the other way around: Globally only page_to_mfn() and
>> mfn_to_page() should remain. In files that need them
>> __page_to_mfn() and __mfn_to_page() could be added to limit
>> the scope of the change / the size of the patch.
> 
> I am still unsure to follow your suggestion here. If you define 
> __page_to_mfn() in the code, then you would have to do renaming in the 
> file not converting to use mfn_t. Therefore increasing size of the patch...
> 
>> 
>> But first and foremost I would have wished that other than for
>> defining these overrides, the patch wouldn't leave around
>> __mfn_to_page() uses (which it does in a few x86 headers). But
>> then again maybe it's unavoidable to leave those in place until
>> full conversion has happened?
> 
> You have to keep __mfn_to_page() in x86 headers because some files may 
> override mfn_to_page(). So it is not possible to use the latter safely.
> 
> We could get rid of them once the hypervisor has fully switched to mfn_t.

Okay, never mind then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel