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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan