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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-29 at 14:06 +0200, Jan Beulich wrote:
> On 29.03.2023 13:43, Oleksii wrote:
> > On Tue, 2023-03-28 at 17:34 +0200, Jan Beulich wrote:
> > > On 27.03.2023 19:17, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/mm.c
> > > > @@ -0,0 +1,277 @@
> > > > +#include <xen/init.h>
> > > > +#include <xen/kernel.h>
> > > > +
> > > > +#include <asm/early_printk.h>
> > > > +#include <asm/config.h>
> > > > +#include <asm/csr.h>
> > > > +#include <asm/mm.h>
> > > > +#include <asm/page.h>
> > > > +#include <asm/processor.h>
> > > > +
> > > > +#define PGTBL_INITIAL_COUNT 8
> > > 
> > > What does this magic number derive from? (A comment may help.)
> > It can be now 4 tables as it is enough to map 2Mb. 8 it was when I
> > experimented with bigger sizes of Xen binary and verified that
> > logic of
> > working with page tables works.
> 
> Since this is connected to Xen's image size as you say, I guess the
> constant wants to move to a header, and then be used in an assertion
> in xen.lds.S. That way one will become easily aware if/when this 2Mb
> assumption breaks.
then we can not be tied to 2MB but to:
ASSERT(_end - _start <= MB((PAGE_TABLE_INIT_COUNT - 2) * PAGE_SIZE),
"Xen too large for early-boot assumptions")

> 
> > > > +struct mmu_desc {
> > > > +    unsigned long num_levels;
> > > > +    uint32_t pgtbl_count;
> > > 
> > > Nit: Style (as before please avoid fixed width types when their
> > > use
> > > isn't really warranted; afaics unsigned int will do fine here and
> > > elsewhere below).
> > I will change it but I am not sure that I fully understand what is
> > an
> > issue with uint32_t.
> 
> The issue is simply that ./CODING_STYLE says to prefer basic types
> whenever possible.
Thanks.

> 
> > > > +void __init setup_initial_pagetables(void)
> > > > +{
> > > > +    struct mmu_desc mmu_desc = { 0, 0, NULL, 0 };
> > > > +
> > > > +    /*
> > > > +     * Access to _{stard, end } is always PC-relative
> > > > +     * thereby when access them we will get load adresses
> > > > +     * of start and end of Xen
> > > > +     * To get linker addresses LOAD_TO_LINK() is required
> > > > +     * to use
> > > > +     */
> > > > +    unsigned long load_start    = (unsigned long)_start;
> > > > +    unsigned long load_end      = (unsigned long)_end;
> > > > +    unsigned long linker_start  = LOAD_TO_LINK(load_start);
> > > > +    unsigned long linker_end    = LOAD_TO_LINK(load_end);
> > > > +
> > > > +    if ( (linker_start != load_start) &&
> > > > +         (linker_start <= load_end) && (load_start <=
> > > > linker_end)
> > > > ) {
> > > > +        early_printk("(XEN) linker and load address ranges
> > > > overlap\n");
> > > > +        die();
> > > > +    }
> > > > +
> > > > +    calc_pgtbl_lvls_num(&mmu_desc);
> > > > +
> > > > +    mmu_desc.pgtbl_base = stage1_pgtbl_root;
> > > > +    mmu_desc.next_pgtbl = stage1_pgtbl_nonroot;
> > > > +
> > > > +    setup_initial_mapping(&mmu_desc,
> > > > +                          linker_start,
> > > > +                          linker_end,
> > > > +                          load_start,
> > > > +                          PTE_LEAF_DEFAULT);
> > > > +
> > > > +    setup_ptes_permission(&mmu_desc);
> > > 
> > > ...: Why does this require a 2nd pass / function in the first
> > > place?
> > Probably I misunderstood Julien and it setup_pte_permission can be
> > done
> > in setup_initial_mapping() but here is the reply:
> > https://lore.kernel.org/xen-devel/79e83610-5980-d9b5-7994-6b0cb2b9049a@xxxxxxx/
> 
> Hmm, yes, his option 2 looks like what you've implemented. Still I
> don't see why the permissions can't be got right on the first pass.

~ Oleksii