Re: [XEN v4] xen/arm64: io: Decode ldr/str post-indexing instructions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 25 Jan 2022, Julien Grall wrote:
> > +
> >       /* TODO: Handle ARM instruction */
> >       gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
> >         return 1;
> >   }
> >   +#if CONFIG_ARM_64
> > +void post_increment_register(union ldr_str_instr_class *instr)
> 
> instr should not be modified, so please use const. Also, it would be
> preferrable to pass the regs in parameter. So the none of the decoding code
> relies on the current regs.
> 
> Furthermore, decode.c should only contain code to update the syndrome and in
> theory Arm could decide to provide an valid syndrome in future revision. So I
> would move this code in io.c (or maybe traps.c).

I was the one to suggest moving it to decode.c to keep it closer to the
decoding function it is related to, and also because it felt a bit out
of place in io.c.

I don't feel strongly about this at all; I am fine either way.


> > +{
> > +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> > +    register_t val;
> > +
> > +    /* handle when rn = SP */
> > +    if ( instr->code.rn == 31 )
> > +        val = regs->sp_el1;
> > +    else
> > +        val = get_user_reg(regs, instr->code.rn);
> > +
> > +    val += instr->code.imm9;
> > +
> > +    if ( instr->code.rn == 31 )
> > +        regs->sp_el1 = val;
> > +    else
> > +        set_user_reg(regs, instr->code.rn, val);
> > +}
> > +#endif
> > +
> >   /*
> >    * Local variables:
> >    * mode: C
> > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> > index 4613763bdb..511cd4a05f 100644
> > --- a/xen/arch/arm/decode.h
> > +++ b/xen/arch/arm/decode.h
> > @@ -23,6 +23,35 @@
> >   #include <asm/regs.h>
> >   #include <asm/processor.h>
> >   +/*
> > + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
> > + * Page 318 specifies the following bit pattern for
> > + * "load/store register (immediate post-indexed)".
> > + *
> > + * 31 30 29  27 26 25  23   21 20              11   9         4       0
> > + * ___________________________________________________________________
> > + * |size|1 1 1 |V |0 0 |opc |0 |      imm9     |0 1 |  Rn     |  Rt   |
> > + * |____|______|__|____|____|__|_______________|____|_________|_______|
> > + */
> > +union ldr_str_instr_class {
> > +    uint32_t value;
> > +    struct ldr_str {
> > +        unsigned int rt:5;     /* Rt register */
> > +        unsigned int rn:5;     /* Rn register */
> > +        unsigned int fixed1:2; /* value == 01b */
> > +        signed int imm9:9;            /* imm9 */
> > +        unsigned int fixed2:1; /* value == 0b */
> > +        unsigned int opc:2;    /* opc */
> > +        unsigned int fixed3:2; /* value == 00b */
> > +        unsigned int v:1;      /* vector */
> > +        unsigned int fixed4:3; /* value == 111b */
> > +        unsigned int size:2;   /* size */
> > +    } code;
> > +};
> 
> Looking at the code, post_increment_register() only care about 'rn' and
> 'imm9'. So rather than exposing the full instruction, could we instead provide
> the strict minimum? I.e something like:
> 
> struct
> {
>      enum instr_type; /* Unknown, ldr/str post increment */
>      union
>      {
>          struct
>          {
>            register; /* Register to increment */
>            imm;      /* Immediate to add */
>          } ldr_str;
>      }
>      uint64_t register;
> }
 
The full description helped a lot during review. I would prefer to keep
it if you don't feel strongly about it.