Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On Thu, 2023-06-01 at 09:59 +0200, Jan Beulich wrote:
> On 31.05.2023 22:06, Oleksii wrote:
> > On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
> > > > +static uint32_t read_instr(unsigned long pc)
> > > > +{
> > > > + uint16_t instr16 = *(uint16_t *)pc;
> > > > +
> > > > + if ( GET_INSN_LENGTH(instr16) == 2 )
> > > > + return (uint32_t)instr16;
> > > > + else
> > > > + return *(uint32_t *)pc;
> > > > +}
> > >
> > > As long as this function is only used on Xen code, it's kind of
> > > okay.
> > > There you/we control whether code can change behind our backs.
> > > But as
> > > soon as you might use this on guest code, the double read is
> > > going to
> > > be a problem
> > Will it be enough to add a comment that read_instr() should be used
> > only on Xen code? Or it is needed to introduce some lock?
>
> A comment will do for now. A lock would be problematic: It won't help
> when the function is used on non-Xen code, and since you use this in
> exception handling you may deadlock unless you carefully use a
> recursive lock.
Then I'll add a comment.
>
> > > (I think; I wonder how hardware is supposed to deal with
> > > the situation: Maybe they indeed fetch in 16-bit quantities?).
> > I thought that it reads amount of bytes corresponded to i-cache
> > size
> > and then the pipeline tracks whether an instruction is 16 or 32
> > bit.
>
> And what if an insn spans a cacheline boundary?
I think it is CPU specific, but your original assumption ( about 16-bit
fetching ) was probably right.
In RISC-V ISA doc [1] I found the following in chapter 1.2:
The base RISC-V ISA has fixed-length 32-bit instructions that must be
naturally aligned on 32-bit boundaries. However, the standard RISC-V
encoding scheme is designed to support ISA extensions with variable-
length instructions, where each instruction can be any number of 16-bit
instruction parcels in length, and parcels are naturally aligned on 16-
bit boundaries. The standard compressed ISA extension described in
Chapter 12 reduces code size by providing compressed 16-bit
instructions and relaxes the alignment constraints to allow all
instructions (16 bit and 32 bit) to be aligned on any 16-bit boundary
to improve code density.
It sounds like h/w reads 16-bit and then based on the first bits
decides if it is needed to read more 16-bit parcels.
[1] https://riscv.org/wp-content/uploads/2017/05/riscv-spec-v2.2.pdf
Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On 31.05.2023 22:06, Oleksii wrote:
> On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
>>> +static uint32_t read_instr(unsigned long pc)
>>> +{
>>> + uint16_t instr16 = *(uint16_t *)pc;
>>> +
>>> + if ( GET_INSN_LENGTH(instr16) == 2 )
>>> + return (uint32_t)instr16;
>>> + else
>>> + return *(uint32_t *)pc;
>>> +}
>>
>> As long as this function is only used on Xen code, it's kind of okay.
>> There you/we control whether code can change behind our backs. But as
>> soon as you might use this on guest code, the double read is going to
>> be a problem
> Will it be enough to add a comment that read_instr() should be used
> only on Xen code? Or it is needed to introduce some lock?
A comment will do for now. A lock would be problematic: It won't help
when the function is used on non-Xen code, and since you use this in
exception handling you may deadlock unless you carefully use a
recursive lock.
>> (I think; I wonder how hardware is supposed to deal with
>> the situation: Maybe they indeed fetch in 16-bit quantities?).
> I thought that it reads amount of bytes corresponded to i-cache size
> and then the pipeline tracks whether an instruction is 16 or 32 bit.
And what if an insn spans a cacheline boundary?
Jan
Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > + uint16_t instr16 = *(uint16_t *)pc;
> > +
> > + if ( GET_INSN_LENGTH(instr16) == 2 )
> > + return (uint32_t)instr16;
> > + else
> > + return *(uint32_t *)pc;
> > +}
>
> As long as this function is only used on Xen code, it's kind of okay.
> There you/we control whether code can change behind our backs. But as
> soon as you might use this on guest code, the double read is going to
> be a problem
Will it be enough to add a comment that read_instr() should be used
only on Xen code? Or it is needed to introduce some lock?
> (I think; I wonder how hardware is supposed to deal with
> the situation: Maybe they indeed fetch in 16-bit quantities?).
I thought that it reads amount of bytes corresponded to i-cache size
and then the pipeline tracks whether an instruction is 16 or 32 bit.
At least something similar is done for BOOM RISC-V CPU [1].
[1]
https://github.com/riscv-boom/riscv-boom/blob/master/docs/sections/instruction-fetch-stage.rst#id64
Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On 31.05.2023 12:40, Oleksii wrote:
> On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
>> On 29.05.2023 14:13, Oleksii Kurochko wrote:
>>> +static uint32_t read_instr(unsigned long pc)
>>> +{
>>> + uint16_t instr16 = *(uint16_t *)pc;
>>> +
>>> + if ( GET_INSN_LENGTH(instr16) == 2 )
>>> + return (uint32_t)instr16;
>>> + else
>>> + return *(uint32_t *)pc;
>>> +}
>>
>> As long as this function is only used on Xen code, it's kind of okay.
>> There you/we control whether code can change behind our backs. But as
>> soon as you might use this on guest code, the double read is going to
>> be a problem (I think; I wonder how hardware is supposed to deal with
>> the situation: Maybe they indeed fetch in 16-bit quantities?).
> I'll check how the hardware fetches instructions.
>
> I am trying to figure out why the double-read can be a problem. It
> looks pretty safe to read 16 bits ( they will be available for any
> instruction length with the assumption that the minimal instruction
> length is 16 ), then check the length of the instruction, and if it is
> 32-bit instruction, read it as uint32_t.
Simply consider what happens if a buggy or malicious entity changes the
code between the two reads. And not just with the detection of "break"
in mind that you use it for here.
Jan
Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On Tue, 2023-05-30 at 18:00 +0200, Jan Beulich wrote:
> On 29.05.2023 14:13, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/bug.h
> > +++ b/xen/arch/riscv/include/asm/bug.h
> > @@ -7,4 +7,32 @@
> > #ifndef _ASM_RISCV_BUG_H
> > #define _ASM_RISCV_BUG_H
> >
> > +#ifndef __ASSEMBLY__
> > +
> > +#define BUG_INSTR "ebreak"
> > +
> > +/*
> > + * The base instruction set has a fixed length of 32-bit naturally
> > aligned
> > + * instructions.
> > + *
> > + * There are extensions of variable length ( where each
> > instruction can be
> > + * any number of 16-bit parcels in length ) but they aren't used
> > in Xen
> > + * and Linux kernel ( where these definitions were taken from ).
>
> This, at least to some degree, looks to contradict ...
>
> > + * Compressed ISA is used now where the instruction length is 16
> > bit and
> > + * 'ebreak' instruction, in this case, can be either 16 or 32 bit
> > (
> > + * depending on if compressed ISA is used or not )
>
> ... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed
> insns
> can very well be used in Xen.
Thanks. You are right. The comment should be updated.
>
> > @@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct
> > cpu_user_regs *regs)
> > die();
> > }
> >
> > +void show_execution_state(const struct cpu_user_regs *regs)
> > +{
> > + printk("implement show_execution_state(regs)\n");
> > +}
> > +
> > +/*
> > + * TODO: change early_printk's function to early_printk with
> > format
> > + * when s(n)printf() will be added.
>
> What is this comment about? I don't think I understand what it says
> needs doing.
I meant that it would be nice to introduce the second version of
early_printk() function which will take 'format', as printk() does.
But there is no any sense in this comment because all early_printk() in
do_bug_frame() were changed to printk().
Thereby I will update the comment.
>
> > + * Probably the TODO won't be needed as generic do_bug_frame()
> > + * has been introduced and current implementation will be replaced
> > + * with generic one when panic(), printk() and find_text_region()
> > + * (virtual memory?) will be ready/merged
> > + */
> > +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>
> While it's going to be the maintainers to judge, I continue to be
> unconvinced that introducing copies of common functions (also in
> patch 1) is a good idea.
Generally I agree with you but as I mentioned before and in the comment
above the function do_bug_frame() the reason not to use generic
implementation of do_bug_frame() now as it will require to introduce
compilation of whole Xen's common code. ( there is no way to enable
just necessary parts for the current one function ).
I think that after this patch series I'll introduce compilation of
Xen's common code and after it'll be merged do_bug_frame() can be
removed.
>
> > +{
> > + const struct bug_frame *start, *end;
> > + const struct bug_frame *bug = NULL;
> > + unsigned int id = 0;
> > + const char *filename, *predicate;
> > + int lineno;
> > +
> > + static const struct bug_frame* bug_frames[] = {
>
> Nit: * and blank want to swap places. I would also expect another
> "const".
Thanks. I'll update that.
>
> > +static uint32_t read_instr(unsigned long pc)
> > +{
> > + uint16_t instr16 = *(uint16_t *)pc;
> > +
> > + if ( GET_INSN_LENGTH(instr16) == 2 )
> > + return (uint32_t)instr16;
> > + else
> > + return *(uint32_t *)pc;
> > +}
>
> As long as this function is only used on Xen code, it's kind of okay.
> There you/we control whether code can change behind our backs. But as
> soon as you might use this on guest code, the double read is going to
> be a problem (I think; I wonder how hardware is supposed to deal with
> the situation: Maybe they indeed fetch in 16-bit quantities?).
I'll check how the hardware fetches instructions.
I am trying to figure out why the double-read can be a problem. It
looks pretty safe to read 16 bits ( they will be available for any
instruction length with the assumption that the minimal instruction
length is 16 ), then check the length of the instruction, and if it is
32-bit instruction, read it as uint32_t.
>
> > --- a/xen/arch/riscv/xen.lds.S
> > +++ b/xen/arch/riscv/xen.lds.S
> > @@ -40,6 +40,16 @@ SECTIONS
> > . = ALIGN(PAGE_SIZE);
> > .rodata : {
> > _srodata = .; /* Read-only data */
> > + /* Bug frames table */
> > + __start_bug_frames = .;
> > + *(.bug_frames.0)
> > + __stop_bug_frames_0 = .;
> > + *(.bug_frames.1)
> > + __stop_bug_frames_1 = .;
> > + *(.bug_frames.2)
> > + __stop_bug_frames_2 = .;
> > + *(.bug_frames.3)
> > + __stop_bug_frames_3 = .;
> > *(.rodata)
> > *(.rodata.*)
> > *(.data.rel.ro)
>
> Nit: There looks to be an off-by-one in how you indent your addition
> (except for the comment).
Thanks. One space is really
Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
On 29.05.2023 14:13, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/bug.h
> +++ b/xen/arch/riscv/include/asm/bug.h
> @@ -7,4 +7,32 @@
> #ifndef _ASM_RISCV_BUG_H
> #define _ASM_RISCV_BUG_H
>
> +#ifndef __ASSEMBLY__
> +
> +#define BUG_INSTR "ebreak"
> +
> +/*
> + * The base instruction set has a fixed length of 32-bit naturally aligned
> + * instructions.
> + *
> + * There are extensions of variable length ( where each instruction can be
> + * any number of 16-bit parcels in length ) but they aren't used in Xen
> + * and Linux kernel ( where these definitions were taken from ).
This, at least to some degree, looks to contradict ...
> + * Compressed ISA is used now where the instruction length is 16 bit and
> + * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
> + * depending on if compressed ISA is used or not )
... this. Plus there already is CONFIG_RISCV_ISA_C, so compressed insns
can very well be used in Xen.
> @@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct
> cpu_user_regs *regs)
> die();
> }
>
> +void show_execution_state(const struct cpu_user_regs *regs)
> +{
> +printk("implement show_execution_state(regs)\n");
> +}
> +
> +/*
> + * TODO: change early_printk's function to early_printk with format
> + * when s(n)printf() will be added.
What is this comment about? I don't think I understand what it says
needs doing.
> + * Probably the TODO won't be needed as generic do_bug_frame()
> + * has been introduced and current implementation will be replaced
> + * with generic one when panic(), printk() and find_text_region()
> + * (virtual memory?) will be ready/merged
> + */
> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
While it's going to be the maintainers to judge, I continue to be
unconvinced that introducing copies of common functions (also in
patch 1) is a good idea.
> +{
> +const struct bug_frame *start, *end;
> +const struct bug_frame *bug = NULL;
> +unsigned int id = 0;
> +const char *filename, *predicate;
> +int lineno;
> +
> +static const struct bug_frame* bug_frames[] = {
Nit: * and blank want to swap places. I would also expect another
"const".
> +static uint32_t read_instr(unsigned long pc)
> +{
> +uint16_t instr16 = *(uint16_t *)pc;
> +
> +if ( GET_INSN_LENGTH(instr16) == 2 )
> +return (uint32_t)instr16;
> +else
> +return *(uint32_t *)pc;
> +}
As long as this function is only used on Xen code, it's kind of okay.
There you/we control whether code can change behind our backs. But as
soon as you might use this on guest code, the double read is going to
be a problem (I think; I wonder how hardware is supposed to deal with
the situation: Maybe they indeed fetch in 16-bit quantities?).
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -40,6 +40,16 @@ SECTIONS
> . = ALIGN(PAGE_SIZE);
> .rodata : {
> _srodata = .; /* Read-only data */
> +/* Bug frames table */
> + __start_bug_frames = .;
> + *(.bug_frames.0)
> + __stop_bug_frames_0 = .;
> + *(.bug_frames.1)
> + __stop_bug_frames_1 = .;
> + *(.bug_frames.2)
> + __stop_bug_frames_2 = .;
> + *(.bug_frames.3)
> + __stop_bug_frames_3 = .;
> *(.rodata)
> *(.rodata.*)
> *(.data.rel.ro)
Nit: There looks to be an off-by-one in how you indent your addition
(except for the comment).
Jan
[PATCH v6 5/6] xen/riscv: introduce an implementation of macros from
The patch introduces macros: BUG(), WARN(), run_in_exception(),
assert_failed.
To be precise, the macros from generic bug implementation ()
will be used.
The implementation uses "ebreak" instruction in combination with
diffrent bug frame tables (for each type) which contains useful
information.
Signed-off-by: Oleksii Kurochko
---
Changes in V6:
- Avoid LINK_TO_LOAD() as bug.h functionality expected to be used
after MMU is enabled.
- Change early_printk() to printk()
---
Changes in V5:
- Remove "#include " from as there is no any need in
it anymore
- Update macros GET_INSN_LENGTH: remove UL and 'unsigned int len;' from it
- Remove " include " from risc/setup.c. it is not needed in the
current version of
the patch
- change an argument type from vaddr_t to uint32_t for is_valid_bugaddr and
introduce read_instr() to
read instruction properly as the length of qinstruction can be either 32 or
16 bits.
- Code style fixes
- update the comments before do_bug_frame() in riscv/trap.c
- Refactor is_valid_bugaddr() function.
- introduce macros cast_to_bug_frame(addr) to hide casts.
- use LINK_TO_LOAD() for addresses which are linker time relative.
---
Changes in V4:
- Updates in RISC-V's :
* Add explanatory comment about why there is only defined for 32-bits length
instructions and 16/32-bits BUG_INSN_{16,32}.
* Change 'unsigned long' to 'unsigned int' inside GET_INSN_LENGTH().
* Update declaration of is_valid_bugaddr(): switch return type from int to
bool
and the argument from 'unsigned int' to 'vaddr'.
- Updates in RISC-V's traps.c:
* replace /xen and /asm includes
* update definition of is_valid_bugaddr():switch return type from int to
bool
and the argument from 'unsigned int' to 'vaddr'. Code style inside
function
was updated too.
* do_bug_frame() refactoring:
* local variables start and bug became 'const struct bug_frame'
* bug_frames[] array became 'static const struct bug_frame[] = ...'
* remove all casts
* remove unneeded comments and add an explanatory comment that the
do_bug_frame()
will be switched to a generic one.
* do_trap() refactoring:
* read 16-bits value instead of 32-bits as compressed instruction can
be used and it might happen than only 16-bits may be accessible.
* code style updates
* re-use instr variable instead of re-reading instruction.
- Updates in setup.c:
* add blank line between xen/ and asm/ includes.
---
Changes in V3:
- Rebase the patch "xen/riscv: introduce an implementation of macros
from " on top of patch series [introduce generic implementation
of macros from bug.h]
---
Changes in V2:
- Remove __ in define namings
- Update run_in_exception_handler() with
register void *fn_ asm(__stringify(BUG_FN_REG)) = (fn);
- Remove bug_instr_t type and change it's usage to uint32_t
---
xen/arch/riscv/include/asm/bug.h | 28 +++
xen/arch/riscv/traps.c | 129 +++
xen/arch/riscv/xen.lds.S | 10 +++
3 files changed, 167 insertions(+)
diff --git a/xen/arch/riscv/include/asm/bug.h b/xen/arch/riscv/include/asm/bug.h
index e8b1e40823..bf3194443f 100644
--- a/xen/arch/riscv/include/asm/bug.h
+++ b/xen/arch/riscv/include/asm/bug.h
@@ -7,4 +7,32 @@
#ifndef _ASM_RISCV_BUG_H
#define _ASM_RISCV_BUG_H
+#ifndef __ASSEMBLY__
+
+#define BUG_INSTR "ebreak"
+
+/*
+ * The base instruction set has a fixed length of 32-bit naturally aligned
+ * instructions.
+ *
+ * There are extensions of variable length ( where each instruction can be
+ * any number of 16-bit parcels in length ) but they aren't used in Xen
+ * and Linux kernel ( where these definitions were taken from ).
+ *
+ * Compressed ISA is used now where the instruction length is 16 bit and
+ * 'ebreak' instruction, in this case, can be either 16 or 32 bit (
+ * depending on if compressed ISA is used or not )
+ */
+#define INSN_LENGTH_MASK_UL(0x3)
+#define INSN_LENGTH_32 _UL(0x3)
+
+#define BUG_INSN_32 _UL(0x00100073) /* ebreak */
+#define BUG_INSN_16 _UL(0x9002) /* c.ebreak */
+#define COMPRESSED_INSN_MASK_UL(0x)
+
+#define GET_INSN_LENGTH(insn) \
+(((insn) & INSN_LENGTH_MASK) == INSN_LENGTH_32 ? 4 : 2) \
+
+#endif /* !__ASSEMBLY__ */
+
#endif /* _ASM_RISCV_BUG_H */
diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
index 48c1059954..535fb058e1 100644
--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -5,6 +5,8 @@
* RISC-V Trap handlers
*/
+#include
+#include
#include
#include
@@ -114,7 +116,134 @@ static void do_unexpected_trap(const struct cpu_user_regs
*regs)
die();
}
+void show_execution_state(const struct cpu_user_regs *regs)
+{
+printk("implement show_execution_state(regs)\n");
+}
+
+/*
+ * TODO: change early_printk's function to early_printk with format
+ * when
