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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-06-12 at 16:24 +0200, Jan Beulich wrote:
> 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.
It makes sense but for some reason it doesn't crash ( I have a bunch of
patches to run dom0 ) but as I mentioned in the previous e-mail I agree
that probably it would be better to map the whole image using 1 Gb page
table for example.


> 
> > > - 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[].
It would be better. but probably we will remove that variable if we
agree to map the whole Xen instead of parts. So I'll wait for your
response before starting to work on new patch series.

Thanks a lot.

~ Oleksii