Re: [PATCH v2 5/8] xen/riscv: introduce asm/pmap.h header

[Julien Grall <julien@xen.org>]

Hi,

On 22/07/2024 15:44, Oleksii Kurochko wrote:
> On Mon, 2024-07-22 at 14:54 +0200, Jan Beulich wrote:
>> On 12.07.2024 18:22, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/pmap.h
>>> @@ -0,0 +1,28 @@
>>> +#ifndef __ASM_PMAP_H__
>>> +#define __ASM_PMAP_H__
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/mm.h>
>>> +
>>> +#include <asm/fixmap.h>
>>> +
>>> +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);
>>> +}
>>
>> Why are these not using set_fixmap() / clear_fixmap() respectively?
> They haven't been introduced yet. And I thought that these fucntion are
> used only in pmap_{un}map() and that is the reason why I decided to not
> introduce them. But while writing the answer on another comment, I
> found other places where set_fixmap() / clear_fixmap() are used, so I
> will introduce them and reuse here.

I am guessing you are going to implement set_fixmap()/clear_fixmap() 
using map_pages_to_xen(). If so, for early boot you are going to end up 
in a circular loop because map_pages_to_xen() will likely use pmap() 
which will call set_fixmap().

There is a big comment in common/pmap.c which explain why arch_pmap_* 
was introduced rather than calling *_fixmap() directly:

     /*
      * We cannot use set_fixmap() here. We use PMAP when the domain map
      * page infrastructure is not yet initialized, so 
map_pages_to_xen() called
      * by set_fixmap() needs to map pages on demand, which then calls 
pmap()
      * again, resulting in a loop. Modify the PTEs directly instead. 
The same
      * is true for pmap_unmap().
      */

Cheers,

-- 
Julien Grall