On 07.07.2023 12:37, Oleksii wrote:
> On Thu, 2023-07-06 at 13:35 +0200, Jan Beulich wrote:
>> On 19.06.2023 15:34, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/config.h
>>> +++ b/xen/arch/riscv/include/asm/config.h
>>> @@ -1,3 +1,5 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> #ifndef __RISCV_CONFIG_H__
>>> #define __RISCV_CONFIG_H__
>>>
>>
>> Unrelated change?
> It should be part of [PATCH v2 5/6] xen/riscv: introduce identity
> mapping.
Hmm, here we're discussing "[PATCH v2 4/6] xen/riscv: introduce identity
mapping". I'm confused, I guess.
>>> --- a/xen/arch/riscv/mm.c
>>> +++ b/xen/arch/riscv/mm.c
>>> @@ -25,6 +25,12 @@ unsigned long __ro_after_init phys_offset;
>>> #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>>> #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
>>>
>>> +/*
>>> + * Should be removed as soon as enough headers will be merged for
>>> inclusion of
>>> + * <xen/lib.h>.
>>> + */
>>> +#define ARRAY_SIZE(arr) (sizeof(arr) /
>>> sizeof((arr)[0]))
>>> +
>>> /*
>>> * It is expected that Xen won't be more then 2 MB.
>>> * The check in xen.lds.S guarantees that.
>>> @@ -35,8 +41,10 @@ unsigned long __ro_after_init phys_offset;
>>> *
>>> * It might be needed one more page table in case when Xen load
>>> address
>>> * isn't 2 MB aligned.
>>> + *
>>> + * (CONFIG_PAGING_LEVELS - 1) page tables are needed for identity
>>> mapping.
>>> */
>>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) + 1)
>>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
>>
>> How come the extra page (see the comment sentence in context) isn't
>> needed for the identity-mapping case?
> It is needed to allocate no more than two 'nonroot' page tables (L0 and
> L1 in case of Sv39 ) as page 'root' table ( L2 in case of Sv39 ) is
> always re-used.
>
> The same ( only 'nonroot' page tables might be needed to allocate )
> works for any MMU mode.
Of course, but if you cross a 2Mb boundary you'll need 2 L0 tables.
>>> @@ -255,25 +262,30 @@ void __init noreturn noinline enable_mmu()
>>> csr_write(CSR_SATP,
>>> PFN_DOWN((unsigned long)stage1_pgtbl_root) |
>>> RV_STAGE1_MODE << SATP_MODE_SHIFT);
>>> +}
>>>
>>> - asm volatile ( ".p2align 2" );
>>> - mmu_is_enabled:
>>> - /*
>>> - * Stack should be re-inited as:
>>> - * 1. Right now an address of the stack is relative to load
>>> time
>>> - * addresses what will cause an issue in case of load start
>>> address
>>> - * isn't equal to linker start address.
>>> - * 2. Addresses in stack are all load time relative which can
>>> be an
>>> - * issue in case when load start address isn't equal to
>>> linker
>>> - * start address.
>>> - *
>>> - * We can't return to the caller because the stack was reseted
>>> - * and it may have stash some variable on the stack.
>>> - * Jump to a brand new function as the stack was reseted
>>> - */
>>> +void __init remove_identity_mapping(void)
>>> +{
>>> + unsigned int i;
>>> + pte_t *pgtbl;
>>> + unsigned int index, xen_index;
>>> + unsigned long load_addr = LINK_TO_LOAD(_start);
>>>
>>> - switch_stack_and_jump((unsigned long)cpu0_boot_stack +
>>> STACK_SIZE,
>>> - cont_after_mmu_is_enabled);
>>> + for ( pgtbl = stage1_pgtbl_root, i = 0;
>>> + i <= (CONFIG_PAGING_LEVELS - 1);
>>
>> i < CONFIG_PAGING_LEVELS ? But maybe it would be easier for i to
>> start
>> at CONFIG_PAGING_LEVELS and be decremented, simplifying ...
>>
>>> + i++ )
>>> + {
>>> + index = pt_index(CONFIG_PAGING_LEVELS - 1 - i, load_addr);
>>> + xen_index = pt_index(CONFIG_PAGING_LEVELS - 1 - i,
>>> XEN_VIRT_START);
>>
>> ... these two expressions?
> It makes sense. I'll update this part of the code.
>
>>
>>> + if ( index != xen_index )
>>> + {
>>> + pgtbl[index].pte = 0;
>>> + break;
>>> + }
>>
>> Is this enough? When load and link address are pretty close (but not
>> overlapping), can't they share a leaf table, in which case you need
>> to clear more than just a single entry? The present overlap check
>> looks to be 4k-granular, not 2M (in which case this couldn't happen;
>> IOW adjusting the overlap check may also be a way out).
> At the start of setup_initial_pagetables() there is a code which checks
> that load and link address don't overlap:
>
> if ( (linker_start != load_start) &&
> (linker_start <= load_end) && (load_start <= linker_end) )
> {
> early_printk("(XEN) linker and load address ranges overlap\n");
> die();
> }
>
> So the closest difference between load and link address can be 4kb.
> Otherwise load and link address ranges are equal ( as we can't map less
> then 4kb).
>
> Let's take concrete examples:
> Load address range is 0x8020_0000 - 0x8020_0FFF
> Linker address range is 0x8020_1000 - 0x8020_1FFF
> MMU mode: Sv39 ( so we have 3 page tables )
>
> So we have:
> * L2 index = 2, L1 index = 1, L0 index = 0 for load address
> * L2 index = 2, L1 index = 1, L0 index = 1 for linker address
> Thereby we have two different L0 tables for load and linker address
> ranges.
> And it looks like it is pretty safe to remove only one L0 index that
> was used for identity mapping.
>
> Is it possible that I missed something?
Looks as if you are thinking of only a Xen which fits in 4k. The code
here wants to cope with Xen getting quite a bit bigger.
Jan
