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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 26 Jan 2022, Julien Grall wrote:
> On 26/01/2022 01:45, Stefano Stabellini wrote:
> > 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.
> 
> How about traps.c? This is where we also take care of incrementing pc after we
> handle a MMIO trap.
> 
> > > > +{
> > > > +    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 {
> 
> No need to name the struct here.
> 
> > > > +        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;
> 
> It would be best to name it ldr_str so this can be easily extended (e.g. no
> renaming) for other instructions in the future.
> 
> > > > +};
> > > 
> > > 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.
> 
> I haven't suggested to drop the union. Instead, I am suggesting to keep it
> internally to decode.c and expose something different to the external the
> user. The idea is the caller doesn't care about the full instruction, it only
> cares about what action to do.
> 
> Basically, what I am asking is an augmented dabt. So all the information are
> in one place rather than having to carry two structure (struct hsr_dabt and
> union ldr_str_instr_class) which contain mostly redundant information.

That could be a good idea. We shouldn't need "union ldr_str_instr_class"
in io.c, it should only be needed in decode.c. Thus, it could be
internal to decode.c. That's fine by me.