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 14:20, <julien.grall@xxxxxxx> wrote:
> Hi Jan,
> 
> 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.

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?

Jan


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