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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@xxxxxxxxxx]
> Sent: 06 October 2017 11:49
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>;
> George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson
> <Ian.Jackson@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wei.liu2@xxxxxxxxxx>; Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>;
> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Jun Nakajima
> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Gang Wei
> <gang.wei@xxxxxxxxx>; Shane Wang <shane.wang@xxxxxxxxx>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel