1. Patchew
  2. Xen
  3. [PATCH v5 0/7] RISCV basic exception handling implementation
  4. View patch

[PATCH v5 1/7] xen/riscv: introduce boot information structure

Oleksii Kurochko posted 7 patches 2 years, 8 months ago
There is a newer version of this series
[PATCH v5 1/7] xen/riscv: introduce boot information structure
Posted by Oleksii Kurochko 2 years, 8 months ago 
The structure holds information about:
1. linker start/end address
2. load start/end address

Also the patch introduces offsets for boot information structure
members to access them in assembly code.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V5:
 * the patch was introduced in the current patch series (V5)
---
 xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
 xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
 2 files changed, 18 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/boot-info.h

diff --git a/xen/arch/riscv/include/asm/boot-info.h b/xen/arch/riscv/include/asm/boot-info.h
new file mode 100644
index 0000000000..cda3d278f5
--- /dev/null
+++ b/xen/arch/riscv/include/asm/boot-info.h
@@ -0,0 +1,15 @@
+#ifndef _ASM_BOOT_INFO_H
+#define _ASM_BOOT_INFO_H
+
+extern struct boot_info {
+    unsigned long linker_start;
+    unsigned long linker_end;
+    unsigned long load_start;
+    unsigned long load_end;
+} boot_info;
+
+/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't enabled. */
+#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start + boot_info.load_start)
+#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start + boot_info.linker_start)
+
+#endif
\ No newline at end of file
diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c
index d632b75c2a..6b89e9a91d 100644
--- a/xen/arch/riscv/riscv64/asm-offsets.c
+++ b/xen/arch/riscv/riscv64/asm-offsets.c
@@ -1,5 +1,6 @@
 #define COMPILE_OFFSETS
 
+#include <asm/boot-info.h>
 #include <asm/processor.h>
 #include <xen/types.h>
 
@@ -50,4 +51,6 @@ void asm_offsets(void)
     OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
     OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
     OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
+    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
+    OFFSET(BI_LOAD_START, struct boot_info, load_start);
 }
-- 
2.39.2
Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure
Posted by Andrew Cooper 2 years, 8 months ago 
On 16/03/2023 2:39 pm, Oleksii Kurochko wrote:
> The structure holds information about:
> 1. linker start/end address
> 2. load start/end address
>
> Also the patch introduces offsets for boot information structure
> members to access them in assembly code.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V5:
>  * the patch was introduced in the current patch series (V5)
> ---
>  xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
>  xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
>  2 files changed, 18 insertions(+)
>  create mode 100644 xen/arch/riscv/include/asm/boot-info.h
>
> diff --git a/xen/arch/riscv/include/asm/boot-info.h b/xen/arch/riscv/include/asm/boot-info.h
> new file mode 100644
> index 0000000000..cda3d278f5
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/boot-info.h
> @@ -0,0 +1,15 @@
> +#ifndef _ASM_BOOT_INFO_H
> +#define _ASM_BOOT_INFO_H
> +
> +extern struct boot_info {
> +    unsigned long linker_start;
> +    unsigned long linker_end;
> +    unsigned long load_start;
> +    unsigned long load_end;
> +} boot_info;
> +
> +/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't enabled. */
> +#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start + boot_info.load_start)
> +#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start + boot_info.linker_start)
> +
> +#endif
> \ No newline at end of file

As a minor point, you should have newlines at the end of each file.

However, I'm not sure boot info like this is a clever move.  You're
creating a 3rd different way of doing something which should be entirely
common.  Some details are in
https://lore.kernel.org/xen-devel/115c178b-f0a7-cf6e-3e33-e6aa49b17baf@srcf.net/
and note how many errors I already found in x86 and ARM.

Perhaps its time to dust that plan off again.  As Jan says, there's
_start and _end (or future variations therefore), and xen_phys_start
which is all that ought to exist in order to build the common functionality.

~Andrew
Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure
Posted by Oleksii 2 years, 8 months ago 
On Tue, 2023-03-21 at 11:56 +0000, Andrew Cooper wrote:
> On 16/03/2023 2:39 pm, Oleksii Kurochko wrote:
> > The structure holds information about:
> > 1. linker start/end address
> > 2. load start/end address
> > 
> > Also the patch introduces offsets for boot information structure
> > members to access them in assembly code.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> > Changes in V5:
> >  * the patch was introduced in the current patch series (V5)
> > ---
> >  xen/arch/riscv/include/asm/boot-info.h | 15 +++++++++++++++
> >  xen/arch/riscv/riscv64/asm-offsets.c   |  3 +++
> >  2 files changed, 18 insertions(+)
> >  create mode 100644 xen/arch/riscv/include/asm/boot-info.h
> > 
> > diff --git a/xen/arch/riscv/include/asm/boot-info.h
> > b/xen/arch/riscv/include/asm/boot-info.h
> > new file mode 100644
> > index 0000000000..cda3d278f5
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/boot-info.h
> > @@ -0,0 +1,15 @@
> > +#ifndef _ASM_BOOT_INFO_H
> > +#define _ASM_BOOT_INFO_H
> > +
> > +extern struct boot_info {
> > +    unsigned long linker_start;
> > +    unsigned long linker_end;
> > +    unsigned long load_start;
> > +    unsigned long load_end;
> > +} boot_info;
> > +
> > +/* LINK_TO_LOAD() and LOAD_TO_LINK() works only when MMU isn't
> > enabled. */
> > +#define LINK_TO_LOAD(addr) ((addr) - boot_info.linker_start +
> > boot_info.load_start)
> > +#define LOAD_TO_LINK(addr) ((addr) - boot_info.load_start +
> > boot_info.linker_start)
> > +
> > +#endif
> > \ No newline at end of file
> 
> As a minor point, you should have newlines at the end of each file.
> 
> However, I'm not sure boot info like this is a clever move.  You're
> creating a 3rd different way of doing something which should be
> entirely
> common.  Some details are in
> https://lore.kernel.org/xen-devel/115c178b-f0a7-cf6e-3e33-e6aa49b17baf@srcf.net/
> and note how many errors I already found in x86 and ARM.
> 
In the link above you mentioned that:
  Reviewing its usage shows that ARM is broken when trying to handle
  BUG/ASSERT in livepatches, because they don't check is_patch() on the
  message target.
Check is_patch() will be added to ARM implementation after generic bug
implementation will be merged: 
https://lore.kernel.org/xen-devel/2afad972cd8da98dcb0ba509ba29ff239dc47cd0.1678900513.git.oleksii.kurochko@gmail.com/
> Perhaps its time to dust that plan off again.  As Jan says, there's
> _start and _end (or future variations therefore), and xen_phys_start
> which is all that ought to exist in order to build the common
> functionality.
I am unsure that I understand why the introduction of boot_info is a
bad idea.
Basically, it is only a wrapper/helper over _start, _end, and
xen_phys_start ( it is not introduced explicitly as taking into account
that access of _start will be PC-relative it is equal to xen_phys_start
).

~ Oleksii
Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure
Posted by Jan Beulich 2 years, 8 months ago 
On 16.03.2023 15:39, Oleksii Kurochko wrote:
> @@ -50,4 +51,6 @@ void asm_offsets(void)
>      OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
>      OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
>      OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
> +    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
> +    OFFSET(BI_LOAD_START, struct boot_info, load_start);
>  }

May I suggest that you add BLANK(); and a blank line between separate
groups of OFFSET() uses? This may not matter much right now, but it'll
help readability and findability once the file further grows.

Jan
Re: [PATCH v5 1/7] xen/riscv: introduce boot information structure
Posted by Oleksii 2 years, 8 months ago 
On Tue, 2023-03-21 at 12:17 +0100, Jan Beulich wrote:
> On 16.03.2023 15:39, Oleksii Kurochko wrote:
> > @@ -50,4 +51,6 @@ void asm_offsets(void)
> >      OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc);
> >      OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus);
> >      OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs);
> > +    OFFSET(BI_LINKER_START, struct boot_info, linker_start);
> > +    OFFSET(BI_LOAD_START, struct boot_info, load_start);
> >  }
> 
> May I suggest that you add BLANK(); and a blank line between separate
> groups of OFFSET() uses? This may not matter much right now, but
> it'll
> help readability and findability once the file further grows.
Sure. I'll update it in the next version of the patch.

~ Oleksii

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.