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

[Jan Beulich <jbeulich@xxxxxxxx>]

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