Re: [PATCH v2 09/12] x86: make mem-paging configuarable and default it to off for being unsupported

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>, Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Daniel de Graaf <dgdegra@tycho.nsa.gov>,
	Paul Durrant <paul@xen.org>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Petre Pircalabu <ppircalabu@bitdefender.com>,
	Alexandru Isaila <aisaila@bitdefender.com>
Subject: Re: [PATCH v2 09/12] x86: make mem-paging configuarable and default it to off for being unsupported
Date: Fri, 30 Apr 2021 16:16:24 +0200	[thread overview]
Message-ID: <143b84c7-84dd-6b36-997e-a38718651723@suse.com> (raw)
In-Reply-To: <YIvUJqtdsOLia8nc@Air-de-Roger>

On 30.04.2021 11:55, Roger Pau Monné wrote:
> On Mon, Apr 12, 2021 at 04:12:41PM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_64/compat/mm.c
>> +++ b/xen/arch/x86/x86_64/compat/mm.c
>> @@ -155,8 +155,10 @@ int compat_arch_memory_op(unsigned long
>>      case XENMEM_get_sharing_shared_pages:
>>          return mem_sharing_get_nr_shared_mfns();
>>  
>> +#ifdef CONFIG_MEM_PAGING
>>      case XENMEM_paging_op:
>>          return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
>> +#endif
>>  
>>  #ifdef CONFIG_MEM_SHARING
>>      case XENMEM_sharing_op:
>> --- a/xen/arch/x86/x86_64/mm.c
>> +++ b/xen/arch/x86/x86_64/mm.c
>> @@ -994,8 +994,10 @@ long subarch_memory_op(unsigned long cmd
>>      case XENMEM_get_sharing_shared_pages:
>>          return mem_sharing_get_nr_shared_mfns();
>>  
>> +#ifdef CONFIG_MEM_PAGING
>>      case XENMEM_paging_op:
>>          return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
>> +#endif
> 
> I would create a dummy handler when !CONFIG_MEM_PAGING in
> asm-x86/mem_paging.h.

I was simply following the neighboring mem-sharing approach,
which you've stripped here, but which is still visible in the
xen/arch/x86/x86_64/compat/mm.c change above. I think the two
are helpful to be similar in such aspects.

>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -136,11 +136,16 @@ typedef unsigned int p2m_query_t;
>>  #define P2M_PAGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
>>                              | p2m_to_mask(p2m_ram_logdirty) )
>>  
>> +#ifdef CONFIG_MEM_PAGING
>>  #define P2M_PAGING_TYPES (p2m_to_mask(p2m_ram_paging_out)        \
>>                            | p2m_to_mask(p2m_ram_paged)           \
>>                            | p2m_to_mask(p2m_ram_paging_in))
>>  
>>  #define P2M_PAGED_TYPES (p2m_to_mask(p2m_ram_paged))
>> +#else
>> +#define P2M_PAGING_TYPES 0
>> +#define P2M_PAGED_TYPES 0
>> +#endif
> 
> Since you don't guard the p2m related paged types in p2m_type_t is it
> worth having diverging P2M_{PAGING/PAGED}_TYPES?
> 
> I guess it might be required in order to force the compiler to DCE
> without having to add yet more CONFIG_MEM_PAGING guards?

Indeed, this is the reason.

> I don't really have a strong opinion on whether the code should be
> removed, IMO it's best to start by making it off by default at build
> time and remove it in a later release?

Matches my way of thinking. I also wouldn't want to be the one to
delete code completely out of the blue.

Jan