Re: [Xen-devel] [PATCH v4 16/16] xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 03/02/2018 04:08 PM, Jan Beulich wrote:
> > > > On 21.02.18 at 15:02, <julien.grall@xxxxxxx> wrote:
> > --- 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
> > - */
> Why does this comment go away?
From an earlier review, Andrew said:

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

The descriptor one got remove by 634afe43ac "x86/pv: Rename 
invalidate_shadow_ldt() to pv_destroy_ldt()". So it is not part of this
patch anymore.

> > @@ -478,10 +478,10 @@ extern paddr_t mem_hotplug;
> >   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
> >   
> >   #define compat_machine_to_phys_mapping ((unsigned int
> > *)RDWR_COMPAT_MPT_VIRT_START)
> > -#define _set_gpfn_from_mfn(mfn, pfn) ({                        \
> > -    struct domain *d = page_get_owner(__mfn_to_page(mfn));     \
> > -    unsigned long entry = (d && (d == dom_cow)) ?              \
> > -        SHARED_M2P_ENTRY : (pfn);                              \
> > +#define _set_gpfn_from_mfn(mfn, pfn) ({                         \
> > +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));    \
> > +    unsigned long entry = (d && (d == dom_cow)) ?               \
> > +        SHARED_M2P_ENTRY : (pfn);                               \
> Please don't break the alignment of the backslashes here. It also looks
> like three of the four lines could be left alone altogether.
I am not sure why I modified the 3 other lines. I fixed it.

> > @@ -157,10 +157,10 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, 
> > unsigned int flags)
> >   #define l4e_from_intpte(intpte)    ((l4_pgentry_t) { (intpte_t)(intpte) })
> >   
> >   /* Construct a pte from a page pointer and access flags. */
> > -#define l1e_from_page(page, flags) l1e_from_pfn(__page_to_mfn(page), (flags))
> > -#define l2e_from_page(page, flags) l2e_from_pfn(__page_to_mfn(page), (flags))
> > -#define l3e_from_page(page, flags) l3e_from_pfn(__page_to_mfn(page), (flags))
> > -#define l4e_from_page(page, flags) l4e_from_pfn(__page_to_mfn(page), (flags))
> > +#define l1e_from_page(page, flags) l1e_from_mfn(page_to_mfn(page), (flags))
> > +#define l2e_from_page(page, flags) l2e_from_mfn(page_to_mfn(page), (flags))
> > +#define l3e_from_page(page, flags) l3e_from_mfn(page_to_mfn(page), (flags))
> > +#define l4e_from_page(page, flags) l4e_from_mfn(page_to_mfn(page), (flags))
> Would again have been nice if you got rid of the extra parentheses
> here at the same time.
I admit, I don't spend my time trying to find the possible cleanup in 
the x86 code. I just do mechanical change and when I get bored I do a 
bit more.
> > @@ -240,12 +240,12 @@ void copy_page_sse2(void *, const void *);
> >   #define __mfn_to_virt(mfn)  (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> >   
> >   /* Convert between machine frame numbers and page-info structures. */
> > -#define __mfn_to_page(mfn)  (frame_table + pfn_to_pdx(mfn))
> > -#define __page_to_mfn(pg)   pdx_to_pfn((unsigned long)((pg) - frame_table))
> > +#define mfn_to_page(mfn)    (frame_table + mfn_to_pdx(mfn))
> > +#define page_to_mfn(pg)     pdx_to_mfn((unsigned long)((pg) - frame_table))
> >   
> >   /* Convert between machine addresses and page-info structures. */
> > -#define __maddr_to_page(ma) __mfn_to_page((ma) >> PAGE_SHIFT)
> > -#define __page_to_maddr(pg) ((paddr_t)__page_to_mfn(pg) << PAGE_SHIFT)
> > +#define __maddr_to_page(ma) mfn_to_page(maddr_to_mfn(ma))
> > +#define __page_to_maddr(pg) (mfn_to_maddr(page_to_mfn(pg)))
> Same here.
>
> With at least the first two items taken care of, relevant x86 pieces
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
I don't plan to address the first one as Andrew were happy with it.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel