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 12:00
> 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
> 
> 
> 
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel