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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Tue, 2023-03-21 at 16:25 +0000, Julien Grall wrote:
> 
> 
> 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).
I thought about running Xen very high address.
Thanks. I think it is a nice option to do the same for RISC-V64.

> 
> 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
> 
It is the same for RV32. As we don't support RV32 I will use:
  #error "Add support of MMU for RV32"
> [...]
> 
> > > > > 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.
Yeah, you answered my question completely. Thank you very much.

~ Oleksii