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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +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.

>>> +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.

Jan