Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map

[Elias El Yandouzi <eliasely@amazon.com>]

On 11/01/2024 14:09, Jan Beulich wrote:
> On 11.01.2024 13:25, Julien Grall wrote:
>> Hi Jan,
>>
>> On 11/01/2024 11:53, Jan Beulich wrote:
>>> On 11.01.2024 11:47, Elias El Yandouzi wrote:
>>>> On 22/12/2022 13:24, Jan Beulich wrote:
>>>>> That said, I think this change comes too early in the series, or there is
>>>>> something missing.
>>>>
>>>> At first, I had the same feeling but looking at the rest of the series,
>>>> I can see that the option is needed in follow-up patches.
>>>>
>>>>> As said in reply to patch 10, while there the mapcache
>>>>> is being initialized for the idle domain, I don't think it can be used
>>>>> just yet. Read through mapcache_current_vcpu() to understand why I think
>>>>> that way, paying particular attention to the ASSERT() near the end.
>>>>
>>>> Would be able to elaborate a bit more why you think that? I haven't been
>>>> able to get your point.
>>>
>>> Why exactly I referred to the ASSERT() there I can't reconstruct. The
>>> function as a whole looks problematic though when suddenly the idle
>>> domain also gains a mapcache. I'm sorry, too much context was lost
>>> from over a year ago; all of this will need looking at from scratch
>>> again whenever a new version was posted.
>>>
>>>>> In preparation of this patch here I think the mfn_to_virt() uses have to all
>>>>> disappear from map_domain_page(). Perhaps yet more strongly all
>>>>> ..._to_virt() (except fix_to_virt() and friends) and __va() have to
>>>>> disappear up front from x86 and any code path which can be taken on x86
>>>>> (which may simply mean purging all respective x86 #define-s, without
>>>>> breaking the build in any way).
>>>>
>>>> I agree with you on that one. I think it is what we're aiming for in the
>>>> long term. However, as mentioned by Julien in the cover letter, the
>>>> series's name is a misnomer and I am afraid we won't be able to remove
>>>> all of them with this series. These helpers would still be used for
>>>> xenheap pages or when the direct map is enabled.
>>>
>>> Leaving a hazard of certain uses not having been converted, or even
>>> overlooked in patches going in at around the same time as this series?
>>> I view this as pretty "adventurous".
>>
>> Until we get rid of the directmap completely (which is not the goal of
>> this series), we will need to keep mfn_to_virt().
>>
>> In fact the one you ask to remove in map_domain_page() will need to be
>> replaced with function doing the same thing. The same for the code that
>> will initially prepare the directmap. This to avoid impacting
>> performance when the user still wants to use the directmap.
>>
>> So are you just asking to remove most of the use and rename *_to_virt()
>> to something that is more directmap specific (e.g. mfn_to_directmap_virt())?
> 
> Well, in a way. If done this way, mfn_to_virt() (and __va()) should have no
> users by the end of the series, and it would be obvious that nothing was
> missed (and by then purging the old ones we could also ensure no new uses
> would appear).

What about maddr_to_virt()? For instance, in the function 
xen/arch/x86/dmi_scan.c:dmi_iterate(), we need to access a very low 
machine address which isn't in the directmap range.

How would you proceed? Calling vmap() seems to be a bit overkill for 
just a temporary mapping and I don't really want to rework this function 
to use map_domain_page().

In such case, how would you proceed? What do you suggest?

Cheers,

-- 
Elias