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

[Julien Grall <julien@xen.org>]

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@gmail.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> ---
>>> 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?

A suggestion for the top of the function:

"Initialize the trap handling. This is called twice (before and after 
the MMU)."

And for on top of 'addr', I would add:

"When the MMU is off, this will be a physical address otherwise it would 
be a virtual address. This is guarantee because [fill the blank]".

Cheers,

-- 
Julien Grall