Re: [PATCH v1 1/3] xen/riscv: introduce setup_initial_pages

[Julien Grall <julien@xxxxxxx>]

On 05/03/2023 16:25, Oleksii wrote:
> Hi Julien,
Hi,

Sorry for the late answer. I was away for the past couple of weeks.

> On Mon, 2023-02-27 at 17:36 +0000, Julien Grall wrote:
> > Hi Oleksii,
> >
> > On 27/02/2023 16:52, Oleksii wrote:
> > > On Sat, 2023-02-25 at 17:53 +0000, Julien Grall wrote:
> > > > > +/*
> > > > > + * WARNING: load_addr() and linker_addr() are to be called
> > > > > only
> > > > > when the MMU is
> > > > > + * disabled and only when executed by the primary CPU.  They
> > > > > cannot refer to
> > > > > + * any global variable or functions.
> > > > I find interesting you are saying when
> > > > _setup_initial_pagetables() is
> > > > called from setup_initial_pagetables(). Would you be able to
> > > > explain
> > > > how
> > > > this is different?
> > > I am not sure that I understand your question correctly but
> > > _setup_initial_pagetables() was introduced to map some addresses
> > > with
> > > write/read flag. Probably I have to rename it to something that is
> > > more
> > > clear.
> > So the comment suggests that you code cannot refer to global
> > functions/variables when the MMU is off. So I have multiple
> > questions:
> >     * Why only global? IOW, why static would be OK?
> >     * setup_initial_pagetables() has a call to
> > _setup_initial_pagetables() (IOW referring to another function). Why
> > is
> > it fine?
> >     * You have code in the next patch referring to global variables
> > (mainly _start and _end). How is this different?
> >
> > > > > + */
> > > > > +
> > > > > +/*
> > > > > + * Convert an addressed layed out at link time to the address
> > > > > where it was loaded
> > > > Typo: s/addressed/address/ ?
> > > Yes, it should be address. and 'layed out' should be changed to
> > > 'laid
> > > out'...
> > > > > + * by the bootloader.
> > > > > + */
> > > > Looking at the implementation, you seem to consider that any
> > > > address
> > > > not
> > > > in the range [linker_addr_start, linker_addr_end[ will have a 1:1
> > > > mappings.
> > > >
> > > > I am not sure this is what you want. So I would consider to throw
> > > > an
> > > > error if such address is passed.
> > > I thought that at this stage and if no relocation was done it is
> > > 1:1
> > > except the case when load_addr_start != linker_addr_start.
> > The problem is what you try to map one to one may clash with the
> > linked
> > region for Xen. So it is not always possible to map the region 1:1.
> >
> > Therefore, I don't see any use for the else part here.
> Got it. Thanks.
>
> I am curious than what is the correct approach in general to handle
> this situation?
There are multiple approach to handle it and I don't know which one 
would be best :). Relocation is one...
> I mean that throw an error it is one option but if I would like to do
> that w/o throwing an error. Should it done some relocation in that
> case?
... solution. For Arm, I decided to avoid relocation it requires more 
work in assembly.
Let me describe what we did and you can decide what you want to do in 
RISC-V.
For Arm64, as we have plenty of virtual address space, I decided to 
reshuffle the layout so Xen is running a very high address (so it is 
unlikely to clash).
For Arm32, we have a smaller address space (4GB) so instead we are going 
through a temporary area to enable the MMU when the load and runtime 
region clash. The sequence is:
  1) Map Xen to a temporary area
  2) Enable the MMU and jump to the temporary area
  3) Map Xen to the runtime area
  4) Jump to the runtime area
  5) Remove the temporary area

[...]

> > > > Hmmm... I would actually expect the address to be properly
> > > > aligned
> > > > and
> > > > therefore not require an alignment here.
> > > >
> > > > Otherwise, this raise the question of what happen if you have
> > > > region
> > > > using the same page?
> > > That map_start &=  ZEROETH_MAP_MASK is needed to page number of
> > > address
> > > w/o page offset.
> > My point is why would the page offset be non-zero?
> I checked a linker script and addresses that passed to
> setup_initial_mapping() and they are really always aligned so there is
> no any sense in additional alignment.
Ok. I would suggest to add some ASSERT()/BUG_ON() in order to confirm 
this is always the case.
[...]

> > > > > +
> > > > > +    /*
> > > > > +     * Create a mapping of the load time address range to...
> > > > > the
> > > > > load time address range.
> > > > Same about the line length here.
> > > >
> > > > > +     * This mapping is used at boot time only.
> > > > > +     */
> > > > > +    _setup_initial_pagetables(second, first, zeroeth,
> > > > This can only work if Xen is loaded at its linked address. So you
> > > > need a
> > > > separate set of L0, L1 tables for the identity mapping.
> > > >
> > > > That said, this would not be sufficient because:
> > > >      1) Xen may not be loaded at a 2M boundary (you can control
> > > > with
> > > > U-boot, but not with EFI). So this may cross a boundary and
> > > > therefore
> > > > need multiple pages.
> > > >      2) The load region may overlap the link address
> > > >
> > > > While I think it would be good to handle those cases from the
> > > > start,
> > > > I
> > > > would understand why are not easy to solve. So I think the
> > > > minimum is
> > > > to
> > > > throw some errors if you are in a case you can't support.
> > > Do you mean to throw some error in load_addr()/linkder_addr()?
> > In this case, I meant to check if load_addr != linker_addr, then
> > throw
> > an error.
> I am not sure that it is needed now and it is easier to throw an error
> but is option exist to handler situation when load_addr != linker_addr
> except throwing an error? relocate?
I believe I answered this above.

Cheers,

--
Julien Grall