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:
> > +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