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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.01.2024 19:25, Elias El Yandouzi wrote:
> 
> 
> 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.

I'm afraid I don't follow: Very low addresses are always in the
direct map range, which - on x86 - always starts at 0.

> 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?

fixmap may be an option to consider, but I also don't see why you
apparently think using vmap() would be a possibility while at the
same time making use of map_domain_page() is too much effort.

Jan