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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/5/26 9:17 AM, Jan Beulich wrote:
> On 04.11.2026 17:38, Oleksii Kurochko wrote:
> > On 10/22/26 12:02 PM, Baptiste Le Duc wrote:
> > > > + * It is needed to decode a trapped instruction: the encodings which exist only
> > > > + * for XLEN=64 must not be recognized for a 32-bit guest. Besides those simply
> > > > + * being reserved there, the compressed ones are ambiguous: C.LD and C.FLW
> > > > + * share the encoding 0x6000 (mask 0xe003), and likewise C.SD/C.FSW,
> > > > + * C.LDSP/C.FLWSP and C.SDSP/C.FSWSP.
> > > > + *
> > > > + * IS_ENABLED() can't be used here as HSTATUS_VSXL is defined for
> > > > + * __riscv_xlen == 64 only, the field not existing on RV32 in the first place.
> > > > + */
> > > > +static unsigned int guest_xlen(const struct cpu_user_regs *regs)
> > > > +{
> > > > +#ifdef CONFIG_RISCV_32
> > > > +    return 32;
> > > > +#else
> > > > +    unsigned long xl = MASK_EXTR(regs->hstatus, HSTATUS_VSXL);
> > > xl only ever holds a 2-bit field, so unsigned int would be better here.
> > MASK_EXTR() yields the type of its operands, unsigned long here (and
> > unsigned long long for the UXL extraction), as do the XLEN_FIELD_*
> > constants xl is compared against. Making xl unsigned int would add an
> > implicit narrowing conversion: harmless value-wise but I think it will
> > violate one of MISRA rules at least if build everything with
> > -Wconversion then it will tell: conversion from 'long unsigned int' to
> > 'unsigned int' may change value.
> If we were to follow that Misra rule, we'd have violations all over the
> place. Hence imo where unsigned int is sufficient for a value, that's
> what should be used.
Fair enough, I'll switch xl to unsigned int.

The one thing which isn't fully clear IMO is that in misra/rules.rst it 
is mentioned for Rule 10.3:
     - Please beware that this rule has many violations in the Xen
       codebase today, and its adoption is aspirational. However, when
       submitting new patches please try to decrease the number of
       violations when possible.
then it isn't  clear when it is needed to apply second sentence and when 
no.
> > > > [ ... 49 lines skipped ... ]
> > > > +static unsigned long *guest_gpr(struct cpu_user_regs *regs,
> > > > +                                const struct decoded_insn *di)
> > > > +{
> > > > +    /*
> > > > +     * decode_ldst_insn() is what fills @di in, and it rejects an access
> > > > +     * wider than a guest register.
> > > > +     */
> > > > +    ASSERT(di->len <= (guest_xlen(regs) / BITS_PER_BYTE));
> > > Nit: the parentheses around the division aren't needed.
> > I can drop them but I think it improves readability here a little bit.
> That's subjective. Personally I agree with Baptiste, yet project-wide
> we don't do things consistently in this regard, and hence either form
> is (supposedly) okay.
Probably then it will be nice to update CODING_STYLE document.

~ Oleksii