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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@linaro.org]
> Sent: 06 October 2017 12:00
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xen.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>;
> Wei Liu <wei.liu2@citrix.com>; Razvan Cojocaru
> <rcojocaru@bitdefender.com>; Tamas K Lengyel <tamas@tklengyel.com>;
> Boris Ostrovsky <boris.ostrovsky@oracle.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>; Gang Wei
> <gang.wei@intel.com>; Shane Wang <shane.wang@intel.com>
> Subject: Re: [PATCH v2 9/9] xen: Convert __page_to_mfn and
> __mfn_to_page to use typesafe MFN
> 
> 
> 
> On 06/10/17 11:55, Paul Durrant wrote:
> >>>> 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.
> 
> Please get agree with Andrew... Here his comment on the previous version:
> 
> "If you're making this change, please take out the Descriptor Tables
> comment like you do with I/O below, because the entire file is dedicated
> to descriptor table support and it will save me one item on a cleanup
> patch :)."

Ok, if Andrew is planning cleanup anyway and has specifically requested this then I withdraw my objection, since the end result will be the same.

  Paul

> 
> >
> >> [...]
> >>
> >>>> 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?
> 
> I will see what I can do when I will have time to spend on clean-up...
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel