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-6b0cb2b90...@xen.org/
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
