Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 6/26/19 9:25 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> The ID map may clash with other parts of the Xen virtual memory layout.
>> At the moment, Xen is handling the clash by only creating a mapping to
>> the runtime virtual address before enabling the MMU.
>>
>> The rest of the mappings (such as the fixmap) will be mapped after the
>> MMU is enabled. However, the code doing the mapping is not safe as it
>> replace mapping without using the Break-Before-Make sequence.
>>
>> As the ID map can be anywhere in the memory, it is easier to remove all
>> the entries added as soon as the ID map is not used rather than adding
>> the Break-Before-Make sequence everywhere.
> 
> I think it is a good idea, but I would ask you to mention 1:1 map
> instead of "ID map" in comments and commit messages because that is the
> wording we used in all comments so far (see the one in
> create_page_tables and mm.c). It is easier to grep and refer to if we
> use the same nomenclature. Note that I don't care about which
> nomenclature we decide to use, I am only asking for consistency.
> Otherwise, it would also work if you say it both way at least once:
> 
>   The ID map (1:1 map) may clash [...]

I would rather drop the wording 1:1 as this is confusing. It is also not 
trivial to find anything on google typing "1:1".

> 
> 
>> It is difficult to track where exactly the ID map was created without a
>> full rework of create_page_tables(). Instead, introduce a new function
>> remove_id_map() will look where is the top-level entry for the ID map
>> and remove it.
> 
> Do you think it would be worth simplifying this code below by preserving
> where/how the ID map was created? We could repurpose x25 for that,
> carrying for instance the address of the ID map section slot or a code
> number to specify which case we are dealing with. We should be able to
> turn remove_id_map into only ~5 lines?

I thought about it but the current implementation of create_map_tables() 
is quite awful to read. So the less I touch this function, the better I 
feel :).

I have some rework for the create_page_tables() which simplify it a lot. 
Yet, I am not entirely sure it is worth to spend time trying to simplify 
remove_id_map. This is unlikely to make the boot significantly faster 
and I don't expect the function to survive more than a release as the ID 
map as to be kept in place (for secondary boot and suspend/resume).

The only reason it is removed now is because it clashes with other 
mapping we may do.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel