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

[Elias El Yandouzi <eliasely@xxxxxxxxxx>]

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