Re: [PATCH v4 1/2] xen/riscv: introduce function for physical offset calculation

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-07-24 at 15:40 +0200, Jan Beulich wrote:
> On 24.07.2023 11:42, Oleksii Kurochko wrote:
> > @@ -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;
> 
> While Misra compliance is distant future for RISC-V, there's a
> problem here with there not being any declaration for this global
> variable. Adding a declaration isn't the only solution though:
> Patch 2 also only uses the variable in assembly code. Therefore
> the variable here could be made static, with ...
> 
> > @@ -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;
> > +}
> 
> ... this function (being invoked by the same assembly code
> function) returning the value alongside storing it.
Thanks for explanation about MISRA compliance.

> 
> FTAOD I wouldn't insist on this being taken care of right away,
> so if you get a maintainer ack this way, I'd be happy to commit as
> is.
If I don't get an ACK, I'll update the code with the next patch
version.

~ Oleksii