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

[Julien Grall <julien@xen.org>]

Hi Oleksii,

On 23/03/2023 12:30, Oleksii wrote:
> On Thu, 2023-03-23 at 11:57 +0000, Julien Grall wrote:
>> On 23/03/2023 11:18, Oleksii wrote:
>>> Hi Julien,
>>
>> Hi Oleksii,
>>
>>> On Wed, 2023-03-22 at 14:21 +0000, Julien Grall wrote:
>>> ...
>>>>
>>>>>>> +    unsigned long page_addr;
>>>>>>> +
>>>>>>> +    // /* align start addresses */
>>>>>>> +    // map_start &= XEN_PT_LEVEL_MAP_MASK(0);
>>>>>>> +    // pa_start &= XEN_PT_LEVEL_MAP_MASK(0);
>>>>>>
>>>>>> They should be switched to ASSERT() or BUG_ON().
>>>>> Sure. Thanks. I'll update.
>>>>>>
>>>>>>> +
>>>>>>> +    page_addr = map_start;
>>>>>>> +    while ( page_addr < map_end )
>>>>>>
>>>>>> This loop is continue to assume that only the mapping can
>>>>>> first
>>>>>> in
>>>>>> 2MB
>>>>>> section (or less if the start is not 2MB aligned).
>>>>>>
>>>>>> I am OK if you want to assume it, but I think this should be
>>>>>> documented
>>>>>> (with words and ASSERT()/BUG_ON()) to avoid any mistake.
>>>>> I add a check in setup_initial_pagetables:
>>>>>         BUG_ON(load_addr_start % (PAGE_ENTRIES * PAGE_SIZE) !=
>>>>> 0);
>>>>> Probably this is not a correct place and should be moved to
>>>>> setup_initial_mapping() instead of setup_initial_pagetables()
>>>>
>>>> Yes it should be moved in setup_initial_mapping().
>>> Then I'll moved it to setup_initial_mapping()
>>>>
>>>>>>
>>>>>>> +    {
>>>>>>> +        unsigned long index2 = pt_index(2, page_addr);
>>>>>>> +        unsigned long index1 = pt_index(1, page_addr);
>>>>>>> +        unsigned long index0 = pt_index(0, page_addr);
>>>>>>> +
>>>>>>> +        /* Setup level2 table */
>>>>>>> +        second[index2] = paddr_to_pte((unsigned
>>>>>>> long)first);
>>>>>>> +        second[index2].pte |= PTE_TABLE;
>>>>>>> +
>>>>>>> +        /* Setup level1 table */
>>>>>>> +        first[index1] = paddr_to_pte((unsigned
>>>>>>> long)zeroeth);
>>>>>>> +        first[index1].pte |= PTE_TABLE;
>>>>>>> +
>>>>>>> +
>>>>>>
>>>>>> NIT: Spurious line?
>>>>> Yeah, should be removed. Thanks.
>>>>>>
>>>>>>> +        /* Setup level0 table */
>>>>>>> +        if ( !pte_is_valid(&zeroeth[index0]) )
>>>>>>
>>>>>> On the previous version, you said it should be checked for
>>>>>> each
>>>>>> level.
>>>>> I had a terrible internet connection, and my message wasn't
>>>>> sent.
>>>>
>>>> No worries.
>>>>
>>>>>
>>>>> I decided not to check that l2 and l1 are used only for
>>>>> referring
>>>>> to
>>>>> the next page table but not leaf PTE. So it is safe to
>>>>> overwrite it
>>>>> each time (the addresses of page tables are the same all the
>>>>> time)
>>>>
>>>> You are letting the caller to decide which page-table to use for
>>>> each
>>>> level. So you are at the mercy that caller will do the right
>>>> thing.
>>>>
>>>> IHMO, this is a pretty bad idea because debugging page-tables
>>>> error
>>>> are
>>>> difficult. So it is better to have safety in place. This is not
>>>> worth...
>>>>
>>>>     and
>>>>> probably it will be better from optimization point of view to
>>>>> ignore if
>>>>> clauses.
>>>>
>>>> ... the optimization in particular when this is at boot time.
>>> I didn't think about that caller will do always the right thing so
>>> I will add the check.
>>>>
>>>>>
>>>>> And it is needed in case of L0 because it is possible that some
>>>>> addressed were marked with specific flag ( execution, read,
>>>>> write )
>>>>> and
>>>>> so not to overwrite the flags set before the check is needed.
>>>> In which case you should really report an error because the
>>>> caller
>>>> may
>>>> have decide to set execution flag and you don't honor. So when
>>>> the
>>>> code
>>>> is executed, you will receive a fault and this may be hard to
>>>> find
>>>> out
>>>> what happen.
>>>
>>> Right now, it is expected situation that the caller will try to
>>> change
>>> execution flag during the setup of initial page tables. >
>>> It is possible in the currently implemented logic of the setup of
>>> initial page tables.
>>
>> This sounds like a bug in your caller implementation. You should not
>> try
>> to workaround this in your code updating the page-tables.
>>
>>> Let me explain what I mean.
>>>
>>> The first step of setup_initial_pagetables() is to map .text,
>>> .init,
>>> .rodata with necessary flags RX, RX, R.
>>> Remaining sections will have RW flags, and to map them,
>>> setup_initial_mapping() is called for the whole range of
>>> [linker_start,
>>> linker_end] not to map them one by one thereby during this step
>>> setup_initial_mapping() will try to remap addresses ranges which
>>> overlap with .text, .init, .rodata with RW flags but it should
>>> leave
>>> with the previously set flags.
>> Why do you need to call setup_init_mapping() with the whole range? In
>> fact the only reason I can think this is useful is when you when to
>> create a 1:1 mapping when the linker and load address is different.
> It is needed to not map each section separately one by one as most of
> the sections have the same PTE_FLAGS (Read, Write, eXectuable, etc )
> 
> So it was decided to map the following sections separately as they have
> 'unique' flags:
>   - .text -> RX
>   - .rodata -> R
>   - .init.text -> RX
> 
> All other sections are RW and could be covered by one call of
> setup_init_mapping() for the whole range:
>   - .data.ro_after_init
>   - .data.read_mostly
>   - .data
>   - .init.data
>   - .bss
> So some ranges ( .text, .rodata, .init.text ) from the whole range will
> be skipped as already mapped and the rest sections will be mapped
> during one call of setup_init_mapping().

This approach is very fragile and not scalable because:
  * You can't use setup_initial_mapping() to change the permission flags.
  * You can't catch caller mistakes (e.g. imagine you end up to use a 
different physical region).

I can see two solutions:

   1) Loop through the region page and page and check within permission 
you want (see the loop in setup_pagetables() for Arm).
   2) Re-order the calls so you want first all Xen and then update the 
permission flags as it fits.

I don't have a strong preference between the two options here.

Cheers,

-- 
Julien Grall