| Message ID | 553b07e967f56b78eba2d27c9115cce707a45c08.1678976127.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
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
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
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
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
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); }
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