Re: [PATCH V3 (resend) 06/19] x86: Add a boot option to enable and disable the direct map

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 13, 2024 at 01:40:33PM +0000, Elias El Yandouzi wrote:
> From: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> 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.
> 
> Note that there remains some users of the directmap at this point. The option
> is introduced now as it will be needed in follow-up patches.

It's hard for me to evaluate whether the option name and the help text
is correct, because the implementation is not yet complete.  It would
be best if this was introduced after the implementation has gone in,
so that the reviewer can evaluate that the text matches the
implementation.  Now it's mostly a promise of what's yet to be
implemented.

> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Elias El Yandouzi <eliasely@xxxxxxxxxx>
> 
> ----
> 
>     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 e760f3266e..743d343ffa 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 directmap region in Xen.

Enable or disable fully populating the directmap region in Xen.

> +
> +By default, Xen creates the directmap region which maps physical memory
                                                          ^ all?
> +in that region. Setting this to no will sparsely populate the directmap,

"Setting this to no" => "Disabling this option will sparsely..."

> +blocking exploits that leak secrets via speculative memory access in the
> +directmap.
> +
>  ### dma_bits
>  > `= <integer>`
>  
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 7e03e4bc55..b4ec0e582e 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -28,6 +28,7 @@ config X86
>       select HAS_PCI_MSI
>       select HAS_PIRQ
>       select HAS_SCHED_GRANULARITY
> +     select HAS_SECRET_HIDING
>       select HAS_UBSAN
>       select HAS_VPCI if HVM
>       select NEEDS_LIBELF
> diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> index 98b66edaca..54d835f156 100644
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -622,11 +622,17 @@ 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, the directmap will mostly be 
> empty
> + * 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);
>  
> +    if ( !has_directmap() )
> +        return false;
> +
>      return (mfn + nr) <= (virt_to_mfn(eva - 1) + 1);
>  }
>  
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index f84e1cd79c..bd6b1184f5 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1517,6 +1517,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");

IMO this wants to be printed as part of the speculation mitigations, see
print_details() in spec_ctrl.c

> +
>      /*
>       * 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 565ceda741..856604068c 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -80,12 +80,29 @@ config HAS_PMAP
>  config HAS_SCHED_GRANULARITY
>       bool
>  
> +config HAS_SECRET_HIDING
> +     bool
> +
>  config HAS_UBSAN
>       bool
>  
>  config MEM_ACCESS_ALWAYS_ON
>       bool
>  
> +config SECRET_HIDING
> +    bool "Secret hiding"
> +    depends on HAS_SECRET_HIDING

IMO 'SECRET_HIDING' is too generic, this wants a more specific name.
Maybe SPARCE_DIRECTMAP or some such.

> +    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 7c1bdfc046..9b7e4721cd 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -174,6 +174,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 7561297a75..9d4f1f2d0d 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -167,6 +167,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;

This likely wants:

return IS_ENABLED(CONFIG_HAS_SECRET_HIDING) && opt_directmap;

So that when HAS_SECRET_HIDING is build time disabled the compiler can
likely eliminate the code.

Thanks, Roger.