Re: [PATCH v2 (resend) 13/27] x86: Add a boot option to enable and disable the direct map

[Jan Beulich <jbeulich@xxxxxxxx>]

On 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