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.
