[PATCH v2 0/3] Register Xen's load address as a boot module

[PATCH v2 0/3] Register Xen's load address as a boot module

[Oleksii Kurochko]

This patch series registers Xen's load address as a boot module and
introduce virt_to_maddr(), and drops LINK_TO_LOAD() to use virt_to_maddr()
instead.

---
Changes in V2:
 - Update the commit message ( drop depency from "device tree mapping" patch
   series as it was merged to staging )
 - Other changes please look at the specific patch.
---

Oleksii Kurochko (3):
  xen/riscv: implement virt_to_maddr()
  xen/riscv: switch LINK_TO_LOAD() to virt_to_maddr()
  xen/riscv: register Xen's load address as a boot module

 xen/arch/riscv/include/asm/config.h |  4 ++++
 xen/arch/riscv/include/asm/mm.h     | 17 ++++++++++++++++-
 xen/arch/riscv/mm.c                 |  9 ++++-----
 xen/arch/riscv/setup.c              |  6 ++++++
 4 files changed, 30 insertions(+), 6 deletions(-)

-- 
2.46.2

[PATCH v2 1/3] xen/riscv: implement virt_to_maddr()

[Oleksii Kurochko]

Implement the virt_to_maddr() function to convert virtual addresses
to machine addresses, including checks for address ranges such as
the direct mapping area (DIRECTMAP_VIRT_START) and the Xen virtual
address space. To implement this, the phys_offset variable is made
accessible outside of riscv/mm.c.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
  - Drop casts in virt_to_maddr() for ASSERT which checks that VA is
    in the range of where Xen is located.
  - Add UL suffix for or XEN_VIRT_START by using _AC(..., UL) and add inclusion
    of <xen/const.h>
  - Add the comment above return which explains why there is no need
    to do " - XEN_VIRT_START.
---
 xen/arch/riscv/include/asm/config.h |  4 ++++
 xen/arch/riscv/include/asm/mm.h     | 17 ++++++++++++++++-
 xen/arch/riscv/mm.c                 |  2 +-
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 7dbb235685..8884aeab16 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -155,6 +155,10 @@
 
 #define IDENT_AREA_SIZE 64
 
+#ifndef __ASSEMBLY__
+extern unsigned long phys_offset;
+#endif
+
 #endif /* __RISCV_CONFIG_H__ */
 /*
  * Local variables:
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 4b7b00b850..0f7879d685 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -5,6 +5,7 @@
 
 #include <public/xen.h>
 #include <xen/bug.h>
+#include <xen/const.h>
 #include <xen/mm-frame.h>
 #include <xen/pdx.h>
 #include <xen/types.h>
@@ -28,7 +29,21 @@ static inline void *maddr_to_virt(paddr_t ma)
     return NULL;
 }
 
-#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
+static inline unsigned long virt_to_maddr(unsigned long va)
+{
+    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
+    if ((va >= DIRECTMAP_VIRT_START) &&
+        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
+        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
+
+    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
+    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
+           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT)));
+
+    /* phys_offset = load_start - XEN_VIRT_START */
+    return phys_offset + va;
+}
+#define virt_to_maddr(va) virt_to_maddr((unsigned long)(va))
 
 /* Convert between Xen-heap virtual addresses and machine frame numbers. */
 #define __virt_to_mfn(va)  mfn_x(maddr_to_mfn(virt_to_maddr(va)))
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 4a628aef83..7a1919e07e 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -26,7 +26,7 @@ struct mmu_desc {
     pte_t *pgtbl_base;
 };
 
-static unsigned long __ro_after_init phys_offset;
+unsigned long __ro_after_init phys_offset;
 
 #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
 #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
-- 
2.46.2

[PATCH v2 2/3] xen/riscv: switch LINK_TO_LOAD() to virt_to_maddr()

[Oleksii Kurochko]

Use virt_to_maddr() instead of LINK_TO_LOAD as virt_to_maddr()
covers all the cases where LINK_TO_LOAD() is used.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - Drop the cast of virt_to_maddr() argument in remove_identity_mapping() as
   this cast is done inside virtu_to_maddr() wrapper macros.
 - Update the commit message ( rewording to be more clear )
---
 xen/arch/riscv/mm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 7a1919e07e..b5a5535812 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -29,7 +29,6 @@ struct mmu_desc {
 unsigned long __ro_after_init phys_offset;
 
 #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
-#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
 
 /*
  * It is expected that Xen won't be more then 2 MB.
@@ -122,7 +121,7 @@ static void __init setup_initial_mapping(struct mmu_desc *mmu_desc,
                 unsigned long paddr = (page_addr - map_start) + pa_start;
                 unsigned int permissions = PTE_LEAF_DEFAULT;
                 unsigned long addr = is_identity_mapping
-                                     ? page_addr : LINK_TO_LOAD(page_addr);
+                                     ? page_addr : virt_to_maddr(page_addr);
                 pte_t pte_to_be_written;
 
                 index = pt_index(0, page_addr);
@@ -225,7 +224,7 @@ void __init setup_fixmap_mappings(void)
 
     BUG_ON(pte_is_valid(*pte));
 
-    tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
+    tmp = paddr_to_pte(virt_to_maddr(&xen_fixmap), PTE_TABLE);
     write_pte(pte, tmp);
 
     RISCV_FENCE(rw, rw);
@@ -312,7 +311,7 @@ void __init remove_identity_mapping(void)
     pte_t *pgtbl;
     unsigned int index, xen_index;
     unsigned long ident_start =
-        LINK_TO_LOAD(turn_on_mmu) & XEN_PT_LEVEL_MAP_MASK(0);
+        virt_to_maddr(turn_on_mmu) & XEN_PT_LEVEL_MAP_MASK(0);
 
     for ( pgtbl = stage1_pgtbl_root, i = CONFIG_PAGING_LEVELS; i; i-- )
     {
-- 
2.46.2

[PATCH v2 3/3] xen/riscv: register Xen's load address as a boot module

[Oleksii Kurochko]

Avoid using BOOTMOD_XEN region for other purposes or boot modules
which could result in memory corruption or undefined behaviour.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
 - Drop local variable xen_bootmodule as it won't be used after initialization.
 - Drop unnecessary cast for the 3rd argument of add_boot_module() call in
   start_xen().
 - Change BUG_ON(!xen_bootmodule) to panic().
---
 xen/arch/riscv/setup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index 6d156c3a40..f531ca38ee 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <xen/bug.h>
+#include <xen/bootfdt.h>
 #include <xen/compile.h>
 #include <xen/device_tree.h>
 #include <xen/init.h>
@@ -44,6 +45,11 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
               "Please check your bootloader.\n",
               dtb_addr, BOOT_FDT_VIRT_SIZE);
 
+    /* Register Xen's load address as a boot module. */
+    if ( !add_boot_module(BOOTMOD_XEN, virt_to_maddr(_start),
+                          _end - _start, false) )
+        panic("Failed to add BOOTMOD_XEN\n");
+
     printk("All set up\n");
 
     machine_halt();
-- 
2.46.2

Re: [PATCH v2 1/3] xen/riscv: implement virt_to_maddr()

[oleksii.kurochko]

On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote:
> Implement the virt_to_maddr() function to convert virtual addresses
> to machine addresses, including checks for address ranges such as
> the direct mapping area (DIRECTMAP_VIRT_START) and the Xen virtual
> address space. To implement this, the phys_offset variable is made
> accessible outside of riscv/mm.c.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V2:
>   - Drop casts in virt_to_maddr() for ASSERT which checks that VA is
>     in the range of where Xen is located.
>   - Add UL suffix for or XEN_VIRT_START by using _AC(..., UL) and add
> inclusion
>     of <xen/const.h>
>   - Add the comment above return which explains why there is no need
>     to do " - XEN_VIRT_START.
> ---
>  xen/arch/riscv/include/asm/config.h |  4 ++++
>  xen/arch/riscv/include/asm/mm.h     | 17 ++++++++++++++++-
>  xen/arch/riscv/mm.c                 |  2 +-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/riscv/include/asm/config.h
> b/xen/arch/riscv/include/asm/config.h
> index 7dbb235685..8884aeab16 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -155,6 +155,10 @@
>  
>  #define IDENT_AREA_SIZE 64
>  
> +#ifndef __ASSEMBLY__
> +extern unsigned long phys_offset;
> +#endif
> +
>  #endif /* __RISCV_CONFIG_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/arch/riscv/include/asm/mm.h
> b/xen/arch/riscv/include/asm/mm.h
> index 4b7b00b850..0f7879d685 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -5,6 +5,7 @@
>  
>  #include <public/xen.h>
>  #include <xen/bug.h>
> +#include <xen/const.h>
>  #include <xen/mm-frame.h>
>  #include <xen/pdx.h>
>  #include <xen/types.h>
> @@ -28,7 +29,21 @@ static inline void *maddr_to_virt(paddr_t ma)
>      return NULL;
>  }
>  
> -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
> +static inline unsigned long virt_to_maddr(unsigned long va)
> +{
> +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
It should be ASSERT(va < (...)).

Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as 
address from Xen's VA space ( XEN_VIRT_START ) are higher then
(DIRECTMAP_VIRT_START + DIRECTMAP_SIZE).

Or as an option we could consider to drop this ASSERT() as if
VA is from directmap range the if below will catch that; otherwise
we have another one ASSERT() which checks that VA is from Xen VA range
where it is sage to use (phys_offset + va).

Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START +
DIRECTMAP_SIZE))" or I am missing something?

~ Oleksii


> +    if ((va >= DIRECTMAP_VIRT_START) &&
> +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> +
> +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> +    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> +           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER +
> PAGE_SHIFT)));
> +
> +    /* phys_offset = load_start - XEN_VIRT_START */
> +    return phys_offset + va;
> +}
> +#define virt_to_maddr(va) virt_to_maddr((unsigned long)(va))
>  
>  /* Convert between Xen-heap virtual addresses and machine frame
> numbers. */
>  #define __virt_to_mfn(va)  mfn_x(maddr_to_mfn(virt_to_maddr(va)))
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 4a628aef83..7a1919e07e 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -26,7 +26,7 @@ struct mmu_desc {
>      pte_t *pgtbl_base;
>  };
>  
> -static unsigned long __ro_after_init phys_offset;
> +unsigned long __ro_after_init phys_offset;
>  
>  #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>  #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)

Re: [PATCH v2 1/3] xen/riscv: implement virt_to_maddr()

[Jan Beulich]

On 08.10.2024 12:26, oleksii.kurochko@gmail.com wrote:
> On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote:
>> @@ -28,7 +29,21 @@ static inline void *maddr_to_virt(paddr_t ma)
>>      return NULL;
>>  }
>>  
>> -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
>> +static inline unsigned long virt_to_maddr(unsigned long va)
>> +{
>> +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
> It should be ASSERT(va < (...)).
> 
> Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as 
> address from Xen's VA space ( XEN_VIRT_START ) are higher then
> (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE).
> 
> Or as an option we could consider to drop this ASSERT() as if
> VA is from directmap range the if below will catch that; otherwise
> we have another one ASSERT() which checks that VA is from Xen VA range
> where it is sage to use (phys_offset + va).
> 
> Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START +
> DIRECTMAP_SIZE))" or I am missing something?

Counter question: Why did you put the ASSERT() there when - once
corrected - it's actually pointless? What you want to make sure is
that virt_to_maddr() can't be invoked with bad argument (without
noticing). If that's achieved with just the other assertion (as it
looks to be), then leaving out this assertion ought to be fine.

Jan

Re: [PATCH v2 2/3] xen/riscv: switch LINK_TO_LOAD() to virt_to_maddr()

[Jan Beulich]

On 04.10.2024 18:04, Oleksii Kurochko wrote:
> Use virt_to_maddr() instead of LINK_TO_LOAD as virt_to_maddr()
> covers all the cases where LINK_TO_LOAD() is used.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 3/3] xen/riscv: register Xen's load address as a boot module

[Jan Beulich]

On 04.10.2024 18:04, Oleksii Kurochko wrote:
> Avoid using BOOTMOD_XEN region for other purposes or boot modules
> which could result in memory corruption or undefined behaviour.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 1/3] xen/riscv: implement virt_to_maddr()

[oleksii.kurochko]

On Tue, 2024-10-08 at 12:34 +0200, Jan Beulich wrote:
> On 08.10.2024 12:26, oleksii.kurochko@gmail.com wrote:
> > On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote:
> > > @@ -28,7 +29,21 @@ static inline void *maddr_to_virt(paddr_t ma)
> > >      return NULL;
> > >  }
> > >  
> > > -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
> > > +static inline unsigned long virt_to_maddr(unsigned long va)
> > > +{
> > > +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
> > It should be ASSERT(va < (...)).
> > 
> > Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as 
> > address from Xen's VA space ( XEN_VIRT_START ) are higher then
> > (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE).
> > 
> > Or as an option we could consider to drop this ASSERT() as if
> > VA is from directmap range the if below will catch that; otherwise
> > we have another one ASSERT() which checks that VA is from Xen VA
> > range
> > where it is sage to use (phys_offset + va).
> > 
> > Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START
> > +
> > DIRECTMAP_SIZE))" or I am missing something?
> 
> Counter question: Why did you put the ASSERT() there when - once
> corrected - it's actually pointless? What you want to make sure is
> that virt_to_maddr() can't be invoked with bad argument (without
> noticing). If that's achieved with just the other assertion (as it
> looks to be), then leaving out this assertion ought to be fine.
Originally, I didn’t include the part after 'if (...)'. The purpose of
ASSERT(va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)) was to ensure that
the virtual address fell within the expected (directmap) range. Later,
I added the part after 'if (...)', which extended the acceptable
virtual address range to also cover addresses from Xen’s linkage
address space. At that point, I should have removed the original
ASSERT() but overlooked it.

I will drop the first ASSERT() and update the commit message / comment
above virt_to_maddr() why it is enough to have only 1 ASSERT() after if
(...).

~ Oleksii