Re: [PATCH v1 4/8] xen/riscv: introduce function for physical offset calculation

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-06-12 at 09:15 +0200, Jan Beulich wrote:
> On 06.06.2023 21:55, Oleksii Kurochko wrote:
> > The function was introduced to not 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.
> 
> "... to _not_ calculate ..."?
_not_ should be dropped. Thanks.

> 
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -19,9 +19,11 @@ struct mmu_desc {
> >  
> >  extern unsigned char cpu0_boot_stack[STACK_SIZE];
> >  
> > -#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)
> > +static unsigned long phys_offset;
> 
> __ro_after_init?
It makes sense to mark variable as __ro_after_init. I'll take into
account in new version of patch.

> 
> > +#define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
> > +#define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
> > +
> >  
> 
> Nit: No double blank lines please.
Sorry. Missed that.

> 
> > @@ -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  calc_phys_offset(void)
> 
> __init? And nit: No double blanks please.
Thanks. It should be __init. I'll remove double blanks in new patch
version.

~ Oleksii