1. Patchew
  2. Xen
  3. [PATCH for 4.20 v9 0/5] RISCV basic exception handling implementation
  4. View patch

[PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME

Oleksii Kurochko posted 5 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Oleksii Kurochko 2 months, 2 weeks ago 
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Kconfig |  1 +
 xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++
 xen/common/bug.c       |  1 +
 3 files changed, 33 insertions(+)

diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index b4b354a778..74ad019fe7 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -5,6 +5,7 @@ config RISCV
 config RISCV_64
 	def_bool y
 	select 64BIT
+	select GENERIC_BUG_FRAME
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index cb18b30ff2..7ba16252fc 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,7 @@
  * RISC-V Trap handlers
  */
 
+#include <xen/bug.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 
@@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
     die();
 }
 
+static bool is_valid_bug_insn(uint32_t insn)
+{
+    return insn == BUG_INSN_32 ||
+           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
+}
+
+/* Should be used only on Xen code */
+static uint32_t read_instr(unsigned long pc)
+{
+    uint16_t instr16 = *(uint16_t *)pc;
+
+    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
+
+    if ( GET_INSN_LENGTH(instr16) == 2 )
+        return instr16;
+
+    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
+
+    return *(uint32_t *)pc;
+}
+
 void do_trap(struct cpu_user_regs *cpu_regs)
 {
+    register_t pc = cpu_regs->sepc;
+    uint32_t instr = read_instr(pc);
+
+    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, pc) >= 0 ) )
+    {
+        cpu_regs->sepc += GET_INSN_LENGTH(instr);
+        return;
+    }
+
     do_unexpected_trap(cpu_regs);
 }
 
diff --git a/xen/common/bug.c b/xen/common/bug.c
index b7c5d8fd4d..75cb35fcfa 100644
--- a/xen/common/bug.c
+++ b/xen/common/bug.c
@@ -1,6 +1,7 @@
 #include <xen/bug.h>
 #include <xen/errno.h>
 #include <xen/kernel.h>
+#include <xen/lib.h>
 #include <xen/livepatch.h>
 #include <xen/string.h>
 #include <xen/types.h>
-- 
2.45.2
Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Jan Beulich 2 months, 1 week ago 
On 02.07.2024 13:23, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  xen/arch/riscv/Kconfig |  1 +
>  xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++
>  xen/common/bug.c       |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> index b4b354a778..74ad019fe7 100644
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -5,6 +5,7 @@ config RISCV
>  config RISCV_64
>  	def_bool y
>  	select 64BIT
> +	select GENERIC_BUG_FRAME

Any particular reason to put this here, and not higher up with RISCV?

> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct cpu_user_regs *regs)
>      die();
>  }
>  
> +static bool is_valid_bug_insn(uint32_t insn)
> +{
> +    return insn == BUG_INSN_32 ||
> +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> +}
> +
> +/* Should be used only on Xen code */
> +static uint32_t read_instr(unsigned long pc)
> +{
> +    uint16_t instr16 = *(uint16_t *)pc;
> +
> +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
> +
> +    if ( GET_INSN_LENGTH(instr16) == 2 )
> +        return instr16;
> +
> +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
> +
> +    return *(uint32_t *)pc;
> +}

Related to the point made further down: If either of these assertions fails,
won't we come back again right here? If either of the is_kernel_*text()
wasn't working quite right, wouldn't we be at risk of entering an infinite
loop (presumably not quite infinite because of the stack overflowing at some
point)?

>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> +    register_t pc = cpu_regs->sepc;
> +    uint32_t instr = read_instr(pc);
> +
> +    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs, pc) >= 0 ) )

No consideration of the kind of exception? I'd expect it is one very
specific one which the BUG insn would raise, and then there's no point
fetching the insn when it's a different kind of exception.

Further, nit: Certainly no need for the parentheses on the lhs of the &&.
Having them on the rhs is a matter of taste, so okay, but then the
blanks immediately inside will want dropping.


> --- a/xen/common/bug.c
> +++ b/xen/common/bug.c
> @@ -1,6 +1,7 @@
>  #include <xen/bug.h>
>  #include <xen/errno.h>
>  #include <xen/kernel.h>
> +#include <xen/lib.h>
>  #include <xen/livepatch.h>
>  #include <xen/string.h>
>  #include <xen/types.h>

Unrelated change? Or did you simply forget to mention in the description
why it's needed?

Jan
Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Oleksii 2 months, 1 week ago 
On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
> On 02.07.2024 13:23, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >  xen/arch/riscv/Kconfig |  1 +
> >  xen/arch/riscv/traps.c | 31 +++++++++++++++++++++++++++++++
> >  xen/common/bug.c       |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> > index b4b354a778..74ad019fe7 100644
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -5,6 +5,7 @@ config RISCV
> >  config RISCV_64
> >  	def_bool y
> >  	select 64BIT
> > +	select GENERIC_BUG_FRAME
> 
> Any particular reason to put this here, and not higher up with RISCV?
Yes, you are right it would be better to put it inside "config RISCV".

> 
> > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> >      die();
> >  }
> >  
> > +static bool is_valid_bug_insn(uint32_t insn)
> > +{
> > +    return insn == BUG_INSN_32 ||
> > +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> > +}
> > +
> > +/* Should be used only on Xen code */
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > +    uint16_t instr16 = *(uint16_t *)pc;
> > +
> > +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
> > +
> > +    if ( GET_INSN_LENGTH(instr16) == 2 )
> > +        return instr16;
> > +
> > +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
> > +
> > +    return *(uint32_t *)pc;
> > +}
> 
> Related to the point made further down: If either of these assertions
> fails,
> won't we come back again right here? If either of the
> is_kernel_*text()
> wasn't working quite right, wouldn't we be at risk of entering an
> infinite
> loop (presumably not quite infinite because of the stack overflowing
> at some
> point)?
It is really possible to have infinite loop here so it should be better
to use 'if' with die() or panic().

> 
> >  void do_trap(struct cpu_user_regs *cpu_regs)
> >  {
> > +    register_t pc = cpu_regs->sepc;
> > +    uint32_t instr = read_instr(pc);
> > +
> > +    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs,
> > pc) >= 0 ) )
> 
> No consideration of the kind of exception? I'd expect it is one very
> specific one which the BUG insn would raise, and then there's no
> point
> fetching the insn when it's a different kind of exception.
Good point.

We should have 0x3 ( breakpoint exception ) in scause register. We can
just check that without reading instruction and then also
is_valid_bug_insn could be dropped too.


> 
> Further, nit: Certainly no need for the parentheses on the lhs of the
> &&.
> Having them on the rhs is a matter of taste, so okay, but then the
> blanks immediately inside will want dropping.

> 
> 
> > --- a/xen/common/bug.c
> > +++ b/xen/common/bug.c
> > @@ -1,6 +1,7 @@
> >  #include <xen/bug.h>
> >  #include <xen/errno.h>
> >  #include <xen/kernel.h>
> > +#include <xen/lib.h>
> >  #include <xen/livepatch.h>
> >  #include <xen/string.h>
> >  #include <xen/types.h>
> 
> Unrelated change? Or did you simply forget to mention in the
> description
> why it's needed?
I added it to "Changes in ..." which I forgot to add, but I will add an
explanation to the description. It is better place for it.

<xen/lib.h> is needed to be included for the reason that panic() and
printk() is used in common/bug.c and RISC-V fails if it is not included
with the following errors:
   common/bug.c:69:9: error: implicit declaration of function 'printk'
   [-Werror=implicit-function-declaration]
      69 |         printk("Xen WARN at %s%s:%d\n", prefix, filename,
   lineno);
         |         ^~~~~~
   common/bug.c:77:9: error: implicit declaration of function 'panic'
   [-Werror=implicit-function-declaration]
      77 |         panic("Xen BUG at %s%s:%d\n", prefix, filename,
   lineno);
> 
> ~ Oleksii
Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Jan Beulich 2 months, 1 week ago 
On 11.07.2024 10:50, Oleksii wrote:
> On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
>> On 02.07.2024 13:23, Oleksii Kurochko wrote:
>>> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const struct
>>> cpu_user_regs *regs)
>>>      die();
>>>  }
>>>  
>>> +static bool is_valid_bug_insn(uint32_t insn)
>>> +{
>>> +    return insn == BUG_INSN_32 ||
>>> +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
>>> +}
>>> +
>>> +/* Should be used only on Xen code */
>>> +static uint32_t read_instr(unsigned long pc)
>>> +{
>>> +    uint16_t instr16 = *(uint16_t *)pc;
>>> +
>>> +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));
>>> +
>>> +    if ( GET_INSN_LENGTH(instr16) == 2 )
>>> +        return instr16;
>>> +
>>> +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));
>>> +
>>> +    return *(uint32_t *)pc;
>>> +}
>>
>> Related to the point made further down: If either of these assertions
>> fails,
>> won't we come back again right here? If either of the
>> is_kernel_*text()
>> wasn't working quite right, wouldn't we be at risk of entering an
>> infinite
>> loop (presumably not quite infinite because of the stack overflowing
>> at some
>> point)?
> It is really possible to have infinite loop here so it should be better
> to use 'if' with die() or panic().
> 
>>
>>>  void do_trap(struct cpu_user_regs *cpu_regs)
>>>  {
>>> +    register_t pc = cpu_regs->sepc;
>>> +    uint32_t instr = read_instr(pc);
>>> +
>>> +    if ( ( is_valid_bug_insn(instr) ) && ( do_bug_frame(cpu_regs,
>>> pc) >= 0 ) )
>>
>> No consideration of the kind of exception? I'd expect it is one very
>> specific one which the BUG insn would raise, and then there's no
>> point
>> fetching the insn when it's a different kind of exception.
> Good point.
> 
> We should have 0x3 ( breakpoint exception ) in scause register. We can
> just check that without reading instruction and then also
> is_valid_bug_insn could be dropped too.

Just that then you'll also lose the is_kernel_*text() checking, which I
understand is there to remind you/us that one this becomes reachable
from non-Xen code, adjustments are going to be needed.

Jan
Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Oleksii 2 months, 1 week ago 
On Thu, 2024-07-11 at 11:25 +0200, Jan Beulich wrote:
> On 11.07.2024 10:50, Oleksii wrote:
> > On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
> > > On 02.07.2024 13:23, Oleksii Kurochko wrote:
> > > > @@ -101,8 +102,38 @@ static void do_unexpected_trap(const
> > > > struct
> > > > cpu_user_regs *regs)
> > > >      die();
> > > >  }
> > > >  
> > > > +static bool is_valid_bug_insn(uint32_t insn)
> > > > +{
> > > > +    return insn == BUG_INSN_32 ||
> > > > +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> > > > +}
> > > > +
> > > > +/* Should be used only on Xen code */
> > > > +static uint32_t read_instr(unsigned long pc)
> > > > +{
> > > > +    uint16_t instr16 = *(uint16_t *)pc;
> > > > +
> > > > +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc +
> > > > 1));
> > > > +
> > > > +    if ( GET_INSN_LENGTH(instr16) == 2 )
> > > > +        return instr16;
> > > > +
> > > > +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc +
> > > > 3));
> > > > +
> > > > +    return *(uint32_t *)pc;
> > > > +}
> > > 
> > > Related to the point made further down: If either of these
> > > assertions
> > > fails,
> > > won't we come back again right here? If either of the
> > > is_kernel_*text()
> > > wasn't working quite right, wouldn't we be at risk of entering an
> > > infinite
> > > loop (presumably not quite infinite because of the stack
> > > overflowing
> > > at some
> > > point)?
> > It is really possible to have infinite loop here so it should be
> > better
> > to use 'if' with die() or panic().
> > 
> > > 
> > > >  void do_trap(struct cpu_user_regs *cpu_regs)
> > > >  {
> > > > +    register_t pc = cpu_regs->sepc;
> > > > +    uint32_t instr = read_instr(pc);
> > > > +
> > > > +    if ( ( is_valid_bug_insn(instr) ) && (
> > > > do_bug_frame(cpu_regs,
> > > > pc) >= 0 ) )
> > > 
> > > No consideration of the kind of exception? I'd expect it is one
> > > very
> > > specific one which the BUG insn would raise, and then there's no
> > > point
> > > fetching the insn when it's a different kind of exception.
> > Good point.
> > 
> > We should have 0x3 ( breakpoint exception ) in scause register. We
> > can
> > just check that without reading instruction and then also
> > is_valid_bug_insn could be dropped too.
> 
> Just that then you'll also lose the is_kernel_*text() checking, which
> I
> understand is there to remind you/us that one this becomes reachable
> from non-Xen code, adjustments are going to be needed.
One thing I wrote incorrectly is that we still need fetch instruction
or at least 16 bits to identify the length of instruction to set proper
sepc:
    cpu_regs->sepc += GET_INSN_LENGTH(instr);

We could write that in the following way:
    cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
Would it be okay?

~ Oleksii
Re: [PATCH v9 4/5] xen/riscv: enable GENERIC_BUG_FRAME
Posted by Jan Beulich 2 months, 1 week ago 
On 11.07.2024 14:14, Oleksii wrote:
> On Thu, 2024-07-11 at 11:25 +0200, Jan Beulich wrote:
>> On 11.07.2024 10:50, Oleksii wrote:
>>> On Wed, 2024-07-10 at 12:01 +0200, Jan Beulich wrote:
>>>> On 02.07.2024 13:23, Oleksii Kurochko wrote:
>>>>> @@ -101,8 +102,38 @@ static void do_unexpected_trap(const
>>>>> struct
>>>>> cpu_user_regs *regs)
>>>>>      die();
>>>>>  }
>>>>>  
>>>>> +static bool is_valid_bug_insn(uint32_t insn)
>>>>> +{
>>>>> +    return insn == BUG_INSN_32 ||
>>>>> +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
>>>>> +}
>>>>> +
>>>>> +/* Should be used only on Xen code */
>>>>> +static uint32_t read_instr(unsigned long pc)
>>>>> +{
>>>>> +    uint16_t instr16 = *(uint16_t *)pc;
>>>>> +
>>>>> +    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc +
>>>>> 1));
>>>>> +
>>>>> +    if ( GET_INSN_LENGTH(instr16) == 2 )
>>>>> +        return instr16;
>>>>> +
>>>>> +    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc +
>>>>> 3));
>>>>> +
>>>>> +    return *(uint32_t *)pc;
>>>>> +}
>>>>
>>>> Related to the point made further down: If either of these
>>>> assertions
>>>> fails,
>>>> won't we come back again right here? If either of the
>>>> is_kernel_*text()
>>>> wasn't working quite right, wouldn't we be at risk of entering an
>>>> infinite
>>>> loop (presumably not quite infinite because of the stack
>>>> overflowing
>>>> at some
>>>> point)?
>>> It is really possible to have infinite loop here so it should be
>>> better
>>> to use 'if' with die() or panic().
>>>
>>>>
>>>>>  void do_trap(struct cpu_user_regs *cpu_regs)
>>>>>  {
>>>>> +    register_t pc = cpu_regs->sepc;
>>>>> +    uint32_t instr = read_instr(pc);
>>>>> +
>>>>> +    if ( ( is_valid_bug_insn(instr) ) && (
>>>>> do_bug_frame(cpu_regs,
>>>>> pc) >= 0 ) )
>>>>
>>>> No consideration of the kind of exception? I'd expect it is one
>>>> very
>>>> specific one which the BUG insn would raise, and then there's no
>>>> point
>>>> fetching the insn when it's a different kind of exception.
>>> Good point.
>>>
>>> We should have 0x3 ( breakpoint exception ) in scause register. We
>>> can
>>> just check that without reading instruction and then also
>>> is_valid_bug_insn could be dropped too.
>>
>> Just that then you'll also lose the is_kernel_*text() checking, which
>> I
>> understand is there to remind you/us that one this becomes reachable
>> from non-Xen code, adjustments are going to be needed.
> One thing I wrote incorrectly is that we still need fetch instruction
> or at least 16 bits to identify the length of instruction to set proper
> sepc:
>     cpu_regs->sepc += GET_INSN_LENGTH(instr);
> 
> We could write that in the following way:
>     cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);
> Would it be okay?

I think so, as long as you retain the assertion in some way, ahead of the
deref of pc.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.