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

From: Jan Beulich <jbeulich@suse.com>
To: 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: Thu, 22 Dec 2022 14:24:30 +0100	[thread overview]
Message-ID: <fc70f967-34bb-9569-f514-7a3208e746c6@suse.com> (raw)
In-Reply-To: <20221216114853.8227-12-julien@xen.org>

On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia <hongyxia@amazon.com>
> 
> Also add a helper function to retrieve it. Change arch_mfns_in_direct_map
> to check this option before returning.

I think the abstract parts of this want to be generic right away. I can't
see why Arm would not suffer from the same issue that this work is trying
to address.

> This is added as a boot command line option, not a Kconfig to allow
> the user to experiment the feature without rebuild the hypervisor.

I think there wants to be a (generic) Kconfig piece here, to control the
default of the option. Plus a 2nd, prompt-less element which an arch can
select to force the setting to always-on, suppressing the choice of
default. That 2nd control would then be used to compile out the
boolean_param() for Arm for the time being.

That said, I think this change comes too early in the series, or there is
something missing. As said in reply to patch 10, while there the mapcache
is being initialized for the idle domain, I don't think it can be used
just yet. Read through mapcache_current_vcpu() to understand why I think
that way, paying particular attention to the ASSERT() near the end. In
preparation of this patch here I think the mfn_to_virt() uses have to all
disappear from map_domain_page(). Perhaps yet more strongly all
..._to_virt() (except fix_to_virt() and friends) and __va() have to
disappear up front from x86 and any code path which can be taken on x86
(which may simply mean purging all respective x86 #define-s, without
breaking the build in any way).

Jan