RE: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

["Lengyel, Tamas" <tamas.lengyel@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: Tuesday, September 22, 2020 3:00 PM
> To: Julien Grall <julien@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Lengyel, Tamas <tamas.lengyel@xxxxxxxxx>
> Subject: Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers
> when !CONFIG_HAVE_M2P
> 
> On 22/09/2020 19:39, Julien Grall wrote:
> > Hi Jan,
> >
> > On 22/09/2020 09:02, Jan Beulich wrote:
> >> On 21.09.2020 20:02, Julien Grall wrote:
> >>> --- a/xen/include/xen/mm.h
> >>> +++ b/xen/include/xen/mm.h
> >>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct
> >>> page_info *page)
> >>>       }
> >>>   }
> >>>   +/*
> >>> + * Dummy implementation of M2P-related helpers for common code
> when
> >>> + * the architecture doesn't have an M2P.
> >>> + */
> >>> +#ifndef CONFIG_HAS_M2P
> >>> +
> >>> +#define INVALID_M2P_ENTRY        (~0UL) #define SHARED_M2P(_e)
> >>> +false
> >>> +
> >>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
> >>> long pfn) {}
> >>
> >> While I think this would better BUG() or at least
> >> ASSERT_UNREACHABLE(), I realize its use in page_alloc.c prevents
> >> this. However, if this was a macro, I think the need for having
> >> INVALID_P2M_ENTRY would vanish, as long as the stub macro didn't
> evaluate its 2nd argument.
> > This is not very future proof... The cost of defining
> > INVALID_M2P_ENTRY is very minimal compare to the damage that may
> > result from this choice.
> >
> >> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
> >> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
> >> existence.
> >
> > I can see pros and cons in both solution. To me it contains the word
> > "M2P" so it makes sense to be protected by HAS_M2P.
> >
> > If someone else think that it should be protected by
> > CONFIG_MEM_SHARING, then I will do the change.
> >
> > I have added Tamas to give him an opportunity to share his view.
> 
> This is clearly guarded by HAS_M2P first first and foremost.
> 
> However, the work to actually let MEM_SHARING be turned off in this regard is
> rather larger, and not appropriate to delay this series with.

I don't see any issue with making CONFIG_MEM_SHARING also depend on 
CONFIG_HAS_M2P, so IMHO it would be enough to put both behind that.

Tamas