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

[Julien Grall <julien@xxxxxxx>]

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.

Cheers,

--
Julien Grall