Re: [PATCH 6/7] xen/arm: mm: Move domain_{,un}map_* helpers in a separate file

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Julien Grall" <jgrall@amazon.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 6/7] xen/arm: mm: Move domain_{,un}map_* helpers in a separate file
Date: Sat, 16 Jul 2022 16:00:09 +0100	[thread overview]
Message-ID: <76a622ec-bd10-04c3-250f-c55563f9a210@xen.org> (raw)
In-Reply-To: <c46469ef-6f42-621c-ef4c-3e1e5d6bb0cd@suse.com>

Hi Jan,

On 24/06/2022 10:43, Jan Beulich wrote:
> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The file xen/arch/mm.c has been growing quite a lot. It now contains
>> various independent part of the MM subsytem.
>>
>> One of them is the helpers to map/unmap a page when CONFIG_DOMAIN_PAGE
>> (only used by arm32). Move them in a new file xen/arch/arm/domain_page.c.
>>
>> In order to be able to use CONFIG_DOMAIN_PAGE in the Makefile, a new
>> Kconfig option is introduced that is selected by x86 and arm32.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> In principle
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> But ...
> 
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -10,6 +10,7 @@ config X86
>>   	select ALTERNATIVE_CALL
>>   	select ARCH_SUPPORTS_INT128
>>   	select CORE_PARKING
>> +	select DOMAIN_PAGE
>>   	select HAS_ALTERNATIVE
>>   	select HAS_COMPAT
>>   	select HAS_CPUFREQ
>> diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
>> index 07bcd158314b..fbc4bb3416bd 100644
>> --- a/xen/arch/x86/include/asm/config.h
>> +++ b/xen/arch/x86/include/asm/config.h
>> @@ -22,7 +22,6 @@
>>   #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
>>   #define CONFIG_DISCONTIGMEM 1
>>   #define CONFIG_NUMA_EMU 1
>> -#define CONFIG_DOMAIN_PAGE 1
> 
> ... while I realize it has been named this way, I wonder whether ...
> 
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -11,6 +11,9 @@ config COMPAT
>>   config CORE_PARKING
>>   	bool
>>   
>> +config DOMAIN_PAGE
>> +	bool
> 
> ... this isn't a good opportunity to make the name match what it is
> about - MAP_DOMAIN_PAGE. See e.g. {clear,copy}_domain_page() which
> aren't under this guard, and domain pages in general is a concept we
> can't get away without in the first place.

Fair point. I decided to move this change in a prerequisite name the new 
Kconfig ARCH_MAP_DOMAIN_PAGE.

Cheers,

-- 
Julien Grall