> -----Original Message----- > From: Julien Grall [mailto:julien.gr...@linaro.org] > Sent: 06 October 2017 11:49 > To: Paul Durrant <paul.durr...@citrix.com>; xen-devel@lists.xen.org > Cc: Stefano Stabellini <sstabell...@kernel.org>; Julien Grall > <julien.gr...@arm.com>; Andrew Cooper <andrew.coop...@citrix.com>; > George Dunlap <george.dun...@citrix.com>; Ian Jackson > <ian.jack...@citrix.com>; Jan Beulich <jbeul...@suse.com>; Konrad > Rzeszutek Wilk <konrad.w...@oracle.com>; Tim (Xen.org) <t...@xen.org>; > Wei Liu <wei.l...@citrix.com>; Razvan Cojocaru > <rcojoc...@bitdefender.com>; Tamas K Lengyel <ta...@tklengyel.com>; > Boris Ostrovsky <boris.ostrov...@oracle.com>; Suravee Suthikulpanit > <suravee.suthikulpa...@amd.com>; Jun Nakajima > <jun.nakaj...@intel.com>; Kevin Tian <kevin.t...@intel.com>; Gang Wei > <gang....@intel.com>; Shane Wang <shane.w...@intel.com> > Subject: Re: [PATCH v2 9/9] xen: Convert __page_to_mfn and > __mfn_to_page to use typesafe MFN > > 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... >
I realise you can't fix it all, but to me it seems reasonable to fix format specifiers in the calls you modify. > >> 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? > It makes the hunk look odd though. I think you should leave the comment alone in this patch, even if you do think it superfluous. > [...] > > >> 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. And again, you are modifying the code so why not modify it such that it is coded appropriately, as you have in other places in this patch? Paul > > -- > Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
