Set up fixmap mappings and the L0 page table for fixmap support.

Define new macros in riscv/config.h for calculating
the FIXMAP_BASE address, including BOOT_FDT_VIRT_{START, SIZE},
XEN_VIRT_SIZE, and XEN_VIRT_END.

Update the check for Xen size in riscv/lds.S to use
XEN_VIRT_SIZE instead of a hardcoded constant.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 - move definition of FIXMAP_ADDR() to asm/fixmap.h
 - add gap size equal to 2 MB ( 512 * 4K one page table entry in L1 page table )
   between Xen, FDT and Fixmap.
 - drop the comment for FIX_LAST.
 - move +1 from FIX_LAST definition to FIXADDR_TOP to be aligned with Arm.
   ( probably everything below FIX_LAST will be moved to a separate header in asm/generic.h )
 - correct the "changes in V4: s/'fence r,r'/'fence rw, rw'
 - use write_atomic() in set_pte().
 - introduce read_pte().
---
Changes in V4:
 - move definitions of XEN_VIRT_SIZE, BOOT_FDT_VIRT_{START,SIZE}, FIXMAP_{BASE,ADDR}
   below XEN_VIRT_START to have definitions appear in order.
 - define FIX_LAST as (FIX_MISC + 1) to have a guard slot at the end.
 - s/enumerated/numbered in the comment
 - update the cycle which looks for L1 page table in setup_fixmap_mapping_function() and
   the comment above him.
 - drop fences inside write_pte() and put 'fence rw,rw' in setup_fixmap() before sfence_vma().
 - update the commit message
 - drop printk message inside setup_fixmap().
---
Changes in V3:
 - s/XEN_SIZE/XEN_VIRT_SIZE
 - drop usage of XEN_VIRT_END.
 - sort newly introduced defines in config.h by address
 - code style fixes
 - drop runtime check of that pte is valid as it was checked in L1 page table finding cycle by BUG_ON().
 - update implementation of write_pte() with FENCE rw, rw.
 - add BUILD_BUG_ON() to check that amount of entries aren't bigger then entries in page table.
 - drop set_fixmap, clear_fixmap declarations as they aren't used and defined now
 - update the commit message.
 - s/__ASM_FIXMAP_H/ASM_FIXMAP_H
 - add SPDX-License-Identifier: GPL-2.0 
---
 xen/arch/riscv/include/asm/config.h | 15 ++++++++--
 xen/arch/riscv/include/asm/fixmap.h | 46 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/mm.h     |  2 ++
 xen/arch/riscv/include/asm/page.h   | 13 ++++++++
 xen/arch/riscv/mm.c                 | 43 +++++++++++++++++++++++++++
 xen/arch/riscv/setup.c              |  2 ++
 xen/arch/riscv/xen.lds.S            |  2 +-
 7 files changed, 120 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/fixmap.h

diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 50583aafdc..f55d6c45da 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -41,8 +41,10 @@
  * Start addr          | End addr         | Slot       | area description
  * ============================================================================
  *                   .....                 L2 511          Unused
- *  0xffffffffc0600000  0xffffffffc0800000 L2 511          Fixmap
- *  0xffffffffc0200000  0xffffffffc0600000 L2 511          FDT
+ *  0xffffffffc0A00000  0xffffffffc0C00000 L2 511          Fixmap
+ *                   ..... ( 2 MB gap )
+ *  0xffffffffc0400000  0xffffffffc0800000 L2 511          FDT
+ *                   ..... ( 2 MB gap )
  *  0xffffffffc0000000  0xffffffffc0200000 L2 511          Xen
  *                   .....                 L2 510          Unused
  *  0x3200000000        0x7f40000000       L2 200-509      Direct map
@@ -74,6 +76,15 @@
 #error "unsupported RV_STAGE1_MODE"
 #endif
 
+#define GAP_SIZE                MB(2)
+
+#define XEN_VIRT_SIZE           MB(2)
+
+#define BOOT_FDT_VIRT_START     (XEN_VIRT_START + XEN_VIRT_SIZE + GAP_SIZE)
+#define BOOT_FDT_VIRT_SIZE      MB(4)
+
+#define FIXMAP_BASE             (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE + GAP_SIZE)
+
 #define DIRECTMAP_SLOT_END      509
 #define DIRECTMAP_SLOT_START    200
 #define DIRECTMAP_VIRT_START    SLOTN(DIRECTMAP_SLOT_START)
diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
new file mode 100644
index 0000000000..63732df36c
--- /dev/null
+++ b/xen/arch/riscv/include/asm/fixmap.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * fixmap.h: compile-time virtual memory allocation
+ */
+#ifndef ASM_FIXMAP_H
+#define ASM_FIXMAP_H
+
+#include <xen/bug.h>
+#include <xen/page-size.h>
+#include <xen/pmap.h>
+
+#include <asm/page.h>
+
+#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE)
+
+/* Fixmap slots */
+#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
+#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */
+#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of hardware */
+
+#define FIX_LAST FIX_MISC
+
+#define FIXADDR_START FIXMAP_ADDR(0)
+#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1)
+
+#ifndef __ASSEMBLY__
+
+/*
+ * Direct access to xen_fixmap[] should only happen when {set,
+ * clear}_fixmap() is unusable (e.g. where we would end up to
+ * recursively call the helpers).
+ */
+extern pte_t xen_fixmap[];
+
+#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
+
+static inline unsigned int virt_to_fix(vaddr_t vaddr)
+{
+    BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
+
+    return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
+}
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* ASM_FIXMAP_H */
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 25af9e1aaa..a0bdc2bc3a 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -255,4 +255,6 @@ static inline unsigned int arch_get_dma_bitsize(void)
     return 32; /* TODO */
 }
 
+void setup_fixmap_mappings(void);
+
 #endif /* _ASM_RISCV_MM_H */
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
index c831e16417..a7419b93b2 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -9,6 +9,7 @@
 #include <xen/bug.h>
 #include <xen/types.h>
 
+#include <asm/atomic.h>
 #include <asm/mm.h>
 #include <asm/page-bits.h>
 
@@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
     BUG_ON("unimplemented");
 }
 
+/* Write a pagetable entry. */
+static inline void write_pte(pte_t *p, pte_t pte)
+{
+    write_atomic(p, pte);
+}
+
+/* Read a pagetable entry. */
+static inline pte_t read_pte(pte_t *p)
+{
+    return read_atomic(p);
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PAGE_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 7d09e781bf..b8ff91cf4e 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -12,6 +12,7 @@
 #include <asm/early_printk.h>
 #include <asm/csr.h>
 #include <asm/current.h>
+#include <asm/fixmap.h>
 #include <asm/page.h>
 #include <asm/processor.h>
 
@@ -49,6 +50,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];
+
 #define HANDLE_PGTBL(curr_lvl_num)                                          \
     index = pt_index(curr_lvl_num, page_addr);                              \
     if ( pte_is_valid(pgtbl[index]) )                                       \
@@ -191,6 +195,45 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
     return is_mode_supported;
 }
 
+void __init setup_fixmap_mappings(void)
+{
+    pte_t *pte, tmp;
+    unsigned int i;
+
+    BUILD_BUG_ON(FIX_LAST >= PAGETABLE_ENTRIES);
+
+    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
+
+    /*
+     * In RISC-V page table levels are numbered from Lx to L0 where
+     * x is the highest page table level for currect  MMU mode ( for example,
+     * for Sv39 has 3 page tables so the x = 2 (L2 -> L1 -> L0) ).
+     *
+     * In this cycle we want to find L1 page table because as L0 page table
+     * xen_fixmap[] will be used.
+     */
+    for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; )
+    {
+        BUG_ON(!pte_is_valid(*pte));
+
+        pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte));
+        pte = &pte[pt_index(i, FIXMAP_ADDR(0))];
+    }
+
+    BUG_ON(pte_is_valid(*pte));
+
+    tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
+    write_pte(pte, tmp);
+
+    RISCV_FENCE(rw, rw);
+    sfence_vma();
+
+    /*
+     * We only need the zeroeth table allocated, but not the PTEs set, because
+     * set_fixmap() will set them on the fly.
+     */
+}
+
 /*
  * setup_initial_pagetables:
  *
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 4defad68f4..13f0e8c77d 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
     test_macros_from_bug_h();
 #endif
 
+    setup_fixmap_mappings();
+
     printk("All set up\n");
 
     for ( ;; )
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 070b19d915..7a683f6065 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
  * Changing the size of Xen binary can require an update of
  * PGTBL_INITIAL_COUNT.
  */
-ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
+ASSERT(_end - _start <= XEN_VIRT_SIZE, "Xen too large for early-boot assumptions")
 
 ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");
-- 
2.46.0

On 21.08.2024 18:06, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -41,8 +41,10 @@
>   * Start addr          | End addr         | Slot       | area description
>   * ============================================================================
>   *                   .....                 L2 511          Unused
> - *  0xffffffffc0600000  0xffffffffc0800000 L2 511          Fixmap
> - *  0xffffffffc0200000  0xffffffffc0600000 L2 511          FDT
> + *  0xffffffffc0A00000  0xffffffffc0C00000 L2 511          Fixmap

Nit: Please can you avoid using mixed case in numbers?

> @@ -74,6 +76,15 @@
>  #error "unsupported RV_STAGE1_MODE"
>  #endif
>  
> +#define GAP_SIZE                MB(2)
> +
> +#define XEN_VIRT_SIZE           MB(2)
> +
> +#define BOOT_FDT_VIRT_START     (XEN_VIRT_START + XEN_VIRT_SIZE + GAP_SIZE)
> +#define BOOT_FDT_VIRT_SIZE      MB(4)
> +
> +#define FIXMAP_BASE             (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE + GAP_SIZE)

Nit: Overly long line.

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fixmap.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * fixmap.h: compile-time virtual memory allocation
> + */
> +#ifndef ASM_FIXMAP_H
> +#define ASM_FIXMAP_H
> +
> +#include <xen/bug.h>
> +#include <xen/page-size.h>
> +#include <xen/pmap.h>
> +
> +#include <asm/page.h>
> +
> +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE)
> +
> +/* Fixmap slots */
> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */
> +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of hardware */
> +
> +#define FIX_LAST FIX_MISC
> +
> +#define FIXADDR_START FIXMAP_ADDR(0)
> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1)
> +
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Direct access to xen_fixmap[] should only happen when {set,
> + * clear}_fixmap() is unusable (e.g. where we would end up to
> + * recursively call the helpers).
> + */
> +extern pte_t xen_fixmap[];

I'm afraid I keep being irritated by the comment: What recursive use of
helpers is being talked about here? I can't see anything recursive in this
patch. If this starts happening with a subsequent patch, then you have
two options: Move the declaration + comment there, or clarify in the
description (in enough detail) what this is about.

> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
>      BUG_ON("unimplemented");
>  }
>  
> +/* Write a pagetable entry. */
> +static inline void write_pte(pte_t *p, pte_t pte)
> +{
> +    write_atomic(p, pte);
> +}
> +
> +/* Read a pagetable entry. */
> +static inline pte_t read_pte(pte_t *p)
> +{
> +    return read_atomic(p);

This only works because of the strange type trickery you're playing in
read_atomic(). Look at x86 code - there's a strict expectation that the
type can be converted to/from unsigned long. And page table accessors
are written with that taken into consideration. Same goes for write_pte()
of course, with the respective comment on the earlier patch in mind.

Otoh I see that Arm does something very similar. If you have a strong
need / desire to follow that, then please at least split the two
entirely separate aspects that patch 1 presently changes both in one go.

Jan

On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
> > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned
> > long mfn, bool sync_icache)
> >       BUG_ON("unimplemented");
> >   }
> >   
> > +/* Write a pagetable entry. */
> > +static inline void write_pte(pte_t *p, pte_t pte)
> > +{
> > +    write_atomic(p, pte);
> > +}
> > +
> > +/* Read a pagetable entry. */
> > +static inline pte_t read_pte(pte_t *p)
> > +{
> > +    return read_atomic(p);
> 
> This only works because of the strange type trickery you're playing
> in
> read_atomic(). Look at x86 code - there's a strict expectation that
> the
> type can be converted to/from unsigned long. And page table accessors
> are written with that taken into consideration. Same goes for
> write_pte()
> of course, with the respective comment on the earlier patch in mind.
> 
> Otoh I see that Arm does something very similar. If you have a strong
> need / desire to follow that, then please at least split the two
> entirely separate aspects that patch 1 presently changes both in one
> go.
I am not 100% sure that type trick could be dropped easily for RISC-V:
1. I still need the separate C function for proper #ifdef-ing:
   #ifndef CONFIG_RISCV_32
       case 8: *(uint32_t *)res = readq_cpu(p); break;
   #endif
   
2. Because of the point 1 the change should be as following:
   -#define read_atomic(p) ({                                   \
   -    union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_;   \
   -    read_atomic_size(p, x_.c, sizeof(*(p)));                \
   -    x_.val;                                                 \
   +#define read_atomic(p) ({                                 \
   +    unsigned long x_;                                     \
   +    read_atomic_size(p, &x_, sizeof(*(p)));               \
   +    (typeof(*(p)))x_;                                     \
    })
   But after that I think it will be an error: "conversion to non-scalar
   type requested" in the last line as *p points to pte_t.
   
   and we can't just in read_pte() change to:
   static inline pte_t read_pte(pte_t *p)
   {
       return read_atomic(&p->pte);
   }
   As in this cases it started it will return unsigned long but function
   expects pte_t. As an option read_pte() can be updated to:
   /* Read a pagetable entry. */
   static inline pte_t read_pte(pte_t *p)
   {
       return (pte_t) { .pte = read_atomic(&p->pte) };
   }
   
   But I am not sure that it is better then just have a union trick inside
   read_atomic() and then just have read_atomic(p) for read_pte().
   
   Am I missing something?
   
~ Oleksii

On 30.08.2024 13:55, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
>>> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned
>>> long mfn, bool sync_icache)
>>>       BUG_ON("unimplemented");
>>>   }
>>>   
>>> +/* Write a pagetable entry. */
>>> +static inline void write_pte(pte_t *p, pte_t pte)
>>> +{
>>> +    write_atomic(p, pte);
>>> +}
>>> +
>>> +/* Read a pagetable entry. */
>>> +static inline pte_t read_pte(pte_t *p)
>>> +{
>>> +    return read_atomic(p);
>>
>> This only works because of the strange type trickery you're playing
>> in
>> read_atomic(). Look at x86 code - there's a strict expectation that
>> the
>> type can be converted to/from unsigned long. And page table accessors
>> are written with that taken into consideration. Same goes for
>> write_pte()
>> of course, with the respective comment on the earlier patch in mind.
>>
>> Otoh I see that Arm does something very similar. If you have a strong
>> need / desire to follow that, then please at least split the two
>> entirely separate aspects that patch 1 presently changes both in one
>> go.
> I am not 100% sure that type trick could be dropped easily for RISC-V:
> 1. I still need the separate C function for proper #ifdef-ing:
>    #ifndef CONFIG_RISCV_32
>        case 8: *(uint32_t *)res = readq_cpu(p); break;
>    #endif
>    
> 2. Because of the point 1 the change should be as following:
>    -#define read_atomic(p) ({                                   \
>    -    union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_;   \
>    -    read_atomic_size(p, x_.c, sizeof(*(p)));                \
>    -    x_.val;                                                 \
>    +#define read_atomic(p) ({                                 \
>    +    unsigned long x_;                                     \
>    +    read_atomic_size(p, &x_, sizeof(*(p)));               \
>    +    (typeof(*(p)))x_;                                     \
>     })
>    But after that I think it will be an error: "conversion to non-scalar
>    type requested" in the last line as *p points to pte_t.
>    
>    and we can't just in read_pte() change to:
>    static inline pte_t read_pte(pte_t *p)
>    {
>        return read_atomic(&p->pte);
>    }
>    As in this cases it started it will return unsigned long but function
>    expects pte_t.

Of course.

> As an option read_pte() can be updated to:
>    /* Read a pagetable entry. */
>    static inline pte_t read_pte(pte_t *p)
>    {
>        return (pte_t) { .pte = read_atomic(&p->pte) };
>    }

That's what's needed.

>    But I am not sure that it is better then just have a union trick inside
>    read_atomic() and then just have read_atomic(p) for read_pte().

It's largely up to you. My main request is that things end up / remain
consistent. Which way round is secondary, and often merely a matter of
suitably justifying the choice made.

Jan

On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
> > 
> > +
> > +/*
> > + * Direct access to xen_fixmap[] should only happen when {set,
> > + * clear}_fixmap() is unusable (e.g. where we would end up to
> > + * recursively call the helpers).
> > + */
> > +extern pte_t xen_fixmap[];
> 
> I'm afraid I keep being irritated by the comment: What recursive use
> of
> helpers is being talked about here? I can't see anything recursive in
> this
> patch. If this starts happening with a subsequent patch, then you
> have
> two options: Move the declaration + comment there, or clarify in the
> description (in enough detail) what this is about.
This comment is added because of:
```
void *__init pmap_map(mfn_t mfn)
  ...
       /*
        * 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().
        */
       arch_pmap_map(slot, mfn);
   ...
```
And it happens because set_fixmap() could be defined using generic PT
helpers so what will lead to recursive behaviour when when there is no
direct map:
   static pte_t *map_table(mfn_t mfn)
   {
       /*
        * During early boot, map_domain_page() may be unusable. Use the
        * PMAP to map temporarily a page-table.
        */
       if ( system_state == SYS_STATE_early_boot )
           return pmap_map(mfn);
       ...
   }

> 
> > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned
> > long mfn, bool sync_icache)
> >      BUG_ON("unimplemented");
> >  }
> >  
> > +/* Write a pagetable entry. */
> > +static inline void write_pte(pte_t *p, pte_t pte)
> > +{
> > +    write_atomic(p, pte);
> > +}
> > +
> > +/* Read a pagetable entry. */
> > +static inline pte_t read_pte(pte_t *p)
> > +{
> > +    return read_atomic(p);
> 
> This only works because of the strange type trickery you're playing
> in
> read_atomic(). Look at x86 code - there's a strict expectation that
> the
> type can be converted to/from unsigned long. And page table accessors
> are written with that taken into consideration. Same goes for
> write_pte()
> of course, with the respective comment on the earlier patch in mind.
I will check x86 code. Probably my answer on the patch with
read/write_atomic() suggest that too. Based on the answers to that
patch I think we can go with x86 approach.

Thanks.

~ Oleksii

> 
> Otoh I see that Arm does something very similar. If you have a strong
> need / desire to follow that, then please at least split the two
> entirely separate aspects that patch 1 presently changes both in one
> go.
> 
> Jan

On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
>>>
>>> +
>>> +/*
>>> + * Direct access to xen_fixmap[] should only happen when {set,
>>> + * clear}_fixmap() is unusable (e.g. where we would end up to
>>> + * recursively call the helpers).
>>> + */
>>> +extern pte_t xen_fixmap[];
>>
>> I'm afraid I keep being irritated by the comment: What recursive use
>> of
>> helpers is being talked about here? I can't see anything recursive in
>> this
>> patch. If this starts happening with a subsequent patch, then you
>> have
>> two options: Move the declaration + comment there, or clarify in the
>> description (in enough detail) what this is about.
> This comment is added because of:
> ```
> void *__init pmap_map(mfn_t mfn)
>   ...
>        /*
>         * 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().
>         */
>        arch_pmap_map(slot, mfn);
>    ...
> ```
> And it happens because set_fixmap() could be defined using generic PT
> helpers

As you say - could be. If I'm not mistaken no set_fixmap() implementation
exists even by the end of the series. Fundamentally I'd expect set_fixmap()
to (possibly) use xen_fixmap[] directly. That in turn ...

> so what will lead to recursive behaviour when when there is no
> direct map:

... would mean no recursion afaict. Hence why clarification is needed as
to what's going on here _and_ what's planned.

Jan

>    static pte_t *map_table(mfn_t mfn)
>    {
>        /*
>         * During early boot, map_domain_page() may be unusable. Use the
>         * PMAP to map temporarily a page-table.
>         */
>        if ( system_state == SYS_STATE_early_boot )
>            return pmap_map(mfn);
>        ...
>    }

On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote:
> On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
> > > > 
> > > > +
> > > > +/*
> > > > + * Direct access to xen_fixmap[] should only happen when {set,
> > > > + * clear}_fixmap() is unusable (e.g. where we would end up to
> > > > + * recursively call the helpers).
> > > > + */
> > > > +extern pte_t xen_fixmap[];
> > > 
> > > I'm afraid I keep being irritated by the comment: What recursive
> > > use
> > > of
> > > helpers is being talked about here? I can't see anything
> > > recursive in
> > > this
> > > patch. If this starts happening with a subsequent patch, then you
> > > have
> > > two options: Move the declaration + comment there, or clarify in
> > > the
> > > description (in enough detail) what this is about.
We can't move declaration of xen_fixmap[] to the patch where
set_fixmap() will be introduced ( which uses PMAP for domain map page
infrastructure ) as xen_fixmap[] is used in the current patch.

And we can't properly provide the proper description with the function
which will be introduced one day in the future ( what can be not good
too ). I came up with the following description in the comment above
xen_fixmap[] declaration:
   /*
    * Direct access to xen_fixmap[] should only happen when {set,
    * clear}_fixmap() is unusable (e.g. where we would end up to
    * recursively call the helpers).
    * 
    * One such case is pmap_map() where set_fixmap() can not be used.
    * It happens because PMAP is used 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.
    * Modification of the PTEs directly instead in arch_pmap_map().
    * The same is true for pmap_unmap().
    */

Could it be an option just to drop the comment for now at all as at the
moment there is no such restriction with the usage of
{set,clear}_fixmap() and xen_fixmap[]?

> > This comment is added because of:
> > ```
> > void *__init pmap_map(mfn_t mfn)
> >   ...
> >        /*
> >         * 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().
> >         */
> >        arch_pmap_map(slot, mfn);
> >    ...
> > ```
> > And it happens because set_fixmap() could be defined using generic
> > PT
> > helpers
> 
> As you say - could be. If I'm not mistaken no set_fixmap()
> implementation
> exists even by the end of the series. Fundamentally I'd expect
> set_fixmap()
> to (possibly) use xen_fixmap[] directly. That in turn ...
> 
> > so what will lead to recursive behaviour when when there is no
> > direct map:
> 
> ... would mean no recursion afaict. Hence why clarification is needed
> as
> to what's going on here _and_ what's planned.
Yes, it is true. No recursion will happen in this case but if to look
at the implementation of set_fixmap() for other Arm or x86 ( but I am
not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using
map_pages_to_xen().

~ Oleksii

> 
> >    static pte_t *map_table(mfn_t mfn)
> >    {
> >        /*
> >         * During early boot, map_domain_page() may be unusable. Use
> > the
> >         * PMAP to map temporarily a page-table.
> >         */
> >        if ( system_state == SYS_STATE_early_boot )
> >            return pmap_map(mfn);
> >        ...
> >    }
> 

On 30.08.2024 13:01, oleksii.kurochko@gmail.com wrote:
> On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote:
>> On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote:
>>> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
>>>>>
>>>>> +
>>>>> +/*
>>>>> + * Direct access to xen_fixmap[] should only happen when {set,
>>>>> + * clear}_fixmap() is unusable (e.g. where we would end up to
>>>>> + * recursively call the helpers).
>>>>> + */
>>>>> +extern pte_t xen_fixmap[];
>>>>
>>>> I'm afraid I keep being irritated by the comment: What recursive
>>>> use
>>>> of
>>>> helpers is being talked about here? I can't see anything
>>>> recursive in
>>>> this
>>>> patch. If this starts happening with a subsequent patch, then you
>>>> have
>>>> two options: Move the declaration + comment there, or clarify in
>>>> the
>>>> description (in enough detail) what this is about.
> We can't move declaration of xen_fixmap[] to the patch where
> set_fixmap() will be introduced ( which uses PMAP for domain map page
> infrastructure ) as xen_fixmap[] is used in the current patch.
> 
> And we can't properly provide the proper description with the function
> which will be introduced one day in the future ( what can be not good
> too ). I came up with the following description in the comment above
> xen_fixmap[] declaration:
>    /*
>     * Direct access to xen_fixmap[] should only happen when {set,
>     * clear}_fixmap() is unusable (e.g. where we would end up to
>     * recursively call the helpers).
>     * 
>     * One such case is pmap_map() where set_fixmap() can not be used.
>     * It happens because PMAP is used 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.
>     * Modification of the PTEs directly instead in arch_pmap_map().
>     * The same is true for pmap_unmap().
>     */
> 
> Could it be an option just to drop the comment for now at all as at the
> moment there is no such restriction with the usage of
> {set,clear}_fixmap() and xen_fixmap[]?

The comment isn't the right place to explain things here, imo. It's the
patch description where unexpected aspects need shedding light on.

>>> This comment is added because of:
>>> ```
>>> void *__init pmap_map(mfn_t mfn)
>>>   ...
>>>        /*
>>>         * 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().
>>>         */
>>>        arch_pmap_map(slot, mfn);
>>>    ...
>>> ```
>>> And it happens because set_fixmap() could be defined using generic
>>> PT
>>> helpers
>>
>> As you say - could be. If I'm not mistaken no set_fixmap()
>> implementation
>> exists even by the end of the series. Fundamentally I'd expect
>> set_fixmap()
>> to (possibly) use xen_fixmap[] directly. That in turn ...
>>
>>> so what will lead to recursive behaviour when when there is no
>>> direct map:
>>
>> ... would mean no recursion afaict. Hence why clarification is needed
>> as
>> to what's going on here _and_ what's planned.
> Yes, it is true. No recursion will happen in this case but if to look
> at the implementation of set_fixmap() for other Arm or x86 ( but I am
> not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using
> map_pages_to_xen().

There's no PMAP so far on x86.

Jan