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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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 :)."

> > [...]
> >
> > > > 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