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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Jan,

On 06/11/17 12:44, Jan Beulich wrote:
> > > > On 06.11.17 at 13:16, <julien.grall@xxxxxxxxxx> wrote:
> > On 06/11/17 12:11, Jan Beulich wrote:
> > > > > > On 06.11.17 at 12:47, <julien.grall@xxxxxxxxxx> wrote:
> > > > Hi Jan,
> > > >
> > > > On 06/11/17 11:37, Jan Beulich wrote:
> > > > > > > > On 01.11.17 at 15:03, <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel