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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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