Re: [PATCH v6 5/6] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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 absent...

~ Oleksii