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.
This is added as a Kconfig option as well as a boot command line option.
While being generic, the Kconfig option is only usable for x86 at the moment.
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
Changes in V2:
* Introduce a Kconfig option
* Reword the commit message
* Make opt_directmap and helper generic
Changes since Hongyan's version:
* Reword the commit message
* opt_directmap is only modified during boot so mark it as
__ro_after_init
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 8e65f8bd18..63c946f482 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying
hardware guarantees (with, where available and known to Xen, respective
tweaks applied).
+### directmap (x86)
+> `= <boolean>`
+
+> Default: `true`
+
+Enable or disable the direct map region in Xen.
+
+By default, Xen creates the direct map region which maps physical memory
+in that region. Setting this to no will remove the direct map, blocking
+exploits that leak secrets via speculative memory access in the direct
+map.
+
### dma_bits
> `= <integer>`
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 1acdffc51c..350f41b832 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -29,6 +29,7 @@ config X86
select HAS_UBSAN
select HAS_VPCI if HVM
select NEEDS_LIBELF
+ select HAS_SECRET_HIDING
config ARCH_DEFCONFIG
string
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index 7d26d9cd2f..4aae270a78 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -620,10 +620,18 @@ void write_32bit_pse_identmap(uint32_t *l2);
/*
* x86 maps part of physical memory via the directmap region.
* Return whether the range of MFN falls in the directmap region.
+ *
+ * When boot command line sets directmap=no, we will not have a direct map at
+ * all so this will always return false.
*/
static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
{
- unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
+ unsigned long eva;
+
+ if ( !has_directmap() )
+ return false;
+
+ eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1);
}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4d0c90b7a0..b813ea75b5 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1512,6 +1512,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
if ( highmem_start )
xenheap_max_mfn(PFN_DOWN(highmem_start - 1));
+ printk("Booting with directmap %s\n", has_directmap() ? "on" : "off");
+
/*
* Walk every RAM region and map it in its entirety (on x86/64, at least)
* and notify it to the boot allocator.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 310ad4229c..9a24c89ac5 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -83,6 +83,23 @@ config HAS_UBSAN
config MEM_ACCESS_ALWAYS_ON
bool
+config HAS_SECRET_HIDING
+ bool
+
+config SECRET_HIDING
+ bool "Secret hiding"
+ depends on HAS_SECRET_HIDING
+ ---help---
+ The directmap contains mapping for most of the RAM which makes domain
+ memory easily accessible. While making the performance better, it also makes
+ the hypervisor more vulnerable to speculation attacks.
+
+ Enabling this feature will allow the user to decide whether the memory
+ is always mapped at boot or mapped only on demand (see the command line
+ option "directmap").
+
+ If unsure, say N.
+
config MEM_ACCESS
def_bool MEM_ACCESS_ALWAYS_ON
prompt "Memory Access and VM events" if !MEM_ACCESS_ALWAYS_ON
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 740b6f0ff7..a3746cfbcf 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -173,6 +173,11 @@ paddr_t __ro_after_init mem_hotplug;
static char __initdata opt_badpage[100] = "";
string_param("badpage", opt_badpage);
+bool __ro_after_init opt_directmap = true;
+#ifdef CONFIG_HAS_SECRET_HIDING
+boolean_param("directmap", opt_directmap);
+#endif
+
/*
* no-bootscrub -> Free pages are not zeroed during boot.
*/
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3d9b2d05a5..f860e98ee4 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -165,6 +165,13 @@ extern unsigned long max_page;
extern unsigned long total_pages;
extern paddr_t mem_hotplug;
+extern bool opt_directmap;
+
+static inline bool has_directmap(void)
+{
+ return opt_directmap;
+}
+
/*
* Extra fault info types which are used to further describe
* the source of an access violation.
--
2.40.1On 16.01.2024 20:25, Elias El Yandouzi wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -799,6 +799,18 @@ that enabling this option cannot guarantee anything beyond what underlying
> hardware guarantees (with, where available and known to Xen, respective
> tweaks applied).
>
> +### directmap (x86)
> +> `= <boolean>`
> +
> +> Default: `true`
> +
> +Enable or disable the direct map region in Xen.
> +
> +By default, Xen creates the direct map region which maps physical memory
> +in that region. Setting this to no will remove the direct map, blocking
> +exploits that leak secrets via speculative memory access in the direct
> +map.
I think this wants wording such that the full truth is conveyed: The directmap
doesn't disappear. It's merely only sparsely populated then.
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -29,6 +29,7 @@ config X86
> select HAS_UBSAN
> select HAS_VPCI if HVM
> select NEEDS_LIBELF
> + select HAS_SECRET_HIDING
Please respect alphabetic sorting. As to "secret hiding" - personally I
consider this too generic a term. This is about limiting the direct map. Why
not name the option then accordingly?
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -620,10 +620,18 @@ void write_32bit_pse_identmap(uint32_t *l2);
> /*
> * x86 maps part of physical memory via the directmap region.
> * Return whether the range of MFN falls in the directmap region.
> + *
> + * When boot command line sets directmap=no, we will not have a direct map at
> + * all so this will always return false.
> */
As with the command line doc, please state the full truth.
> static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
> {
> - unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
> + unsigned long eva;
> +
> + if ( !has_directmap() )
> + return false;
Hmm. The sole user of this function is init_node_heap(). Would it perhaps make
sense to simply map the indicated number of pages then? init_node_heap() would
fall back to xmalloc(), so the data will be in what's left of the directmap
anyway.
> + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
Irrespective I don't see a need to replace the initializer by an assignment.
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -83,6 +83,23 @@ config HAS_UBSAN
> config MEM_ACCESS_ALWAYS_ON
> bool
>
> +config HAS_SECRET_HIDING
> + bool
This again wants placing suitably among the other HAS_*.
> +config SECRET_HIDING
> + bool "Secret hiding"
> + depends on HAS_SECRET_HIDING
> + ---help---
> + The directmap contains mapping for most of the RAM which makes domain
> + memory easily accessible. While making the performance better, it also makes
> + the hypervisor more vulnerable to speculation attacks.
> +
> + Enabling this feature will allow the user to decide whether the memory
> + is always mapped at boot or mapped only on demand (see the command line
> + option "directmap").
> +
> + If unsure, say N.
Nit: Indentation and no ---help--- anymore (just help) please in new Kconfig
entries.
Also as an alternative did you consider making this new setting merely
control the default of opt_directmap? Otherwise the variable shouldn't exist
at all when the Kconfig option is off, but rather be #define-d to "true" in
that case.
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -165,6 +165,13 @@ extern unsigned long max_page;
> extern unsigned long total_pages;
> extern paddr_t mem_hotplug;
>
> +extern bool opt_directmap;
> +
> +static inline bool has_directmap(void)
> +{
> + return opt_directmap;
> +}
If opt_directmap isn't static, I see little point in having such a wrapper.
If there are reasons, I think they want stating in the description.
On the whole: Is the placement of this patch in the series an indication
that as of here all directmap uses have gone away? If so, what's the rest of
the series about? Alternatively isn't use of this option still problematic
at this point of the series? Whichever way it is - this wants clarifying in
the description.
Jan
On 20/02/2024 11:14, Jan Beulich wrote:
> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -29,6 +29,7 @@ config X86
>> select HAS_UBSAN
>> select HAS_VPCI if HVM
>> select NEEDS_LIBELF
>> + select HAS_SECRET_HIDING
>
> Please respect alphabetic sorting. As to "secret hiding" - personally I
> consider this too generic a term. This is about limiting the direct map. Why
> not name the option then accordingly?
>
I think it is a fairly decent name, would you have any suggestion?
Otherwise I will just stick to it.
>> --- a/xen/arch/x86/include/asm/mm.h
>> +++ b/xen/arch/x86/include/asm/mm.h
>> @@ -620,10 +620,18 @@ void write_32bit_pse_identmap(uint32_t *l2);
>> /*
>> * x86 maps part of physical memory via the directmap region.
>> * Return whether the range of MFN falls in the directmap region.
>> + *
>> + * When boot command line sets directmap=no, we will not have a direct map at
>> + * all so this will always return false.
>> */
>
> As with the command line doc, please state the full truth.
>
>> static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
>> {
>> - unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>> + unsigned long eva;
>> +
>> + if ( !has_directmap() )
>> + return false;
>
> Hmm. The sole user of this function is init_node_heap(). Would it perhaps make
> sense to simply map the indicated number of pages then? init_node_heap() would
> fall back to xmalloc(), so the data will be in what's left of the directmap
> anyway.
>
There will be more users of arch_mfns_in_directmap() in the following
patches.
>> + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>
> Irrespective I don't see a need to replace the initializer by an assignment.
I guess it was to avoid the useless min() computation in case directmap
is disabled. I can put it back to what it was.
>
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -83,6 +83,23 @@ config HAS_UBSAN
>> config MEM_ACCESS_ALWAYS_ON
>> bool
>>
>> +config HAS_SECRET_HIDING
>> + bool
>
> This again wants placing suitably among the other HAS_*.
>
>> +config SECRET_HIDING
>> + bool "Secret hiding"
>> + depends on HAS_SECRET_HIDING
>> + ---help---
>> + The directmap contains mapping for most of the RAM which makes domain
>> + memory easily accessible. While making the performance better, it also makes
>> + the hypervisor more vulnerable to speculation attacks.
>> +
>> + Enabling this feature will allow the user to decide whether the memory
>> + is always mapped at boot or mapped only on demand (see the command line
>> + option "directmap").
>> +
>> + If unsure, say N.
>
> Also as an alternative did you consider making this new setting merely
> control the default of opt_directmap? Otherwise the variable shouldn't exist
> at all when the Kconfig option is off, but rather be #define-d to "true" in
> that case.
I am not sure to understand why the option shouldn't exist at all when
Kconfig option is off.
If SECRET_HIDING option is off, then opt_directmap must be
unconditionally set to true. If SECRET_HIDING option is on, then
opt_directmap value depends on the commandline option.
The corresponding wrapper, has_directmap(), will be used in multiple
location in follow-up patch. I don't really see how you want to do.
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -165,6 +165,13 @@ extern unsigned long max_page;
>> extern unsigned long total_pages;
>> extern paddr_t mem_hotplug;
>>
>> +extern bool opt_directmap;
>> +
>> +static inline bool has_directmap(void)
>> +{
>> + return opt_directmap;
>> +}
>
> If opt_directmap isn't static, I see little point in having such a wrapper.
> If there are reasons, I think they want stating in the description.
I don't think there is a specific reason to be mentioned, if you really
wish to, I can remove it.
> On the whole: Is the placement of this patch in the series an indication
> that as of here all directmap uses have gone away? If so, what's the rest of
> the series about? Alternatively isn't use of this option still problematic
> at this point of the series? Whichever way it is - this wants clarifying in
> the description.
This patch is not an indication that all directmap uses have been
removed. We need to know in follow-up patch whether or not the option is
enabled and so we have to introduce this patch here.
At this point in the series, the feature is not yet complete.
Elias
On 13.05.2024 12:50, Elias El Yandouzi wrote:
> On 20/02/2024 11:14, Jan Beulich wrote:
>> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -29,6 +29,7 @@ config X86
>>> select HAS_UBSAN
>>> select HAS_VPCI if HVM
>>> select NEEDS_LIBELF
>>> + select HAS_SECRET_HIDING
>>
>> Please respect alphabetic sorting. As to "secret hiding" - personally I
>> consider this too generic a term. This is about limiting the direct map. Why
>> not name the option then accordingly?
>
> I think it is a fairly decent name, would you have any suggestion?
> Otherwise I will just stick to it.
See how Roger, on v3, has now responded along the same lines? His naming
suggestion (with spelling adjusted) would be fine with me.
>>> + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>>
>> Irrespective I don't see a need to replace the initializer by an assignment.
>
> I guess it was to avoid the useless min() computation in case directmap
> is disabled. I can put it back to what it was.
The compiler ought to be able to re-arrange code accordingly, if it thinks
the overall result will then be better.
>>> +config SECRET_HIDING
>>> + bool "Secret hiding"
>>> + depends on HAS_SECRET_HIDING
>>> + ---help---
>>> + The directmap contains mapping for most of the RAM which makes domain
>>> + memory easily accessible. While making the performance better, it also makes
>>> + the hypervisor more vulnerable to speculation attacks.
>>> +
>>> + Enabling this feature will allow the user to decide whether the memory
>>> + is always mapped at boot or mapped only on demand (see the command line
>>> + option "directmap").
>>> +
>>> + If unsure, say N.
>>
>> Also as an alternative did you consider making this new setting merely
>> control the default of opt_directmap? Otherwise the variable shouldn't exist
>> at all when the Kconfig option is off, but rather be #define-d to "true" in
>> that case.
>
> I am not sure to understand why the option shouldn't exist at all when
> Kconfig option is off.
I didn't say "option", but "variable", and ...
> If SECRET_HIDING option is off, then opt_directmap must be
> unconditionally set to true. If SECRET_HIDING option is on, then
> opt_directmap value depends on the commandline option.
... I did clearly say what I think you want to do, bringing things in line
with other opt_* that reduce to a constant when a certain CONFIG_* is not
defined.
> The corresponding wrapper, has_directmap(), will be used in multiple
> location in follow-up patch. I don't really see how you want to do.
The wrapper is fine to have if, as per the earlier reply still visible in
context below, the variable itself can then be suitably static (and the
fallback #define local to that same C file). Otherwise I simply don't see
the value of the wrapper function.
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -165,6 +165,13 @@ extern unsigned long max_page;
>>> extern unsigned long total_pages;
>>> extern paddr_t mem_hotplug;
>>>
>>> +extern bool opt_directmap;
>>> +
>>> +static inline bool has_directmap(void)
>>> +{
>>> + return opt_directmap;
>>> +}
>>
>> If opt_directmap isn't static, I see little point in having such a wrapper.
>> If there are reasons, I think they want stating in the description.
>
> I don't think there is a specific reason to be mentioned, if you really
> wish to, I can remove it.
>
>> On the whole: Is the placement of this patch in the series an indication
>> that as of here all directmap uses have gone away? If so, what's the rest of
>> the series about? Alternatively isn't use of this option still problematic
>> at this point of the series? Whichever way it is - this wants clarifying in
>> the description.
>
> This patch is not an indication that all directmap uses have been
> removed. We need to know in follow-up patch whether or not the option is
> enabled and so we have to introduce this patch here.
There's a pretty clear indication: "directmap=off" means "no directmap".
It does not mean "a little less of direct mapping". Aiui that won't even
change by the end of the series. It's only the ratio which is going to
change.
> At this point in the series, the feature is not yet complete.
Right, and again - see how Roger, on v3, has now replied along the same
line.
Jan
© 2016 - 2026 Red Hat, Inc.