Re: [PATCH v1 5/8] xen/riscv: introduce identity mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2023 15:48, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
>> -void __init noreturn noinline enable_mmu()
>> +/*
>> + * enable_mmu() can't be __init because __init section isn't part of 
>> identity
>> + * mapping so it will cause an issue after MMU will be enabled.
>> + */
> 
> As hinted at above already - perhaps the identity mapping wants to be
> larger, up to covering the entire Xen image? Since it's temporary
> only anyway, you could even consider using a large page (and RWX
> permission). You already require no overlap of link and load addresses,
> so at least small page mappings ought to be possible for the entire
> image.

To expand on that: Assume a future change on this path results in a call
to memcpy() or memset() being introduced by the compiler (and then let's
further assume this only occurs for a specific compiler version). Right
now such a case would be noticed simply because we don't build those
library functions yet. But it'll likely be a perplexing crash once a full
hypervisor can be built, the more that exception handlers also aren't
mapped.

>> - mmu_is_enabled:
>>      /*
>> -     * Stack should be re-inited as:
>> -     * 1. Right now an address of the stack is relative to load time
>> -     *    addresses what will cause an issue in case of load start address
>> -     *    isn't equal to linker start address.
>> -     * 2. Addresses in stack are all load time relative which can be an
>> -     *    issue in case when load start address isn't equal to linker
>> -     *    start address.
>> -     *
>> -     * We can't return to the caller because the stack was reseted
>> -     * and it may have stash some variable on the stack.
>> -     * Jump to a brand new function as the stack was reseted
>> +     * id_addrs should be in sync with id mapping in
>> +     * setup_initial_pagetables()
> 
> What is "id" meant to stand for here? Also if things need keeping in
> sync, then a similar comment should exist on the other side.

I guess it's meant to stand for "identity mapping", but the common use
of "id" makes we wonder if the variable wouldn't better be ident_addrs[].

Jan