Re: [PATCH v3 for-next 0/4] xen: Convert __page_to_mfn and _mfn_to_page to use typesafe MFN

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 06/11/17 12:44, Jan Beulich wrote:
>>>> On 06.11.17 at 13:16, <julien.grall@linaro.org> wrote:
> 
>>
>> On 06/11/17 12:11, Jan Beulich wrote:
>>>>>> On 06.11.17 at 12:47, <julien.grall@linaro.org> wrote:
>>>> Hi Jan,
>>>>
>>>> On 06/11/17 11:37, Jan Beulich wrote:
>>>>>>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote:
>>>>>> Most of the users of page_to_mfn and mfn_to_page are either overriding
>>>>>> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest
>>>>>> of the function use mfn_t.
>>>>>>
>>>>>> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe
>>>>>> MFN.
>>>>>
>>>>> I have to admit that I still find the overall goal confusing: Afaict
>>>>> the double-underscore-prefixed versions exist only to allow
>>>>> easily overriding the non-prefixed ones. Hence the first and
>>>>> foremost goal ought to be to convert everyone to using the
>>>>> non-prefixed versions. Files wanting to avoid the typed forms
>>>>> could then continue to use / be switched to the prefixed ones.
>>>>>
>>>>> What you're doing here is producing a mess: The prefixed
>>>>> versions should never have been touched in the first place.
>>>>> And iirc this was discussed before, with the suggestion to use
>>>>> overrides (for the non-prefixed versions) to limit overall patch
>>>>> size.
>>>>
>>>> At the end of the discussion in the previous version, you were happy
>>>> with the modification done here (see [1]).
>>>
>>>   From the angle looked at it back then I indeed was, but I'm looking
>>> at this from a slightly different angle now with the reply above.
>>>
>>>> Overall, I think this is an improvement compare to what we have today.
>>>> Because we enforce the use of MFN typesafe by default. The developer
>>>> would have to override the helpers if he wants to to use the
>>>> non-typesafe version.
>>>>
>>>> With your suggestion here, you would just keep the override around even
>>>> when they are not necessary. They will have to be dropped at some point,
>>>> so why not now?
>>>
>>> Why would we keep the overrides in place once no longer needed?
>>> All I'm asking for is that the double-underscore prefixed macros be
>>> left alone, and the non-prefixed versions be converted to be
>>> type-safe by default (with the option to override). That'll allow the
>>> prefixed variants to go way altogether once all code was switched
>>> to typesafe, no longer requiring any overrides.
>>
>> If you left the double-underscore alone, then you can't convert headers
>> using them to typesafe. This is because they can't use the non-prefixed
>> version as they may be override.
>>
>> So what you are suggesting here is will avoid converting those headers
>> until someone step up and finish to convert all the source to MFN
>> typesafe. Personally, I find quite silly to have to delay that...
> 
> Hmm, I see your point, but if we went the route you suggest,
> what would be the steps to reach the ultimate result I've
> described (the prefixed variants gone)? I seems to me that
> this would require touching a lot of code a second time that is
> being touched now, or is going to be touched as further
> conversion happens.

We would indeed need to modify the headers to use 
page_to_mfn/mfn_to_page instead of __page_to_mfn/__mfn_to_page. Those 
headers are:
	- asm-arm/mm.h
	- xen/domain_page.h
	- asm-x86/mm.h
	- asm-x86/page.h
	- asm-x86/p2m.h

I had a look at the files that needs to convert. It seems there are few 
files with page_to_mfn/mfn_to_page re-defined but no callers:
	- arch/x86/mm/hap/nested_hap.c
	- arch/x86/mm/p2m-pt.c
	- arch/x86/pv/traps.c
	- arch/x86/pv/mm.c
	- arch/x86/pv/iret.c

Those can be fixed now. That leave the following files:
	- arch/x86/mm/p2m-ept.c
		In that file, the override prevents all the caller to use the 
construction mfn_to_page(_mfn(...)) as it mostly deals with hardware.
	- arch/x86/x86_64/mm.c
		This is could be done without too much trouble.
	- arch/x86/pv/dom0_build.c
		It would need a bit of rework to get the code MFN typesafe in a neat way.
	- drivers/passthrough/amd/iommu_map.c
		It would need a bit of rework to get the code MFN typesafe in a neat way.
	- common/trace.c
		I think it should be ok. Though I am bit surprised to see uint32_t 
been used for MFN...
	- common/memory.c
		It would need a bit of rework to get the code MFN typesafe in a neat way.
	- common/page_alloc.c
		This is could be done without too much trouble.
	- common/grant_table.c
		It would need a bit of rework to get the code MFN typesafe in a neat way.

Cheers,

-- 
Julien Grall

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