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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 09/10/17 14:40, Jan Beulich wrote:
> > > > 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.
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.

Cheers,

--
Julien Grall

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