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

Oleksii Kurochko posted 7 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Oleksii Kurochko 1 year, 2 months ago
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;
+
+    csr_write(CSR_STVEC, addr);
+}
+
 static const char *decode_trap_cause(unsigned long cause)
 {
     static const char *const trap_causes[] = {
-- 
2.39.2
Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Julien Grall 1 year, 2 months ago
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.

Depending on the answer, the next would be whether the value would still 
be valid after the MMU is turned on?

> +
> +    csr_write(CSR_STVEC, addr);
> +}
> +
>   static const char *decode_trap_cause(unsigned long cause)
>   {
>       static const char *const trap_causes[] = {

Cheers,

-- 
Julien Grall
Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Oleksii 1 year, 2 months ago
Hi Julien,

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.
> 
> 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.
> 
> > +
> > +    csr_write(CSR_STVEC, addr);
> > +}
> > +
> >   static const char *decode_trap_cause(unsigned long cause)
> >   {
> >       static const char *const trap_causes[] = {
> 
> Cheers,
> 
> ~ Oleksii
Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Julien Grall 1 year, 2 months ago

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

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Oleksii 1 year, 2 months ago
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 patch:
https://lore.kernel.org/xen-devel/2785518800dce64fafb3096480a5ae4c4e026bcb.1678970065.git.oleksii.kurochko@gmail.com/
Which guarantees that Xen is complied with -noPIE.

The cmodel determines which software addressing mode is used, and,
therefore, what constraints are enforced on the linked program.

> 
> 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]".
Thanks for the recommendations.
I will take them into account.

~ Oleksii
Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Julien Grall 1 year, 2 months ago
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

Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Oleksii 1 year, 2 months ago
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@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?

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
Re: [PATCH v5 5/7] xen/riscv: introduce trap_init()
Posted by Jan Beulich 1 year, 2 months ago
On 22.03.2023 14:51, Julien Grall wrote:
> 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?

It might for RV32 (using lui), but for 64-bit the ISA simply doesn't
have any halfway efficient means to load addresses in other than a
PC-relative manner [1]. -nopie really suppresses the compiler emitting
code going through .got (which then indeed would mean using absolute
addresses). I understand that at least for now RV32 isn't really of
interest; if it were, I guess the concern would be more significant.

Jan

[1] There aren't even suitable relocations to express such in ELF,
because you'd need to have a way to get at the top 32 bits of an
address. The only alternative would be .got-like indirection without
actually using .got.