Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/03/2022 11:27, Ayan Kumar Halder wrote:
> > > +        {
> > > +            rc = decode_instruction(regs, info);
> > > +            if ( rc )
> > > +            {
> > > +                gprintk(XENLOG_DEBUG, "Unable to decode 
> > > instruction\n");
> > > +                info->dabt_instr.state = INSTR_ERROR;
> > > +        }
> > > +        return;
> > > +    }
> > > +
> > > +    /*
> > > +     * Armv8 processor does not provide a valid syndrome for 
> > > decoding some
> > > +     * instructions. So in order to process these instructions, Xen 
> > > must
> > > +     * decode them.
> > > +     */
> > > +    rc = decode_instruction(regs, info);
> > > +    if ( rc )
> > > +    {
> > > +        gprintk(XENLOG_ERR, "Unable to decode instruction\n");
> > > +        info->dabt_instr.state = INSTR_ERROR;
> > > +    }
> > > +}
> > > +
> > >   enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> > > -                              const union hsr hsr,
> > > -                              paddr_t gpa)
> > > +                              mmio_info_t *info)
> > >   {
> > >       struct vcpu *v = current;
> > >       const struct mmio_handler *handler = NULL;
> > > -    const struct hsr_dabt dabt = hsr.dabt;
> > > -    mmio_info_t info = {
> > > -        .gpa = gpa,
> > > -        .dabt = dabt
> > > -    };
> > > +    int rc;
> > >   -    ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> > > +    ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> > >   -    handler = find_mmio_handler(v->domain, info.gpa);
> > > -    if ( !handler )
> > > +    if ( !((info->dabt_instr.state == INSTR_VALID) || 
> > > (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) )
> > This check will become quite large if we decode more class. I would 
> > instead set the dabt.valid bit whenever we successfully decoded the 
> > instruction and check that if dabt.valid here.
> Actually the main reason to introduce INSTR_LDR_STR_POSTINDEXING is to 
> distinguish the scenario where the ISS was valid vs when instruction was 
> decoded manually.
> In the later scenario, one would need to do the post increment of the rn.
>
> It makes sense to me to have a unque 'info->dabt_instr.state' for each 
> type of instruction decoded as the post processing will vary. In this 
> case, the post processing logic checks that the instruction is 
> ldr_str_postindexing.
So I agree we want to have a unique state for type of instruction. I 
wasn't suggesting to remove it. Instead, I was suggesting to use 
dabt.valid as "This is a valid instruction for accessing an emulated MMIO".
> However your concern that the check will become large is valid. I would 
> introduce a function as follows :-
> bool check_instr_is_valid(enum instr_decode_state state)
>
> {
>
>      if (state == INSTR_VALID) || (state == INSTR_LDR_STR_POSTINDEXING) 
> || ...)
>          return true;
>
>      else
>
>          return false;
>
> }
>
> And then in
>
> enum io_state try_handle_mmio(struct cpu_user_regs *regs, ...)
>
> {
>
> ...
>
>      if ( !check_instr_is_valid(info->dabt_instr.state) )
>
>      {
>
>          ASSERT_UNREACHABLE();
>          return IO_ABORT;
>
>      }
>
> ...
>
> }
>
> Please let me know your thoughts,
This is only moving the check to a separate function. It doesn't help 
with the fact that the check in check_instr_is_valid() is going to grow.
I can see two options:
  * Using dabt.valid as "The instruction was fully decoded".
  * Check that the state is not INSTR_ERROR

Above, I was suggesting the former. But I am open to use latter.

Cheers,

--
Julien Grall