|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 4/8] xen/riscv: setup fixmap mapping
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 <oleksii.kurochko@xxxxxxxxx>
> > ---
> > 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,
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |