Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map

From: Elias El Yandouzi <eliasely@amazon.com>
To: Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>
Cc: "Hongyan Xia" <hongyxia@amazon.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Julien Grall" <jgrall@amazon.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map
Date: Mon, 15 Jan 2024 14:50:11 +0000	[thread overview]
Message-ID: <fcfb79c5-0509-4ed3-be58-bd3d0935a21e@amazon.com> (raw)
In-Reply-To: <6926f363-3994-4059-b7e1-cb8e45367be8@suse.com>

Hi,

On 12/01/2024 07:47, Jan Beulich wrote:
> On 11.01.2024 19:25, Elias El Yandouzi wrote:
>> On 11/01/2024 14:09, Jan Beulich wrote:
>>
>> What about maddr_to_virt()? For instance, in the function
>> xen/arch/x86/dmi_scan.c:dmi_iterate(), we need to access a very low
>> machine address which isn't in the directmap range.
> 
> I'm afraid I don't follow: Very low addresses are always in the
> direct map range, which - on x86 - always starts at 0.
> 

I reckon it was poorly phrased. IIUC, we'd like to remove every use of 
*_to_virt() in the case the directmap option is disabled.
So I meant that in this situation, the helper arch_mfns_in_direct_map() 
would return false.

>> How would you proceed? Calling vmap() seems to be a bit overkill for
>> just a temporary mapping and I don't really want to rework this function
>> to use map_domain_page().
>>
>> In such case, how would you proceed? What do you suggest?
> 
> fixmap may be an option to consider, but I also don't see why you
> apparently think using vmap() would be a possibility while at the
> same time making use of map_domain_page() is too much effort.

I thought about using vmap() as it allows to map a contiguous region 
easily. It is also used in the follow-up patch 17/22, so I thought it 
could be viable.

I was reluctant to use map_domain_page() for two reasons. 1) it only 
allows to map one page at the time, so I'd need to rework more deeply 
the function dmi_iterate() 2) because the mapcache wouldn't be ready to 
use at that time, the mapping would end up in PMAP which is meant to map 
the page tables, nothing else.

-- 
Elias