Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-03-22 at 13:51 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 22/03/2023 13:40, Oleksii wrote:
> > On Wed, 2023-03-22 at 12:14 +0000, Julien Grall wrote:
> > > 
> > > 
> > > On 22/03/2023 11:33, Oleksii wrote:
> > > > Hi Julien,
> > > 
> > > Hi Oleksii,
> > > 
> > > > 
> > > > On Tue, 2023-03-21 at 17:42 +0000, Julien Grall wrote:
> > > > > Hi Oleksii,
> > > > > 
> > > > > On 16/03/2023 14:39, Oleksii Kurochko wrote:
> > > > > > Signed-off-by: Oleksii Kurochko
> > > > > > <oleksii.kurochko@xxxxxxxxx>
> > > > > > Reviewed-by: Alistair Francis <alistair.francis@xxxxxxx>
> > > > > > ---
> > > > > > Changes in V5:
> > > > > >      - Nothing changed
> > > > > > ---
> > > > > > Changes in V4:
> > > > > >      - Nothing changed
> > > > > > ---
> > > > > > Changes in V3:
> > > > > >      - Nothing changed
> > > > > > ---
> > > > > > Changes in V2:
> > > > > >      - Rename setup_trap_handler() to trap_init().
> > > > > >      - Add Reviewed-by to the commit message.
> > > > > > ---
> > > > > >     xen/arch/riscv/include/asm/traps.h | 1 +
> > > > > >     xen/arch/riscv/setup.c             | 5 +++++
> > > > > >     xen/arch/riscv/traps.c             | 7 +++++++
> > > > > >     3 files changed, 13 insertions(+)
> > > > > > 
> > > > > > diff --git a/xen/arch/riscv/include/asm/traps.h
> > > > > > b/xen/arch/riscv/include/asm/traps.h
> > > > > > index f3fb6b25d1..f1879294ef 100644
> > > > > > --- a/xen/arch/riscv/include/asm/traps.h
> > > > > > +++ b/xen/arch/riscv/include/asm/traps.h
> > > > > > @@ -7,6 +7,7 @@
> > > > > >     
> > > > > >     void do_trap(struct cpu_user_regs *cpu_regs);
> > > > > >     void handle_trap(void);
> > > > > > +void trap_init(void);
> > > > > >     
> > > > > >     #endif /* __ASSEMBLY__ */
> > > > > >     
> > > > > > diff --git a/xen/arch/riscv/setup.c
> > > > > > b/xen/arch/riscv/setup.c
> > > > > > index 36556eb779..b44d105b5f 100644
> > > > > > --- a/xen/arch/riscv/setup.c
> > > > > > +++ b/xen/arch/riscv/setup.c
> > > > > > @@ -3,7 +3,9 @@
> > > > > >     #include <xen/kernel.h>
> > > > > >     
> > > > > >     #include <asm/boot-info.h>
> > > > > > +#include <asm/csr.h>
> > > > > >     #include <asm/early_printk.h>
> > > > > > +#include <asm/traps.h>
> > > > > >     
> > > > > >     /* Xen stack for bringing up the first CPU. */
> > > > > >     unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
> > > > > > @@ -32,7 +34,10 @@ void __init noreturn start_xen(unsigned
> > > > > > long
> > > > > > bootcpu_id,
> > > > > >     
> > > > > >         fill_boot_info();
> > > > > >     
> > > > > > +    trap_init();
> > > > > > +
> > > > > >         early_printk("All set up\n");
> > > > > > +
> > > > > >         for ( ;; )
> > > > > >             asm volatile ("wfi");
> > > > > >     
> > > > > > diff --git a/xen/arch/riscv/traps.c
> > > > > > b/xen/arch/riscv/traps.c
> > > > > > index 8a1529e0c5..581f34efbc 100644
> > > > > > --- a/xen/arch/riscv/traps.c
> > > > > > +++ b/xen/arch/riscv/traps.c
> > > > > > @@ -13,6 +13,13 @@
> > > > > >     #include <asm/processor.h>
> > > > > >     #include <asm/traps.h>
> > > > > >     
> > > > > > +void trap_init(void)
> > > > > > +{
> > > > > > +    unsigned long addr = (unsigned long)&handle_trap;
> > > > > 
> > > > > It is not super clear to me whether this is going to store
> > > > > the
> > > > > virtual
> > > > > or physical address.
> > > > Actually it is going to store both the virtual and physical
> > > > address.
> > > > Depending on if MMU is enabled or not.
> > > 
> > > I think some comment in the code would be really good because
> > > this
> > > is...
> > > 
> > > > > 
> > > > > Depending on the answer, the next would be whether the value
> > > > > would
> > > > > still
> > > > > be valid after the MMU is turned on?
> > > > It would still be valid because for addr will be generated PC-
> > > > relative
> > > > address.
> > > 
> > > ... not clear to me what would guarantee that Xen is compiled
> > > with
> > > -noPIE. Is the cmodel?
> 
> There is a missing "given" after "that".
> 
> > There is a patch:
> > https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@xxxxxxxxx/
> > Which guarantees that Xen is complied with -noPIE.
> I am a bit puzzled with what you wrote. From my understanding, with 
> -noPIE, the compiler would be free to use absolute address rather
> than 
> pc-relative one. Do you have any pointer to documentation that would 
> back your reasoning?

https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf
If look at pseudoinstructions (p110 in pdf) which work with addresses
they are always pc-relative ( they all use AUIPC instruction ). The
only question is that if .got is used or not ( which depends on
pie,pic, etc... )
> 
> I have already mentioned before, but I think it would really useful
> if 
> you start adding more documentation (in particular of such behavior)
> in 
> the code or docs/ (for more wide). This would help everyone to 
> understand how everything is meant to work.
Sure. I will add more documentation for such kind of code.

~ Oleksii