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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
Cheers,

--
Julien Grall