Re: [PATCH v2 4/8] xen/riscv: setup fixmap mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2024 16:36, Oleksii wrote:
> On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote:
>> On 12.07.2024 18:22, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/config.h
>>> +++ b/xen/arch/riscv/include/asm/config.h
>>> @@ -74,11 +74,20 @@
>>>  #error "unsupported RV_STAGE1_MODE"
>>>  #endif
>>>  
>>> +#define XEN_SIZE                MB(2)
>>> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
>>> +
>>> +#define BOOT_FDT_VIRT_START     XEN_VIRT_END
>>> +#define BOOT_FDT_VIRT_SIZE      MB(4)
>>> +
>>>  #define DIRECTMAP_SLOT_END      509
>>>  #define DIRECTMAP_SLOT_START    200
>>>  #define DIRECTMAP_VIRT_START    SLOTN(DIRECTMAP_SLOT_START)
>>>  #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END) -
>>> SLOTN(DIRECTMAP_SLOT_START))
>>>  
>>> +#define FIXMAP_BASE             (BOOT_FDT_VIRT_START +
>>> BOOT_FDT_VIRT_SIZE)
>>> +#define FIXMAP_ADDR(n)          (FIXMAP_BASE + (n) * PAGE_SIZE)
>>
>> Why exactly do you insert this here, and not adjacent to
>> BOOT_FDT_VIRT_*,
>> which it actually is adjacent with?
> I tried to follow alphabetical order.

Oh, X before B (just making fun) ... Anyway, my take here is that sorting
by address is going to be more helpful.

>>> --- a/xen/arch/riscv/mm.c
>>> +++ b/xen/arch/riscv/mm.c
>>> @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES];
>>>  pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
>>>  stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES];
>>>  
>>> +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
>>> +xen_fixmap[PAGETABLE_ENTRIES];
>>
>> Any reason this cannot be static?
> It will be used by pmap:
>    static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>    {
>        pte_t *entry = &xen_fixmap[slot];
>        pte_t pte;
>    
>        ASSERT(!pte_is_valid(*entry));
>    
>        pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
>        pte.pte |= PTE_LEAF_DEFAULT;
>        write_pte(entry, pte);
>    }
>    
>    static inline void arch_pmap_unmap(unsigned int slot)
>    {
>        pte_t pte = {};
>    
>        write_pte(&xen_fixmap[slot], pte);
>    }

Yet as asked there - shouldn't that be set_fixmap() and clear_fixmap()?

Jan