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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Julien Grall <julien@xen.org>, Jan Beulich <jbeulich@suse.com>
Cc: <xen-devel@lists.xenproject.org>,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	George Dunlap <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Tamas K Lengyel <tamas.lengyel@intel.com>
Subject: Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P
Date: Tue, 22 Sep 2020 19:59:35 +0100	[thread overview]
Message-ID: <3778eaa1-c1cc-d458-407f-e5ca3ca028ab@citrix.com> (raw)
In-Reply-To: <e0a73d9b-5c68-9b57-dfe4-34e91b0972bd@xen.org>

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.

~Andrew