| Message ID | 131ecfd1b39b4ca4fe3e5d7f7e28a130c301e0fd.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: > @@ -160,6 +158,18 @@ static inline struct page_info *virt_to_page(const void *v) > > pte_t * pt_walk(vaddr_t va, unsigned int *pte_level); > > +static inline mfn_t vmap_to_mfn_(vaddr_t va) Btw., for static functions (and variables) a prefixing underscore is fine to use. Its identifiers that don't have file scope which shouldn't. > +{ > + pte_t *entry = pt_walk(va, NULL); Oh, noticing the anomaly only here: Why would pt_walk() return a pointer to a PTE, rather than the pte_t by value? All this does is encourage open-coded accesses (even writes), when especially writes are supposed to be going through pt_update(). > + BUG_ON(!pte_is_mapping(*entry)); > + > + return mfn_from_pte(*entry); > +} > + > +#define vmap_to_mfn(va) vmap_to_mfn_((vaddr_t)va) You've lost the parenthesizing of va. Jan
On 2/4/25 2:56 PM, Jan Beulich wrote: > On 03.02.2025 14:12, Oleksii Kurochko wrote: >> @@ -160,6 +158,18 @@ static inline struct page_info *virt_to_page(const void *v) >> >> pte_t * pt_walk(vaddr_t va, unsigned int *pte_level); >> >> +static inline mfn_t vmap_to_mfn_(vaddr_t va) > Btw., for static functions (and variables) a prefixing underscore is > fine to use. Its identifiers that don't have file scope which shouldn't. Should it be used a single underscore prefixing or a double one? > >> +{ >> + pte_t *entry = pt_walk(va, NULL); > Oh, noticing the anomaly only here: Why would pt_walk() return a pointer > to a PTE, rather than the pte_t by value? All this does is encourage > open-coded accesses (even writes), when especially writes are supposed > to be going through pt_update(). I tried to play with forward declaration of pte_t to not introduce circular dependency in the previous patch. It would be really better to return pte_t by value, I will update that. Thanks. ~Oleksii
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index 10a15a8b03..814a7035a8 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -23,8 +23,6 @@ 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_page(va) mfn_to_page(vmap_to_mfn(va)) static inline void *maddr_to_virt(paddr_t ma) { @@ -160,6 +158,18 @@ static inline struct page_info *virt_to_page(const void *v) pte_t * pt_walk(vaddr_t va, unsigned int *pte_level); +static inline mfn_t vmap_to_mfn_(vaddr_t va) +{ + pte_t *entry = pt_walk(va, NULL); + + BUG_ON(!pte_is_mapping(*entry)); + + return mfn_from_pte(*entry); +} + +#define vmap_to_mfn(va) vmap_to_mfn_((vaddr_t)va) +#define vmap_to_page(va) mfn_to_page(vmap_to_mfn(va)) + /* * 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
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. 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 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 | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)