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

Commit Message

Oleksii Kurochko July 12, 2024, 4:22 p.m. UTC

Introduce a function to set up fixmap mappings and L0 page
table for fixmap.

Additionally, defines were introduced in riscv/config.h to
calculate the FIXMAP_BASE address.
This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
XEN_SIZE, XEN_VIRT_END.

Also, the check of Xen size was updated in the riscv/lds.S
script to use XEN_SIZE instead of a hardcoded constant.

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes in V2:
 - newly introduced patch
---
 xen/arch/riscv/include/asm/config.h |  9 ++++++
 xen/arch/riscv/include/asm/fixmap.h | 48 +++++++++++++++++++++++++++++
 xen/arch/riscv/include/asm/mm.h     |  2 ++
 xen/arch/riscv/include/asm/page.h   |  7 +++++
 xen/arch/riscv/mm.c                 | 35 +++++++++++++++++++++
 xen/arch/riscv/setup.c              |  2 ++
 xen/arch/riscv/xen.lds.S            |  2 +-
 7 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/riscv/include/asm/fixmap.h

Comments

Julien Grall July 21, 2024, 8:46 a.m. UTC | #1

Hi Oleksii,

On 12/07/2024 17:22, Oleksii Kurochko wrote:
> Introduce a function to set up fixmap mappings and L0 page
> table for fixmap.
> 
> Additionally, defines were introduced in riscv/config.h to
> calculate the FIXMAP_BASE address.
> This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
> XEN_SIZE, XEN_VIRT_END.
> 
> Also, the check of Xen size was updated in the riscv/lds.S
> script to use XEN_SIZE instead of a hardcoded constant.
> 
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
> Changes in V2:
>   - newly introduced patch
> ---
>   xen/arch/riscv/include/asm/config.h |  9 ++++++
>   xen/arch/riscv/include/asm/fixmap.h | 48 +++++++++++++++++++++++++++++
>   xen/arch/riscv/include/asm/mm.h     |  2 ++
>   xen/arch/riscv/include/asm/page.h   |  7 +++++
>   xen/arch/riscv/mm.c                 | 35 +++++++++++++++++++++
>   xen/arch/riscv/setup.c              |  2 ++
>   xen/arch/riscv/xen.lds.S            |  2 +-
>   7 files changed, 104 insertions(+), 1 deletion(-)
>   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..3275477c17 100644
> --- 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)

NIT: I would name it XEN_VIRT_SIZE to be consistent with the start/end.

> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
Can we get away with not introducing *_END and just use START, SIZE? The 
reason I am asking is with "end" it is never clear whether it is 
inclusive or exclusive. For instance, here you use an exclusive range 
but ...

> +
> +#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)
> +
>   #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct page_info))
>   #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1)
>   
> diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
> new file mode 100644
> index 0000000000..fcfb82d69c
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fixmap.h
> @@ -0,0 +1,48 @@
> +/*
> + * 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>
> +
> +/* Fixmap slots */
> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */

... here is seems to be inclusive. Furthermore if you had 32-bit address 
space, it is also quite easy to have to create a region right at the top 
of it. So when END is exclusive, it would become 0.

So on Arm, we decided to start to get rid of "end". I would consider to 
do the same on RISC-V for new functions.

> +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of hardware */

Are you going to use this fixmap? If not, then I would consider to 
remove it.

> +
> +#define FIX_LAST FIX_MISC
> +
> +#define FIXADDR_START FIXMAP_ADDR(0)
> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
> +
> +#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[];
> +
> +/* Map a page in a fixmap entry */
> +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int attributes);
> +/* Remove a mapping from a fixmap entry */
> +extern void clear_fixmap(unsigned int map);

Neither of the functions seem to be implemented in this patch. Can you 
clarify what's the plan for them?

Also, I know that for x86/arm, we have some function prefixed with 
extern. But AFAIK, we are trying to get rid of them.

In any case, I think for RISC-V we need some consistency. For instance, 
here you define with extern but...

> +
> +#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);

... here it is without.

> +
>   #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..cbbf3656d1 100644
> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -81,6 +81,13 @@ 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)
> +{
> +    *p = pte;
> +    asm volatile ("sfence.vma");
> +}
> +
>   #endif /* __ASSEMBLY__ */
>   
>   #endif /* _ASM_RISCV_PAGE_H */
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 7d09e781bf..d69a174b5d 100644
> --- 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];

Can you add a BUILD_BUG_ON() to check that the number of entries in the 
fixmap will never be above PAGETABLE_ENTRIES?

> +
>   #define HANDLE_PGTBL(curr_lvl_num)                                          \
>       index = pt_index(curr_lvl_num, page_addr);                              \
>       if ( pte_is_valid(pgtbl[index]) )                                       \
> @@ -191,6 +194,38 @@ 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;
> +    unsigned int i;
> +
> +    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
> +
> +    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )

I am a little bit confused with the - 1. Is this because you only want 
to map at L1 (I am not sure if this is the correct naming for RISC-V)?

In any case, I think it would be worth a comment.

> +    {
> +        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) );

Coding style: BUG_ON(pte_is_valid(*pte));

> +
> +    if ( !pte_is_valid(*pte) )

I am a bit confused with this check. Above, Xen will crash if the PTE is 
valid. So why do we need a runtime check?

> +    {
> +        pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
> +
> +        write_pte(pte, tmp);
> +
> +        printk("(XEN) fixmap is mapped\n");
> +    }
> +
> +    /*
> +     * We only need the zeroeth table allocated, but not the PTEs set, because
> +     * set_fixmap() will set them on the fly.

This function doesn't seem to exists yet (?).

> +     */
> +}
> +
>   /*
>    * 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..63b1dd7bb6 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_SIZE, "Xen too large for early-boot assumptions")
>   
>   ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");

Cheers,

Jan Beulich July 22, 2024, 12:42 p.m. UTC | #2

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? Then ...

>  #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct page_info))
>  #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1)

... this would also stay next to DIRECTMAP_*, which it uses.

> --- a/xen/arch/riscv/include/asm/page.h
> +++ b/xen/arch/riscv/include/asm/page.h
> @@ -81,6 +81,13 @@ 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)
> +{
> +    *p = pte;
> +    asm volatile ("sfence.vma");

When they don't have operands, asm()-s are volatile anyway (being explicit
about this may still be desirable, yes). But: Can you get away without
operands here? Don't you need a memory clobber for the fence to actually
remain where it is needed?

Also, nit (style): Blanks missing.

> --- 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?

Jan

Oleksii Kurochko July 22, 2024, 2:31 p.m. UTC | #3

Hi Julien,

On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> On 12/07/2024 17:22, Oleksii Kurochko wrote:
> > Introduce a function to set up fixmap mappings and L0 page
> > table for fixmap.
> > 
> > Additionally, defines were introduced in riscv/config.h to
> > calculate the FIXMAP_BASE address.
> > This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
> > XEN_SIZE, XEN_VIRT_END.
> > 
> > Also, the check of Xen size was updated in the riscv/lds.S
> > script to use XEN_SIZE instead of a hardcoded constant.
> > 
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > ---
> > Changes in V2:
> >   - newly introduced patch
> > ---
> >   xen/arch/riscv/include/asm/config.h |  9 ++++++
> >   xen/arch/riscv/include/asm/fixmap.h | 48
> > +++++++++++++++++++++++++++++
> >   xen/arch/riscv/include/asm/mm.h     |  2 ++
> >   xen/arch/riscv/include/asm/page.h   |  7 +++++
> >   xen/arch/riscv/mm.c                 | 35 +++++++++++++++++++++
> >   xen/arch/riscv/setup.c              |  2 ++
> >   xen/arch/riscv/xen.lds.S            |  2 +-
> >   7 files changed, 104 insertions(+), 1 deletion(-)
> >   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..3275477c17 100644
> > --- 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)
> 
> NIT: I would name it XEN_VIRT_SIZE to be consistent with the
> start/end.
> 
> > +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
> Can we get away with not introducing *_END and just use START, SIZE?
> The 
> reason I am asking is with "end" it is never clear whether it is 
> inclusive or exclusive. For instance, here you use an exclusive range
> but ...
> 
> > +
> > +#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)
> > +
> >   #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct
> > page_info))
> >   #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) /
> > FRAMETABLE_SCALE_FACTOR) + 1)
> >   
> > diff --git a/xen/arch/riscv/include/asm/fixmap.h
> > b/xen/arch/riscv/include/asm/fixmap.h
> > new file mode 100644
> > index 0000000000..fcfb82d69c
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/fixmap.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * 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>
> > +
> > +/* Fixmap slots */
> > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of
> > PMAP */
> 
> ... here is seems to be inclusive. Furthermore if you had 32-bit
> address 
> space, it is also quite easy to have to create a region right at the
> top 
> of it. So when END is exclusive, it would become 0.
> 
> So on Arm, we decided to start to get rid of "end". I would consider
> to 
> do the same on RISC-V for new functions.
I will refactor the code and get rid of "end".

> 
> > +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of
> > hardware */
> 
> Are you going to use this fixmap? If not, then I would consider to 
> remove it.
Yes, it used now in copy_from_paddr():
   /**
    * copy_from_paddr - copy data from a physical address
    * @dst: destination virtual address
    * @paddr: source physical address
    * @len: length to copy
    */
   void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long
   len)
   {
       void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
   
       while (len) {
           unsigned long l, s;
   
           s = paddr & (PAGE_SIZE-1);
           l = min(PAGE_SIZE - s, len);
   
           set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr),
   PAGE_HYPERVISOR_WC);
           memcpy(dst, src + s, l);
           clear_fixmap(FIXMAP_MISC);
   
           paddr += l;
           dst += l;
           len -= l;
       }
   }

> 
> > +
> > +#define FIX_LAST FIX_MISC
> > +
> > +#define FIXADDR_START FIXMAP_ADDR(0)
> > +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
> > +
> > +#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[];
> > +
> > +/* Map a page in a fixmap entry */
> > +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int
> > attributes);
> > +/* Remove a mapping from a fixmap entry */
> > +extern void clear_fixmap(unsigned int map);
> 
> Neither of the functions seem to be implemented in this patch. Can
> you 
> clarify what's the plan for them?
You are right, it could be dropped now. But in future this functions
are used for copy_from_paddr(). Look at the code above.


> 
> Also, I know that for x86/arm, we have some function prefixed with 
> extern. But AFAIK, we are trying to get rid of them.
> 
> In any case, I think for RISC-V we need some consistency. For
> instance, 
> here you define with extern but...
> 
> > +
> > +#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);
> 
> ... here it is without.
> 
> > +
> >   #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..cbbf3656d1 100644
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -81,6 +81,13 @@ 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)
> > +{
> > +    *p = pte;
> > +    asm volatile ("sfence.vma");
> > +}
> > +
> >   #endif /* __ASSEMBLY__ */
> >   
> >   #endif /* _ASM_RISCV_PAGE_H */
> > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > index 7d09e781bf..d69a174b5d 100644
> > --- 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];
> 
> Can you add a BUILD_BUG_ON() to check that the number of entries in
> the 
> fixmap will never be above PAGETABLE_ENTRIES?
Sure. What is the best place? Somewhere in setup_fixmap_mappings()?

> 
> > +
> >   #define
> > HANDLE_PGTBL(curr_lvl_num)                                         
> > \
> >       index = pt_index(curr_lvl_num,
> > page_addr);                              \
> >       if ( pte_is_valid(pgtbl[index])
> > )                                       \
> > @@ -191,6 +194,38 @@ 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;
> > +    unsigned int i;
> > +
> > +    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL,
> > FIXMAP_ADDR(0))];
> > +
> > +    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )
> 
> I am a little bit confused with the - 1. Is this because you only
> want 
> to map at L1 (I am not sure if this is the correct naming for RISC-
> V)?
Yes, the idea is that I want to stop in L1 ( 2Mb pages ) as this
mapping is already exist and there will not be need to create a new
table. ( what will fail because boot allocator isn't initialized yet
and alloc_boot_pages() will start to alarm because of
BUG_ON(!nr_bootmem_regions) ).

RISC-V also uses word levels, but the order is an opposite to Arm.

> 
> In any case, I think it would be worth a comment.
Sure, I will add it.


> 
> > +    {
> > +        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) );
> 
> Coding style: BUG_ON(pte_is_valid(*pte));
> 
> > +
> > +    if ( !pte_is_valid(*pte) )
> 
> I am a bit confused with this check. Above, Xen will crash if the PTE
> is 
> valid. So why do we need a runtime check?
You are right, there is no any sense. We should drop it.

> 
> > +    {
> > +        pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned
> > long)&xen_fixmap), PTE_TABLE);
> > +
> > +        write_pte(pte, tmp);
> > +
> > +        printk("(XEN) fixmap is mapped\n");
> > +    }
> > +
> > +    /*
> > +     * We only need the zeroeth table allocated, but not the PTEs
> > set, because
> > +     * set_fixmap() will set them on the fly.
> 
> This function doesn't seem to exists yet (?).
Not yet. It will be introduced later...

~ Oleksii

> 
> > +     */
> > +}
> > +
> >   /*
> >    * 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..63b1dd7bb6 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_SIZE, "Xen too large for early-boot
> > assumptions")
> >   
> >   ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity
> > region is too big");
> 
> Cheers,
>

Oleksii Kurochko July 22, 2024, 2:36 p.m. UTC | #4

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.

>  Then ...
> 
> >  #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct
> > page_info))
> >  #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) /
> > FRAMETABLE_SCALE_FACTOR) + 1)
> 
> ... this would also stay next to DIRECTMAP_*, which it uses.
> 
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -81,6 +81,13 @@ 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)
> > +{
> > +    *p = pte;
> > +    asm volatile ("sfence.vma");
> 
> When they don't have operands, asm()-s are volatile anyway (being
> explicit
> about this may still be desirable, yes). But: Can you get away
> without
> operands here? Don't you need a memory clobber for the fence to
> actually
> remain where it is needed?
It should be "::memory" here. Thanks.

> 
> Also, nit (style): Blanks missing.
> 
> > --- 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);
   }

~ Oleksii

Julien Grall July 22, 2024, 2:44 p.m. UTC | #5

On 22/07/2024 15:31, Oleksii wrote:
> Hi Julien,

Hi Oleksii,

> On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
>> Hi Oleksii,
>>
>> On 12/07/2024 17:22, Oleksii Kurochko wrote:
>>> Introduce a function to set up fixmap mappings and L0 page
>>> table for fixmap.
>>>
>>> Additionally, defines were introduced in riscv/config.h to
>>> calculate the FIXMAP_BASE address.
>>> This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
>>> XEN_SIZE, XEN_VIRT_END.
>>>
>>> Also, the check of Xen size was updated in the riscv/lds.S
>>> script to use XEN_SIZE instead of a hardcoded constant.
>>>
>>> Signed-off-by: Oleksii Kurochko <[e-mail]>
>>> ---
>>> Changes in V2:
>>>    - newly introduced patch
>>> ---
>>>    xen/arch/riscv/include/asm/config.h |  9 ++++++
>>>    xen/arch/riscv/include/asm/fixmap.h | 48
>>> +++++++++++++++++++++++++++++
>>>    xen/arch/riscv/include/asm/mm.h     |  2 ++
>>>    xen/arch/riscv/include/asm/page.h   |  7 +++++
>>>    xen/arch/riscv/mm.c                 | 35 +++++++++++++++++++++
>>>    xen/arch/riscv/setup.c              |  2 ++
>>>    xen/arch/riscv/xen.lds.S            |  2 +-
>>>    7 files changed, 104 insertions(+), 1 deletion(-)
>>>    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..3275477c17 100644
>>> --- 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)
>>
>> NIT: I would name it XEN_VIRT_SIZE to be consistent with the
>> start/end.
>>
>>> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
>> Can we get away with not introducing *_END and just use START, SIZE?
>> The
>> reason I am asking is with "end" it is never clear whether it is
>> inclusive or exclusive. For instance, here you use an exclusive range
>> but ...
>>
>>> +
>>> +#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)
>>> +
>>>    #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct
>>> page_info))
>>>    #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) /
>>> FRAMETABLE_SCALE_FACTOR) + 1)
>>>    
>>> diff --git a/xen/arch/riscv/include/asm/fixmap.h
>>> b/xen/arch/riscv/include/asm/fixmap.h
>>> new file mode 100644
>>> index 0000000000..fcfb82d69c
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/fixmap.h
>>> @@ -0,0 +1,48 @@
>>> +/*
>>> + * 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>
>>> +
>>> +/* Fixmap slots */
>>> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
>>> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of
>>> PMAP */
>>
>> ... here is seems to be inclusive. Furthermore if you had 32-bit
>> address
>> space, it is also quite easy to have to create a region right at the
>> top
>> of it. So when END is exclusive, it would become 0.
>>
>> So on Arm, we decided to start to get rid of "end". I would consider
>> to
>> do the same on RISC-V for new functions.
> I will refactor the code and get rid of "end".
> 
>>
>>> +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of
>>> hardware */
>>
>> Are you going to use this fixmap? If not, then I would consider to
>> remove it.
> Yes, it used now in copy_from_paddr():
>     /**
>      * copy_from_paddr - copy data from a physical address
>      * @dst: destination virtual address
>      * @paddr: source physical address
>      * @len: length to copy
>      */
>     void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long
>     len)
>     {
>         void *src = (void *)FIXMAP_ADDR(FIXMAP_MISC);
>     
>         while (len) {
>             unsigned long l, s;
>     
>             s = paddr & (PAGE_SIZE-1);
>             l = min(PAGE_SIZE - s, len);
>     
>             set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr),
>     PAGE_HYPERVISOR_WC);
>             memcpy(dst, src + s, l);
>             clear_fixmap(FIXMAP_MISC);
>     
>             paddr += l;
>             dst += l;
>             len -= l;
>         }
>     }
> 
>>
>>> +
>>> +#define FIX_LAST FIX_MISC
>>> +
>>> +#define FIXADDR_START FIXMAP_ADDR(0)
>>> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
>>> +
>>> +#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[];
>>> +
>>> +/* Map a page in a fixmap entry */
>>> +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int
>>> attributes);
>>> +/* Remove a mapping from a fixmap entry */
>>> +extern void clear_fixmap(unsigned int map);
>>
>> Neither of the functions seem to be implemented in this patch. Can
>> you
>> clarify what's the plan for them?
> You are right, it could be dropped now. But in future this functions
> are used for copy_from_paddr(). Look at the code above.

Right, to me it is just odd we are definition prototype for functions 
that don't yet exist.

>>
>> Also, I know that for x86/arm, we have some function prefixed with
>> extern. But AFAIK, we are trying to get rid of them.
>>
>> In any case, I think for RISC-V we need some consistency. For
>> instance,
>> here you define with extern but...
>>
>>> +
>>> +#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);
>>
>> ... here it is without.
>>
>>> +
>>>    #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..cbbf3656d1 100644
>>> --- a/xen/arch/riscv/include/asm/page.h
>>> +++ b/xen/arch/riscv/include/asm/page.h
>>> @@ -81,6 +81,13 @@ 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)
>>> +{
>>> +    *p = pte;
>>> +    asm volatile ("sfence.vma");
>>> +}
>>> +
>>>    #endif /* __ASSEMBLY__ */
>>>    
>>>    #endif /* _ASM_RISCV_PAGE_H */
>>> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
>>> index 7d09e781bf..d69a174b5d 100644
>>> --- 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];
>>
>> Can you add a BUILD_BUG_ON() to check that the number of entries in
>> the
>> fixmap will never be above PAGETABLE_ENTRIES?
> Sure. What is the best place? Somewhere in setup_fixmap_mappings()?

I think so.

Cheers,

Jan Beulich July 22, 2024, 3:25 p.m. UTC | #6

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

Oleksii Kurochko July 22, 2024, 5:04 p.m. UTC | #7

On Mon, 2024-07-22 at 17:25 +0200, Jan Beulich wrote:
> 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()?
It should be, I'll rework that in the next patch version.

~ Oleksii

Oleksii Kurochko July 23, 2024, 12:58 p.m. UTC | #8

Hi Julien,

On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
> > +/* Fixmap slots */
> > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of
> > PMAP */
> 
> ... here is seems to be inclusive. Furthermore if you had 32-bit
> address 
> space, it is also quite easy to have to create a region right at the
> top 
> of it. So when END is exclusive, it would become 0.
> 
> So on Arm, we decided to start to get rid of "end". I would consider
> to 
> do the same on RISC-V for new functions.
I assume that you wrote here just as an example of confusion occurs
because of using *_END but just to be clear I have to leave
FIXMAP_MAP_END as-is because it is used now by common code.

~ Oleksii

Oleksii Kurochko July 23, 2024, 1:27 p.m. UTC | #9

Hello Julien,

On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
> > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > index 7d09e781bf..d69a174b5d 100644
> > --- 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];
> 
> Can you add a BUILD_BUG_ON() to check that the number of entries in
> the 
> fixmap will never be above PAGETABLE_ENTRIES?
I just realized that we don't have the information about how many
entries has been used. Am I confusing something?

~ Oleksii

Julien Grall July 23, 2024, 1:32 p.m. UTC | #10

On 23/07/2024 13:58, [e-mail] wrote:
> Hi Julien,

Hi Oleksii,

> On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
>>> +/* Fixmap slots */
>>> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
>>> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of
>>> PMAP */
>>
>> ... here is seems to be inclusive. Furthermore if you had 32-bit
>> address
>> space, it is also quite easy to have to create a region right at the
>> top
>> of it. So when END is exclusive, it would become 0.
>>
>> So on Arm, we decided to start to get rid of "end". I would consider
>> to
>> do the same on RISC-V for new functions.
> I assume that you wrote here just as an example of confusion occurs
> because of using *_END but just to be clear I have to leave
> FIXMAP_MAP_END as-is because it is used now by common code.

Indeed. FIXMAP_PMAP_END should stay for now.

Cheers,

Julien Grall July 23, 2024, 1:33 p.m. UTC | #11

On 23/07/2024 14:27, [e-mail] wrote:
> Hello Julien,

Hi Oleksii,


> On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
>>> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
>>> index 7d09e781bf..d69a174b5d 100644
>>> --- 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];
>>
>> Can you add a BUILD_BUG_ON() to check that the number of entries in
>> the
>> fixmap will never be above PAGETABLE_ENTRIES?
> I just realized that we don't have the information about how many
> entries has been used. Am I confusing something?

I think we do. It is FIX_LAST.

Cheers,

Oleksii Kurochko July 23, 2024, 1:34 p.m. UTC | #12

On Mon, 2024-07-22 at 19:04 +0200, [e-mail] wrote:
> On Mon, 2024-07-22 at 17:25 +0200, Jan Beulich wrote:
> > 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()?
> It should be, I'll rework that in the next patch version.
It couldn't be set_fixmap() and clear_fixmap() as I am going to
implement them using map_pages_to_xen() because:
...
    /*
     * 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);
...

~ Oleksii

Oleksii Kurochko July 23, 2024, 1:38 p.m. UTC | #13

On Tue, 2024-07-23 at 14:33 +0100, Julien Grall wrote:
> On 23/07/2024 14:27, [e-mail] wrote:
> > Hello Julien,
> 
> Hi Oleksii,
> 
> 
> > On Sun, 2024-07-21 at 09:46 +0100, Julien Grall wrote:
> > > > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > > > index 7d09e781bf..d69a174b5d 100644
> > > > --- 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];
> > > 
> > > Can you add a BUILD_BUG_ON() to check that the number of entries
> > > in
> > > the
> > > fixmap will never be above PAGETABLE_ENTRIES?
> > I just realized that we don't have the information about how many
> > entries has been used. Am I confusing something?
> 
> I think we do. It is FIX_LAST.
Sure. We have FIX_LAST. Thanks

~ Oleksii

Patch

diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 50583aafdc..3275477c17 100644
--- 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)
+
 #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct page_info))
 #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1)
 
diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
new file mode 100644
index 0000000000..fcfb82d69c
--- /dev/null
+++ b/xen/arch/riscv/include/asm/fixmap.h
@@ -0,0 +1,48 @@ 
+/*
+ * 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>
+
+/* 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)
+
+#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[];
+
+/* Map a page in a fixmap entry */
+extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int attributes);
+/* Remove a mapping from a fixmap entry */
+extern void clear_fixmap(unsigned int map);
+
+#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..cbbf3656d1 100644
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -81,6 +81,13 @@  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)
+{
+    *p = pte;
+    asm volatile ("sfence.vma");
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_RISCV_PAGE_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 7d09e781bf..d69a174b5d 100644
--- 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];
+
 #define HANDLE_PGTBL(curr_lvl_num)                                          \
     index = pt_index(curr_lvl_num, page_addr);                              \
     if ( pte_is_valid(pgtbl[index]) )                                       \
@@ -191,6 +194,38 @@  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;
+    unsigned int i;
+
+    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
+
+    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )
+    {
+        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) );
+
+    if ( !pte_is_valid(*pte) )
+    {
+        pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
+
+        write_pte(pte, tmp);
+
+        printk("(XEN) fixmap is mapped\n");
+    }
+
+    /*
+     * 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..63b1dd7bb6 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_SIZE, "Xen too large for early-boot assumptions")
 
 ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");