| Message ID | 9f1fbf84a82fd141f40428993106f0672d6d8c4c.1739363240.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for vmap_to_mfn() and pt_mapping_level | expand |
On 12.02.2025 17:50, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/pt.c > +++ b/xen/arch/riscv/pt.c > @@ -185,6 +185,68 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned int offset) > return XEN_TABLE_NORMAL; > } > > +/* > + * _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 by using > + * `pte_level` argument. > + * `pte_level` is optional, set `pte_level`=NULL if a caller doesn't need > + * the level of the found pte. How about this, reducing redundancy a little? * _pt_walk() can optionally return the level of the found pte. Pass NULL * for `pte_level` if this information isn't needed. > +pte_t pt_walk(vaddr_t va, unsigned int *pte_level) > +{ > + pte_t *entry = _pt_walk(va, pte_level); > + pte_t pte = *entry; > + > + unmap_table(entry); > + > + return pte; > +} "entry" especially in this context is ambiguous. I would expect a variable of this name to be of type pte_t, not pte_t *. How about "ptep"? Preferably with these adjustments, which I'd be fine making while committing, Reviewed-by: Jan Beulich <jbeulich@suse.com> Considering the 4.20? tag you'll need to decide whether you still want this in before the release. Jan
On 2/19/25 12:14 PM, Jan Beulich wrote: > On 12.02.2025 17:50, Oleksii Kurochko wrote: >> --- a/xen/arch/riscv/pt.c >> +++ b/xen/arch/riscv/pt.c >> @@ -185,6 +185,68 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned int offset) >> return XEN_TABLE_NORMAL; >> } >> >> +/* >> + * _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 by using >> + * `pte_level` argument. >> + * `pte_level` is optional, set `pte_level`=NULL if a caller doesn't need >> + * the level of the found pte. > How about this, reducing redundancy a little? > > * _pt_walk() can optionally return the level of the found pte. Pass NULL > * for `pte_level` if this information isn't needed. > >> +pte_t pt_walk(vaddr_t va, unsigned int *pte_level) >> +{ >> + pte_t *entry = _pt_walk(va, pte_level); >> + pte_t pte = *entry; >> + >> + unmap_table(entry); >> + >> + return pte; >> +} > "entry" especially in this context is ambiguous. I would expect a variable of > this name to be of type pte_t, not pte_t *. How about "ptep"? Agree with both your suggestions, it would be better to use `ptep instead of `entry` and rephrase the comment. > > Preferably with these adjustments, which I'd be fine making while committing, > Reviewed-by: Jan Beulich<jbeulich@suse.com> > > Considering the 4.20? tag you'll need to decide whether you still want this > in before the release. Considering that it is still needed a new version for patch3 of this patch series and that the mentioned issues aren't affected no one, lets consider the full patch series for 4.21. Thanks. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h index 7a6174a109..0439a1a9ee 100644 --- a/xen/arch/riscv/include/asm/page.h +++ b/xen/arch/riscv/include/asm/page.h @@ -208,6 +208,8 @@ static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags) return (pte_t){ .pte = pte }; } +pte_t pt_walk(vaddr_t va, unsigned int *pte_level); + #endif /* __ASSEMBLY__ */ #endif /* ASM__RISCV__PAGE_H */ diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c index a703e0f1bd..260a3a9699 100644 --- a/xen/arch/riscv/pt.c +++ b/xen/arch/riscv/pt.c @@ -185,6 +185,68 @@ static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned int offset) return XEN_TABLE_NORMAL; } +/* + * _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 by using + * `pte_level` argument. + * `pte_level` is optional, set `pte_level`=NULL if a caller doesn't need + * the level of the found pte. + * + * Note: unmapping of final `table` should be done by a caller. + */ +static pte_t *_pt_walk(vaddr_t va, unsigned int *pte_level) +{ + const mfn_t root = get_root_page(); + unsigned int 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 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. + */ + 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); + } + + if ( pte_level ) + *pte_level = level; + + return table + offsets[level]; +} + +pte_t pt_walk(vaddr_t va, unsigned int *pte_level) +{ + pte_t *entry = _pt_walk(va, pte_level); + pte_t pte = *entry; + + unmap_table(entry); + + return pte; +} + /* Update an entry at the level @target. */ static int pt_update_entry(mfn_t root, vaddr_t virt, mfn_t mfn, unsigned int target,
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. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v4: - Update the comment message above _pt_walk(): add information that `pte_level` is optional and add a note that `table` should be unmapped by a caller. - Unmap `table` in pt_walk(). --- Changes in v3: - Remove circular dependency. - Move declaration of pt_walk() to asm/page.h. - Revert other not connected to pt_walk() changes. - Update the commit message. - Drop unnessary anymore local variables of pt_walk(). - Refactor pt_walk() to use for() loop instead of while() loop as it was suggested by Jan B. - Introduce _pt_walk() which returns pte_t * and update prototype of pt_walk() to return pte_t by value. --- 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/page.h | 2 + xen/arch/riscv/pt.c | 62 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)