| Message ID | bbea545c2ca25f5e827e4d3b4cb2466478791480.1738933678.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for vmap_to_mfn() and pt_mapping_level | expand |
On 07.02.2025 14:13, Oleksii Kurochko wrote: > vmap_to_mfn() uses virt_to_maddr(), which is designed to work with VA from > either the direct map region or Xen's linkage region (XEN_VIRT_START). > An assertion will occur if it is used with other regions, in particular for > the VMAP region. > > Since RISC-V lacks a hardware feature to request the MMU to translate a VA to > a PA (as Arm does, for example), software page table walking (pt_walk()) is > used for the VMAP region to obtain the mfn from pte_t. > > To avoid introduce a circular dependency between asm/mm.h and asm/page.h by > including each other, the macro _vmap_to_mfn() is introduced in asm/page.h, > as it uses struct pte_t and pte_is_mapping() from asm/page.h. _vmap_to_mfn() > macro is then reused in the definition of vmap_to_mfn() macro in asm/mm.h. > > Fixes: 7db8d2bd9b ("xen/riscv: add minimal stuff to mm.h to build full Xen") > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in v3: > - Move vmap_to_mfn_ to asm/page.h to deal with circular dependency. > - Convert vmap_to_mfn_() to macros. Why both? > --- a/xen/arch/riscv/include/asm/page.h > +++ b/xen/arch/riscv/include/asm/page.h > @@ -210,6 +210,13 @@ static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags) > > pte_t pt_walk(vaddr_t va, unsigned int *pte_level); > > +#define _vmap_to_mfn(va) \ > +({ \ > + pte_t entry = pt_walk((va), NULL); \ If this is to remain a macro, va doesn't need parenthesizing (as the argument passed is just the identifier, not an expression. Be careful with the naming of macro local variables. Consider a use size (for whatever reason) having unsigned long entry; ... mfn = vmap_to_mfn(entry); This is where appending an underscore comes into play. Jan
On 2/7/25 2:30 PM, Jan Beulich wrote: > On 07.02.2025 14:13, Oleksii Kurochko wrote: >> vmap_to_mfn() uses virt_to_maddr(), which is designed to work with VA from >> either the direct map region or Xen's linkage region (XEN_VIRT_START). >> An assertion will occur if it is used with other regions, in particular for >> the VMAP region. >> >> Since RISC-V lacks a hardware feature to request the MMU to translate a VA to >> a PA (as Arm does, for example), software page table walking (pt_walk()) is >> used for the VMAP region to obtain the mfn from pte_t. >> >> To avoid introduce a circular dependency between asm/mm.h and asm/page.h by >> including each other, the macro _vmap_to_mfn() is introduced in asm/page.h, >> as it uses struct pte_t and pte_is_mapping() from asm/page.h. _vmap_to_mfn() >> macro is then reused in the definition of vmap_to_mfn() macro in asm/mm.h. >> >> Fixes: 7db8d2bd9b ("xen/riscv: add minimal stuff to mm.h to build full Xen") >> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> >> --- >> Changes in v3: >> - Move vmap_to_mfn_ to asm/page.h to deal with circular dependency. >> - Convert vmap_to_mfn_() to macros. > Why both? Just for consistency. vmap_to_mfn_() could be defined as static inline, I can send the new patch version with such changes, probably, static inline would be better in this case. >> --- a/xen/arch/riscv/include/asm/page.h >> +++ b/xen/arch/riscv/include/asm/page.h >> @@ -210,6 +210,13 @@ static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags) >> >> pte_t pt_walk(vaddr_t va, unsigned int *pte_level); >> >> +#define _vmap_to_mfn(va) \ >> +({ \ >> + pte_t entry = pt_walk((va), NULL); \ > If this is to remain a macro, va doesn't need parenthesizing (as the argument > passed is just the identifier, not an expression. > > Be careful with the naming of macro local variables. Consider a use size (for > whatever reason) having > > unsigned long entry; > ... > mfn = vmap_to_mfn(entry); > > This is where appending an underscore comes into play. This could be another reason to use|static inline _vmap_to_mfn(...)| instead of a macro. I think I will rewrite the|_vmap_to_mfn()| macro as a|static inline| function in the next patch series version. I'll send it after receiving comments on the entire patch series. Thanks. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index 292aa48fc1..4035cd400a 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -23,7 +23,7 @@ extern vaddr_t directmap_virt_start; #define gaddr_to_gfn(ga) _gfn(paddr_to_pfn(ga)) #define mfn_to_maddr(mfn) pfn_to_paddr(mfn_x(mfn)) #define maddr_to_mfn(ma) _mfn(paddr_to_pfn(ma)) -#define vmap_to_mfn(va) maddr_to_mfn(virt_to_maddr((vaddr_t)(va))) +#define vmap_to_mfn(va) _vmap_to_mfn((vaddr_t)(va)) #define vmap_to_page(va) mfn_to_page(vmap_to_mfn(va)) static inline void *maddr_to_virt(paddr_t ma) diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h index 0439a1a9ee..18ba0dd9df 100644 --- a/xen/arch/riscv/include/asm/page.h +++ b/xen/arch/riscv/include/asm/page.h @@ -210,6 +210,13 @@ static inline pte_t pte_from_mfn(mfn_t mfn, unsigned int flags) pte_t pt_walk(vaddr_t va, unsigned int *pte_level); +#define _vmap_to_mfn(va) \ +({ \ + pte_t entry = pt_walk((va), NULL); \ + BUG_ON(!pte_is_mapping(entry)); \ + mfn_from_pte(entry); \ +}) + #endif /* __ASSEMBLY__ */ #endif /* ASM__RISCV__PAGE_H */
vmap_to_mfn() uses virt_to_maddr(), which is designed to work with VA from either the direct map region or Xen's linkage region (XEN_VIRT_START). An assertion will occur if it is used with other regions, in particular for the VMAP region. Since RISC-V lacks a hardware feature to request the MMU to translate a VA to a PA (as Arm does, for example), software page table walking (pt_walk()) is used for the VMAP region to obtain the mfn from pte_t. To avoid introduce a circular dependency between asm/mm.h and asm/page.h by including each other, the macro _vmap_to_mfn() is introduced in asm/page.h, as it uses struct pte_t and pte_is_mapping() from asm/page.h. _vmap_to_mfn() macro is then reused in the definition of vmap_to_mfn() macro in asm/mm.h. Fixes: 7db8d2bd9b ("xen/riscv: add minimal stuff to mm.h to build full Xen") Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v3: - Move vmap_to_mfn_ to asm/page.h to deal with circular dependency. - Convert vmap_to_mfn_() to macros. - Rename vmap_to_mfn_ to _vmap_to_mfn. - Update _vmap_to_mfn() to work with pte_t instead of pte_t*. - Add parentheses around va argument for macros vmap_to_mfn(). - Updated the commit message. --- Changes in v2: - Update defintion of vmap_to_mfn() as pt_walk() now returns pte_t instead of paddr_t. - Update the commit message. --- xen/arch/riscv/include/asm/mm.h | 2 +- xen/arch/riscv/include/asm/page.h | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-)