> -----Original Message----- > From: Julien Grall [mailto:julien.gr...@linaro.org] > Sent: 06 October 2017 12:00 > 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 > > > > 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
