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(-)

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
-- 
2.48.1

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

On 05.02.2025 17:58, Oleksii Kurochko wrote:
> 
> 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?

Never use double underscores as an identifier prefix of your own. Only the
compiler (and in principle the library, if there was one) may use such.

Jan