Re: [PATCH v3 23/39] xen/riscv: add helpers for decoding a trapped load or store

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

On 2026-11-04 17:38 +0100, Oleksii Kurochko wrote:
> [ ... 147 lines skipped ... ]
> >> +        /*
> >> +         * Bit[0] == 0 implies trapped instruction value is zero or 
> >> special
> >> +         * value. With the pseudoinstructions ruled out above, only zero 
> >> (or a
> >> +         * custom value, see above) is left: the instruction has to be 
> >> read from
> >> +         * guest memory.
> > Nit: as the assert would disappears in release build, this comment is
> > partially true. I would suggest something like:
> > ```
> >           * Bit[0] == 0 implies trapped instruction value is zero or
> >           * special value. With the pseudoinstructions ruled out (see
> >           * comment above) only zero (or a custom value, see above) is
> >           * left: the instruction has to be read from guest memory.
> > ```
> > 
> 
> Agree "ruled out" could be read incorrectly. I think it will be better then:
> 
>           /*
>            * Bit[0] == 0 implies trapped instruction value is zero or 
> special
> -         * value. With the pseudoinstructions ruled out above, only 
> zero (or a
> -         * custom value, see above) is left: the instruction has to be 
> read from
> -         * guest memory.
> +         * value. Pseudoinstructions never get here (see the comment 
> ahead of
> +         * the ASSERT() above), so only zero (or a custom value, see 
> above) is
> +         * left: the instruction has to be read from guest memory.
>            */
Agree.
> [ ... 6 lines skipped ... ]
> >> +            /*
> >> +             * If during getting of trapped instruction a fault happen in
> >> +             * G-stage translation then CAUSE_LOAD_GUEST_PAGE_FAULT is
> >> +             * generated. Such faults during this operation is considered 
> >> as
> >> +             * bus error.
> > Typo: "Such faults ... are considered" (plural subject). Also "a fault
> > happen" above should be "a fault happens", and "as bus error" -> "a
> > bus error". Maybe:
> > 
> >      /*
> >      * If a G-stage translation fault happens while fetching the
> >      * trapped instruction, CAUSE_LOAD_GUEST_PAGE_FAULT is generated.
> >      * Such faults are treated as an access fault.
> >      */
> > I used access fault instead of bus error because the code below sets
> > CAUSE_FETCH_ACCESS which is more like an access fault.
> > 
> 
> Suggestion LGTM with one nit that "... instruction acccess fault" + 
> mentioned of that it is because of hlvx instruction so:
> 
>               /*
> -             * If during getting of trapped instruction a fault happen in
> -             * G-stage translation then CAUSE_LOAD_GUEST_PAGE_FAULT is
> -             * generated. Such faults during this operation is 
> considered as
> -             * bus error.
> +             * If a G-stage translation fault happens while fetching the
> +             * trapped instruction, CAUSE_LOAD_GUEST_PAGE_FAULT is 
> generated,
> +             * hlvx being a load. Such a fault is reported to the guest 
> as an
> +             * instruction access fault.
Agree.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>