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

[Julien Grall <julien@xen.org>]

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@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?

There is a missing "given" after "that".

> There is a patch:
> https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@gmail.com/
> 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?

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.

Cheers,

-- 
Julien Grall