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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxx/
> 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@xxxxxxxxx/
> 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