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

[Julien Grall <julien@xxxxxxx>]

On 26/01/2022 12:21, Ayan Kumar Halder wrote:
> Hi Julien/Stefano,
Hi,

> On 25/01/2022 23:02, Julien Grall wrote:
> > > +    }
> > > +
> > > +    /*
> > > +     * Handle when rn = SP
> > > +     * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer 
> > > register selection"
> > > +     * As we are interested in handling exceptions only from EL1 in 
> > > AArch64 state,
> > > +     * thus M[3:0] == EL1h (Page - C5-480 "When exception taken from 
> > > AArch64 state:")
> > I read the last sentence as "We only support decoding from instruction 
> > run at EL1". But I can't find a check guarantee that.
> Sorry, the check below does that but is used only for rn == 31. I should 
> move ((regs->cpsr & PSR_MODE_MASK) != PSR_MODE_EL1h) ) into a separate 
> check of its own.
The question is why do we want to limit instruction decoding to EL1?

> > > +     */
> > > +    if ( (instr->code.rn == 31) && ((regs->cpsr & PSR_MODE_MASK) != 
> > > PSR_MODE_EL1h) )
> > > +    {
> > > +        gprintk(XENLOG_ERR, "SP is valid only for EL1h\n");
> > > +        goto bad_loadstore;
> > > +    }
> > > +
> > > +    if ( instr->code.v != 0 )
> > > +    {
> > > +        gprintk(XENLOG_ERR,
> > > +            "ldr/str post indexing for vector types are not 
> > > supported\n");
> > > +        goto bad_loadstore;
> > > +    }
> > > +
> > > +    /* Check for STR (immediate) */
> > > +    if ( instr->code.opc == 0 )
> > > +    {
> > > +        dabt->write = 1;
> > > +    }
> > Coding style: We don't use {} for single line. In this case, it would 
> > also result to have a more readable code.
> > > +    /* Check for LDR (immediate) */
> > > +    else if ( instr->code.opc == 1 )
> > > +    {
> > > +        dabt->write = 0;
> > > +    }
> > Same.
> >
> > > +    else
> > > +    {
> > > +        gprintk(XENLOG_ERR,
> > > +            "Decoding ldr/str post indexing is not supported for 
> > > this variant\n");
> > The indentation looks wrong here.
> >
> > > +        goto bad_loadstore;
> > > +    }
> > > +
> > > +    gprintk(XENLOG_INFO,
> > > +        "instr->code.rt = 0x%x, instr->code.size = 0x%x, 
> > > instr->code.imm9 = %d\n",
> > > +        instr->code.rt, instr->code.size, instr->code.imm9);
> > The indentation looks wrong here.
> >
> > > +
> > > +    update_dabt(dabt, instr->code.rt, instr->code.size, false);
> > > +    dabt->valid = 1;
> > > +
> > > +    return 0;
> > > +
> > > + bad_loadstore:
> > > +    gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n", 
> > > instr->value);
> > > +    return 1;
> > > +}
> > > +
> > >   static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
> > >   {
> > >       uint16_t instr;
> > > @@ -150,17 +245,44 @@ bad_thumb:
> > >       return 1;
> > >   }
> > >   -int decode_instruction(const struct cpu_user_regs *regs, struct 
> > > hsr_dabt *dabt)
> > > +int decode_instruction(const struct cpu_user_regs *regs, struct 
> > > hsr_dabt *dabt,
> > > +                       union ldr_str_instr_class *instr)
> > I would like to avoid make the assumption that the instr we decode 
> > will always be a store/load. So please rename it to something more 
> > generic.
> A difference of thought. Should we not name it as per the current usage 
> ? This will avoid any ambiguity.
What ambiguity? The caller should be completely agnostic to what 
instruction we decode.
> Later, if this gets extended, then it 
> can be named more appropriately depending on the usage.
Renaming afterwards is exactly what I want to avoid.

> Also, the bit-pattern in "union ldr_str_instr_class" is very much 
> specific to ldr/str.
I agree that the bit-pattern is specific to load/store. But that's just 
one way to interpret the 32-bit value. It would be easy to add another 
way to interpret it. I.e:
union
{
   value;
   struct {
   } str_ldr;
   struct {
   } brk;
}

> > I think moving if ( !dabt.valid ) earlier should be part of a 
> > pre-patch. This would allows us to backport it as we don't want to 
> > forward the I/O to an IOREQ server if ISV=0.
> I would say that in the pre-patch, we should move "if ( !dabt.valid )" 
> before "find_mmio_handler()". The reason being if the intruction was not 
> decoded successfully (ie ISV=0), then there is no need to find the mmio 
> handler corresponding to the gpa. Please let me know your thoughts and I 
> can send the pre-patch.
Sounds good to me.

> > >       /*
> > >        * Erratum 766422: Thumb store translation fault to Hypervisor may
> > >        * not have correct HSR Rt value.
> > > @@ -134,7 +145,7 @@ enum io_state try_handle_mmio(struct 
> > > cpu_user_regs *regs,
> > >       {
> > >           int rc;
> > >   -        rc = decode_instruction(regs, &info.dabt);
> > > +        rc = decode_instruction(regs, &info.dabt, NULL);
> > Could we combine the two decode_instruction()?
> Do you mean something like this :-
Yes. But...

> if ( (!info.dabt.valid ) || (( check_workaround_766422() && (regs->cpsr 
> & PSR_THUMB) &&
>           dabt.write) )
>
> {
>
>          rc = decode_instruction(regs, &info.dabt, &instr); // We know 
> that for PSR_THUMB, instr is ignored.
... without this comment. Also see my reply to Stefano's email.

[...]

> So you mean something like this (in traps.c):-
>
> #if CONFIG_ARM_64
>
> void post_increment_register(union ldr_str_instr_class *instr)
>
> {
>
>      // handle the post increment
>
> }
>
> #else
>
> void post_increment_register(union ldr_str_instr_class *instr)
>
> {
>
>      ASSERT_UNREACHABLE();
>
> }
>
> #endif
>
> If so, I am fine with this.
Yes.

Cheers,

--
Julien Grall