1. Patchew
  2. Xen
  3. [PATCH v3 0/3] xen/riscv: introduce identity mapping
  4. View patch

[PATCH v3 2/3] xen/riscv: introduce function for physical offset calculation

Oleksii Kurochko posted 3 patches 2 years, 6 months ago
There is a newer version of this series
[PATCH v3 2/3] xen/riscv: introduce function for physical offset calculation
Posted by Oleksii Kurochko 2 years, 6 months ago 
The function was introduced to calculate and save physical
offset before MMU is enabled because access to start() is
PC-relative and in case of linker_addr != load_addr it will
result in incorrect value in phys_offset.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V3:
 - save/restore of a0/a1 registers before C first function call.
---
Changes in V2:
  - add __ro_after_init for phys_offset variable.
  - remove double blank lines.
  - add __init for calc_phys_offset().
  - update the commit message.
  - declaring variable phys_off as non static as it will be used in head.S.
---
 xen/arch/riscv/include/asm/mm.h |  2 ++
 xen/arch/riscv/mm.c             | 18 +++++++++++++++---
 xen/arch/riscv/riscv64/head.S   | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 5e3ac5cde3..d9c4205103 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -15,4 +15,6 @@ void setup_initial_pagetables(void);
 void enable_mmu(void);
 void cont_after_mmu_is_enabled(void);
 
+void calc_phys_offset(void);
+
 #endif /* _ASM_RISCV_MM_H */
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index fddb3cd0bd..c84a8a7c3c 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/cache.h>
 #include <xen/compiler.h>
 #include <xen/init.h>
 #include <xen/kernel.h>
@@ -19,9 +20,10 @@ struct mmu_desc {
     pte_t *pgtbl_base;
 };
 
-#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
-#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
-#define LINK_TO_LOAD(addr) ((addr) + 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)
 
 /*
  * It is expected that Xen won't be more then 2 MB.
@@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
     switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
                           cont_after_mmu_is_enabled);
 }
+
+/*
+ * calc_phys_offset() should be used before MMU is enabled because access to
+ * start() is PC-relative and in case when load_addr != linker_addr phys_offset
+ * will have an incorrect value
+ */
+void __init calc_phys_offset(void)
+{
+    phys_offset = (unsigned long)start - XEN_VIRT_START;
+}
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
index 2c0304646a..9015d06233 100644
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -29,6 +29,20 @@ ENTRY(start)
 
         jal     reset_stack
 
+        /*
+         * save hart_id and dtb_base as a0 and a1 register can be used
+         * by C code ( f.e. setup_initial_pagetables will update a0 and
+         * a1 )
+         */
+        mv      s0, a0
+        mv      s1, a1
+
+        jal     calc_phys_offset
+
+        /* restore bootcpu_id and dtb address */
+        mv      a0, s0
+        mv      a1, s1
+
         tail    start_xen
 
         .section .text, "ax", %progbits
-- 
2.41.0
Re: [PATCH v3 2/3] xen/riscv: introduce function for physical offset calculation
Posted by Jan Beulich 2 years, 6 months ago 
On 17.07.2023 16:40, Oleksii Kurochko wrote:
> The function was introduced to calculate and save physical
> offset before MMU is enabled because access to start() is
> PC-relative and in case of linker_addr != load_addr it will
> result in incorrect value in phys_offset.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V3:
>  - save/restore of a0/a1 registers before C first function call.
> ---
> Changes in V2:
>   - add __ro_after_init for phys_offset variable.
>   - remove double blank lines.
>   - add __init for calc_phys_offset().
>   - update the commit message.
>   - declaring variable phys_off as non static as it will be used in head.S.
> ---
>  xen/arch/riscv/include/asm/mm.h |  2 ++
>  xen/arch/riscv/mm.c             | 18 +++++++++++++++---
>  xen/arch/riscv/riscv64/head.S   | 14 ++++++++++++++
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> index 5e3ac5cde3..d9c4205103 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -15,4 +15,6 @@ void setup_initial_pagetables(void);
>  void enable_mmu(void);
>  void cont_after_mmu_is_enabled(void);
>  
> +void calc_phys_offset(void);
> +
>  #endif /* _ASM_RISCV_MM_H */
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index fddb3cd0bd..c84a8a7c3c 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/cache.h>
>  #include <xen/compiler.h>
>  #include <xen/init.h>
>  #include <xen/kernel.h>
> @@ -19,9 +20,10 @@ struct mmu_desc {
>      pte_t *pgtbl_base;
>  };
>  
> -#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
> -#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
> -#define LINK_TO_LOAD(addr) ((addr) + 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)
>  
>  /*
>   * It is expected that Xen won't be more then 2 MB.
> @@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
>      switch_stack_and_jump((unsigned long)cpu0_boot_stack + STACK_SIZE,
>                            cont_after_mmu_is_enabled);
>  }
> +
> +/*
> + * calc_phys_offset() should be used before MMU is enabled because access to
> + * start() is PC-relative and in case when load_addr != linker_addr phys_offset
> + * will have an incorrect value
> + */
> +void __init calc_phys_offset(void)
> +{
> +    phys_offset = (unsigned long)start - XEN_VIRT_START;
> +}
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index 2c0304646a..9015d06233 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -29,6 +29,20 @@ ENTRY(start)
>  
>          jal     reset_stack
>  
> +        /*
> +         * save hart_id and dtb_base as a0 and a1 register can be used
> +         * by C code ( f.e. setup_initial_pagetables will update a0 and
> +         * a1 )

I'd recommend dropping the part in parentheses - for one the function
doesn't exist yet, and then you're merely restating what the ABI has.

> +         */
> +        mv      s0, a0
> +        mv      s1, a1
> +
> +        jal     calc_phys_offset
> +
> +        /* restore bootcpu_id and dtb address */

Is the first name here intentionally different from that in the other
comment (where it's "hart_id")?

Jan

> +        mv      a0, s0
> +        mv      a1, s1
> +
>          tail    start_xen
>  
>          .section .text, "ax", %progbits
Re: [PATCH v3 2/3] xen/riscv: introduce function for physical offset calculation
Posted by Oleksii 2 years, 6 months ago 
On Mon, 2023-07-17 at 16:58 +0200, Jan Beulich wrote:
> On 17.07.2023 16:40, Oleksii Kurochko wrote:
> > The function was introduced to calculate and save physical
> > offset before MMU is enabled because access to start() is
> > PC-relative and in case of linker_addr != load_addr it will
> > result in incorrect value in phys_offset.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V3:
> >  - save/restore of a0/a1 registers before C first function call.
> > ---
> > Changes in V2:
> >   - add __ro_after_init for phys_offset variable.
> >   - remove double blank lines.
> >   - add __init for calc_phys_offset().
> >   - update the commit message.
> >   - declaring variable phys_off as non static as it will be used in
> > head.S.
> > ---
> >  xen/arch/riscv/include/asm/mm.h |  2 ++
> >  xen/arch/riscv/mm.c             | 18 +++++++++++++++---
> >  xen/arch/riscv/riscv64/head.S   | 14 ++++++++++++++
> >  3 files changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > index 5e3ac5cde3..d9c4205103 100644
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -15,4 +15,6 @@ void setup_initial_pagetables(void);
> >  void enable_mmu(void);
> >  void cont_after_mmu_is_enabled(void);
> >  
> > +void calc_phys_offset(void);
> > +
> >  #endif /* _ASM_RISCV_MM_H */
> > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > index fddb3cd0bd..c84a8a7c3c 100644
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0-only */
> >  
> > +#include <xen/cache.h>
> >  #include <xen/compiler.h>
> >  #include <xen/init.h>
> >  #include <xen/kernel.h>
> > @@ -19,9 +20,10 @@ struct mmu_desc {
> >      pte_t *pgtbl_base;
> >  };
> >  
> > -#define PHYS_OFFSET ((unsigned long)_start - XEN_VIRT_START)
> > -#define LOAD_TO_LINK(addr) ((addr) - PHYS_OFFSET)
> > -#define LINK_TO_LOAD(addr) ((addr) + 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)
> >  
> >  /*
> >   * It is expected that Xen won't be more then 2 MB.
> > @@ -273,3 +275,13 @@ void __init noreturn noinline enable_mmu()
> >      switch_stack_and_jump((unsigned long)cpu0_boot_stack +
> > STACK_SIZE,
> >                            cont_after_mmu_is_enabled);
> >  }
> > +
> > +/*
> > + * calc_phys_offset() should be used before MMU is enabled because
> > access to
> > + * start() is PC-relative and in case when load_addr !=
> > linker_addr phys_offset
> > + * will have an incorrect value
> > + */
> > +void __init calc_phys_offset(void)
> > +{
> > +    phys_offset = (unsigned long)start - XEN_VIRT_START;
> > +}
> > diff --git a/xen/arch/riscv/riscv64/head.S
> > b/xen/arch/riscv/riscv64/head.S
> > index 2c0304646a..9015d06233 100644
> > --- a/xen/arch/riscv/riscv64/head.S
> > +++ b/xen/arch/riscv/riscv64/head.S
> > @@ -29,6 +29,20 @@ ENTRY(start)
> >  
> >          jal     reset_stack
> >  
> > +        /*
> > +         * save hart_id and dtb_base as a0 and a1 register can be
> > used
> > +         * by C code ( f.e. setup_initial_pagetables will update
> > a0 and
> > +         * a1 )
> 
> I'd recommend dropping the part in parentheses - for one the function
> doesn't exist yet, and then you're merely restating what the ABI has.
Thanks. I'll do that.
> 
> > +         */
> > +        mv      s0, a0
> > +        mv      s1, a1
> > +
> > +        jal     calc_phys_offset
> > +
> > +        /* restore bootcpu_id and dtb address */
> 
> Is the first name here intentionally different from that in the other
> comment (where it's "hart_id")?
Only one reason for that is that bootcpu_id is used an argument of
start_xen() function. But generally I missed it to sync with the name
above.

Probably it would be better to use 'bootcpu' everywhere instead of
'hart_id' as it is 'architecture independent way' to name CPU.

So I can update the comment above to: 'hart_id ( bootcpu_id )' ( as it
was done in the comment above ENTRY(start) ) or just bootcpu_id.

~ Oleksii

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.