| Message ID | a4f0b312351e5f6a9e57f50ebbc3bda8a72c18bb.1738587493.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for vmap_to_mfn() and pt_mapping_level | expand |
On 03.02.2025 14:12, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -156,6 +156,10 @@ static inline struct page_info *virt_to_page(const void *v) > return frametable_virt_start + PFN_DOWN(va - directmap_virt_start); > } > > +#include <asm/page.h> asm/page.h already includes asm/mm.h, so you're introducing a circular dependency here (much of which the patch description is about, so it's unclear why you didn't solve this another way). Afaict ... > +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level); ... it's pte_t that presents a problem here. Why not simply put the declaration in asm/page.h (and drop all the secondary changes that don't really belong in this patch anyway)? Also nit: Excess blank after the first *. > --- a/xen/arch/riscv/pt.c > +++ b/xen/arch/riscv/pt.c > @@ -274,6 +274,61 @@ static int pt_update_entry(mfn_t root, vaddr_t virt, > return rc; > } > > +/* > + * pt_walk() performs software page table walking and returns the pte_t of > + * a leaf node or the leaf-most not-present pte_t if no leaf node is found > + * for further analysis. > + * Additionally, pt_walk() returns the level of the found pte. > + */ > +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level) See nit above. > +{ > + const mfn_t root = get_root_page(); > + /* > + * In pt_walk() only XEN_TABLE_MAP_NONE and XEN_TABLE_SUPER_PAGE are > + * handled (as they are only possible for page table walking), so > + * initialize `ret` with "impossible" XEN_TABLE_MAP_NOMEM. > + */ > + int ret = XEN_TABLE_MAP_NOMEM; > + unsigned int level = HYP_PT_ROOT_LEVEL; With this initialization and ... > + pte_t *table; > + > + DECLARE_OFFSETS(offsets, va); > + > + table = map_table(root); > + > + /* > + * Find `table` of an entry which corresponds to `va` by iterating for each > + * page level and checking if the entry points to a next page table or > + * to a page. > + * > + * Two cases are possible: > + * - ret == XEN_TABLE_SUPER_PAGE means that the entry was find; (nit: found) > + * (Despite the name) XEN_TABLE_SUPER_PAGE also covers 4K mappings. If > + * pt_next_level() is called for page table level 0, it results in the > + * entry being a pointer to a leaf node, thereby returning > + * XEN_TABLE_SUPER_PAGE, despite of the fact this leaf covers 4k mapping. > + * - ret == XEN_TABLE_MAP_NONE means that requested `va` wasn't actually > + * mapped. > + */ > + while ( (ret != XEN_TABLE_MAP_NONE) && (ret != XEN_TABLE_SUPER_PAGE) ) > + { > + /* > + * This case shouldn't really occur as it will mean that for table > + * level 0 a pointer to next page table has been written, but at > + * level 0 it could be only a pointer to 4k page. > + */ > + ASSERT(level <= HYP_PT_ROOT_LEVEL); > + > + ret = pt_next_level(false, &table, offsets[level]); > + level--; ... this being the only updating of the variable, what are the assertion and accompanying comment about? What you're rather at risk of is for level to wrap around through 0. In fact I think it does, ... > + } > + > + if ( pte_level ) > + *pte_level = level + 1; > + > + return table + offsets[level + 1]; > +} ... which you account for by adding 1 in both of the places here. Don't you think that it would be better to avoid such, e.g.: for ( level = HYP_PT_ROOT_LEVEL; ; --level ) { int ret = pt_next_level(false, &table, offsets[level]); if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE ) break; ASSERT(level); } or for ( level = CONFIG_PAGING_LEVELS; level--; ) { int ret = pt_next_level(false, &table, offsets[level]); if ( ret == XEN_TABLE_MAP_NONE || ret == XEN_TABLE_SUPER_PAGE ) break; } This then also avoids the oddity about ret's initializer. Jan
diff --git a/xen/arch/riscv/include/asm/cmpxchg.h b/xen/arch/riscv/include/asm/cmpxchg.h index 662d3fd5d4..4cfe5927b7 100644 --- a/xen/arch/riscv/include/asm/cmpxchg.h +++ b/xen/arch/riscv/include/asm/cmpxchg.h @@ -4,6 +4,7 @@ #ifndef ASM__RISCV__CMPXCHG_H #define ASM__RISCV__CMPXCHG_H +#include <xen/bitops.h> #include <xen/compiler.h> #include <xen/lib.h> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index 292aa48fc1..10a15a8b03 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -156,6 +156,10 @@ static inline struct page_info *virt_to_page(const void *v) return frametable_virt_start + PFN_DOWN(va - directmap_virt_start); } +#include <asm/page.h> + +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level); + /* * Common code requires get_page_type and put_page_type. * We don't care about typecounts so we just do the minimum to make it diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h index 7a6174a109..b9076173f4 100644 --- a/xen/arch/riscv/include/asm/page.h +++ b/xen/arch/riscv/include/asm/page.h @@ -7,7 +7,6 @@ #include <xen/bug.h> #include <xen/const.h> -#include <xen/domain_page.h> #include <xen/errno.h> #include <xen/types.h> @@ -177,18 +176,7 @@ static inline void invalidate_icache(void) #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE) #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE) -static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) -{ - const void *v = map_domain_page(_mfn(mfn)); - - if ( clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) ) - BUG(); - - unmap_domain_page(v); - - if ( sync_icache ) - invalidate_icache(); -} +void flush_page_to_ram(unsigned long mfn, bool sync_icache); /* Write a pagetable entry. */ static inline void write_pte(pte_t *p, pte_t pte) diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c index f2bf279bac..32574c879b 100644 --- a/xen/arch/riscv/mm.c +++ b/xen/arch/riscv/mm.c @@ -3,6 +3,7 @@ #include <xen/bootfdt.h> #include <xen/bug.h> #include <xen/compiler.h> +#include <xen/domain_page.h> #include <xen/init.h> #include <xen/kernel.h> #include <xen/libfdt/libfdt.h> @@ -564,3 +565,16 @@ void *__init arch_vmap_virt_end(void) { return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE); } + +void flush_page_to_ram(unsigned long mfn, bool sync_icache) +{ + const void *v = map_domain_page(_mfn(mfn)); + + if ( clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) ) + BUG(); + + unmap_domain_page(v); + + if ( sync_icache ) + invalidate_icache(); +} diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c index a703e0f1bd..2a5a191a70 100644 --- a/xen/arch/riscv/pt.c +++ b/xen/arch/riscv/pt.c @@ -274,6 +274,61 @@ static int pt_update_entry(mfn_t root, vaddr_t virt, return rc; } +/* + * pt_walk() performs software page table walking and returns the pte_t of + * a leaf node or the leaf-most not-present pte_t if no leaf node is found + * for further analysis. + * Additionally, pt_walk() returns the level of the found pte. + */ +pte_t * pt_walk(vaddr_t va, unsigned int *pte_level) +{ + const mfn_t root = get_root_page(); + /* + * In pt_walk() only XEN_TABLE_MAP_NONE and XEN_TABLE_SUPER_PAGE are + * handled (as they are only possible for page table walking), so + * initialize `ret` with "impossible" XEN_TABLE_MAP_NOMEM. + */ + int ret = XEN_TABLE_MAP_NOMEM; + unsigned int level = HYP_PT_ROOT_LEVEL; + pte_t *table; + + DECLARE_OFFSETS(offsets, va); + + table = map_table(root); + + /* + * Find `table` of an entry which corresponds to `va` by iterating for each + * page level and checking if the entry points to a next page table or + * to a page. + * + * Two cases are possible: + * - ret == XEN_TABLE_SUPER_PAGE means that the entry was find; + * (Despite the name) XEN_TABLE_SUPER_PAGE also covers 4K mappings. If + * pt_next_level() is called for page table level 0, it results in the + * entry being a pointer to a leaf node, thereby returning + * XEN_TABLE_SUPER_PAGE, despite of the fact this leaf covers 4k mapping. + * - ret == XEN_TABLE_MAP_NONE means that requested `va` wasn't actually + * mapped. + */ + while ( (ret != XEN_TABLE_MAP_NONE) && (ret != XEN_TABLE_SUPER_PAGE) ) + { + /* + * This case shouldn't really occur as it will mean that for table + * level 0 a pointer to next page table has been written, but at + * level 0 it could be only a pointer to 4k page. + */ + ASSERT(level <= HYP_PT_ROOT_LEVEL); + + ret = pt_next_level(false, &table, offsets[level]); + level--; + } + + if ( pte_level ) + *pte_level = level + 1; + + return table + offsets[level + 1]; +} + /* Return the level where mapping should be done */ static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr, unsigned int flags)
RISC-V doesn't have hardware feature to ask MMU to translate virtual address to physical address ( like Arm has, for example ), so software page table walking is implemented. As pte_walk() returns pte_t and it requires inclusion of <asm/page.h> in <asm/mm.h>, the following compilation started to occur after this inclusion: - implicit declaration of function 'GENMASK'. To resolve this, <xen/bitops.h> needs to be included at the top of <asm/cmpxchg.h>. - implicit declaration of map_domain_page() and unmap_domain_page(). This issue arises because, after including <asm/page.h> in <asm/mm.h>, we could have the following hierarchy if someone decides to include <xen/domain.h>: <xen/domain_page.h> <xen/mm.h> <asm/mm.h> <asm/page.h> <xen/domain_page.h> ... flush_to_ram() which uses {un}map_domain_page() <--- ... Declaration of {un}map_domain_page() happens here <--- To avoid this compilation issue, definition of flush_to_ram() is moved to mm.c. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v2: - Update the code of pt_walk() to return pte_t instead of paddr_t. - Fix typos and drop blankets inside parentheses in the comment. - Update the `ret` handling; there is no need for `mfn` calculation anymore as pt_walk() returns or pte_t of a leaf node or non-present pte_t. - Drop the comment before unmap_table(). - Drop local variable `pa` as pt_walk() is going to return pte_t instead of paddr_t. - Add the comment above pt_walk() to explain what it does and returns. - Add additional explanation about possible return values of pt_next_level() used inside while loop in pt_walk(). - Change `pa` to `table` in the comment before while loop in pt_walk() as actually this loop finds a page table where paga table entry for `va` is located. - After including <asm/page.h> in <asm/mm.h>, the following compilation error occurs: ./arch/riscv/include/asm/cmpxchg.h:56:9: error: implicit declaration of function 'GENMASK' To resolve this, <xen/bitops.h> needs to be included at the top of <asm/cmpxchg.h>. - To avoid an issue with the implicit declaration of map_domain_page() and unmap_domain_page() after including <asm/page.h> in <asm/mm.h>, the implementation of flush_page_to_ram() has been moved to mm.c. (look for more detailed explanation in the commit message) As a result drop inclusion of <xen/domain.h> in <asm/page.h>. - Update the commit message --- xen/arch/riscv/include/asm/cmpxchg.h | 1 + xen/arch/riscv/include/asm/mm.h | 4 ++ xen/arch/riscv/include/asm/page.h | 14 +------ xen/arch/riscv/mm.c | 14 +++++++ xen/arch/riscv/pt.c | 55 ++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 13 deletions(-)