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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Paul,

On 06/10/17 10:11, Paul Durrant wrote:
> > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > index 0410b1e86b..1e7a0c6c40 100644
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -38,12 +38,6 @@ static unsigned int __read_mostly max_vmid =
> > MAX_VMID_8_BIT;
> >
> >   #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
> >
> > -/* Override macros from asm/mm.h to make them work with mfn_t */
> > -#undef mfn_to_page
> > -#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> > -#undef page_to_mfn
> > -#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
> > -
> >   unsigned int __read_mostly p2m_ipa_bits;
> >
> >   /* Helpers to lookup the properties of each level */
> > @@ -98,7 +92,7 @@ void dump_p2m_lookup(struct domain *d, paddr_t
> > addr)
> >       printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
> >
> >       printk("P2M @ %p mfn:0x%lx\n",
> > -           p2m->root, __page_to_mfn(p2m->root));
> > +           p2m->root, mfn_x(page_to_mfn(p2m->root)));
> The format specifier should really be using PRI_mfn now. Same goes for others 
> below.
Similarly we could do much more clean-up in each chunk. So where do I 
stop? That's why I wrote down in this comment I will not handle all the 
clean-up...
> > diff --git a/xen/arch/x86/pv/descriptor-tables.c
> > b/xen/arch/x86/pv/descriptor-tables.c
> > index 81973af124..f2b20f9910 100644
> > --- a/xen/arch/x86/pv/descriptor-tables.c
> > +++ b/xen/arch/x86/pv/descriptor-tables.c
> > @@ -25,16 +25,6 @@
> >   #include <asm/p2m.h>
> >   #include <asm/pv/mm.h>
> >
> > -/* Override macros from asm/page.h to make them work with mfn_t */
> > -#undef mfn_to_page
> > -#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> > -#undef page_to_mfn
> > -#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
> > -
> > -/*******************
> > - * Descriptor Tables
> > - */
> > -
> Is the comment wrong?
[...]

> > diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
> > op.c
> > index dd90713acf..9ccbd021ef 100644
> > --- a/xen/arch/x86/pv/emul-priv-op.c
> > +++ b/xen/arch/x86/pv/emul-priv-op.c
> > @@ -43,16 +43,6 @@
> >   #include "emulate.h"
> >   #include "mm.h"
> >
> > -/* Override macros from asm/page.h to make them work with mfn_t */
> > -#undef mfn_to_page
> > -#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> > -#undef page_to_mfn
> > -#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
> > -
> > -/***********************
> > - * I/O emulation support
> > - */
> > -
> What's wrong with the comment?
The file is dedicated to I/O emulation support as said in the header and 
the name. I can understand why it was there given there was macros 
defined not related to I/O. Now they are dropped, why would you need a 
comment to separate includes and the code?
[...]

> > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> > index 86506f3747..b85394d1f9 100644
> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -811,7 +811,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
> >
> >               gdprintk(XENLOG_WARNING,
> >                        "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
> > -                     gmfn, page ? page_to_mfn(page) : -1UL, base);
> > +                     gmfn, page ? mfn_x(page_to_mfn(page)) : -1UL, base);
> Would this not be better as mfn_x(page ? page_to_mfn(page) : INVALID_MFN), as 
> you have done elsewhere?
See above.

--
Julien Grall

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