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

[Julien Grall <julien@xen.org>]

On 21/02/2022 21:10, Ayan Kumar Halder wrote:
> Hi Julien,

Hi,

> 
> On 21/02/2022 19:13, Julien Grall wrote:
>>
>>
>> On 21/02/2022 19:05, Ayan Kumar Halder wrote:
>>> If we (ie Xen) didn't decode the instruction manually, then 
>>> check_p2m() has not been invoked yet.  This is because of the 
>>> following (info.dabt.valid == True) :-
>>>
>>>          if ( !is_data || !info.dabt.valid )
>>>          {
>>>
>>>                  ...
>>>
>>>                  if ( check_p2m(is_data, gpa) )
>>>                      return;
>>>
>>>                  ...
>>>
>>>          }
>>>
>>> So, in this scenario ( !info.dabt.valid), it would not be necessary 
>>> to invoke check_p2m() after try_handle_mmio().
>>>
>>> However, if we havenot decoded the instruction manually (ie 
>>> info.dabt.valid == True), and try_handle_mmio() returns IO_UNHANDLED, 
>>> then it will be necessary to invoke "check_p2m(is_data, gpa)"
>>
>> Hmmm you are right. But this doesn't seem to match the code you wrote 
>> below. What did I miss?
> 
> My code was not correct.  I have rectified it as below. Please let me 
> know if it looks sane.

This looks good to me with one remark below.

> 
> <snip>
> 
>      case FSC_FLT_TRANS:
>      {
>          info.gpa = gpa;
>          info.dabt = hsr.dabt;
> 
>          /*
>           * Assumption :- Most of the times when we get a data abort and 
> the ISS
>           * is invalid or an instruction abort, the underlying cause is 
> that the
>           * page tables have not been set up correctly.
>           */
>          if ( !is_data || !info.dabt.valid )
>          {
>              if ( check_p2m(is_data, gpa) )
>                  return;
> 
>              /*
>               * If the instruction abort could not be resolved by 
> setting the
>               * appropriate bits in the translation table, then Xen should
>               * forward the abort to the guest.
>               */
>              if ( !is_data )
>                  goto inject_abt;
> 
>              try_decode_instruction(regs, &info);
> 
>              /*
>               * If Xen could not decode the instruction or encountered 
> an error
>               * while decoding, then it should forward the abort to the 
> guest.
>               */
>              if ( info.dabt_instr.state == INSTR_ERROR )
>                  goto inject_abt;
>          }
> 
>          state = try_handle_mmio(regs, &info);
> 
>          switch ( state )
>          {
>              case IO_ABORT:
>                  goto inject_abt;
>              case IO_HANDLED:
>                  /*
>                   * If the instruction was decoded and has executed 
> successfully
>                   * on the MMIO region, then Xen should execute the next 
> part of
>                   * the instruction. (for eg increment the rn if it is a
>                   * post-indexing instruction.
>                   */
>                  post_increment_register(&info.dabt_instr);
>                  advance_pc(regs, hsr);
>                  return;
>              case IO_RETRY:
>                  /* finish later */
>                  return;
>              case IO_UNHANDLED:
>                  /* IO unhandled, try another way to handle it. */
>                  break;
>          }
> 
>          /*
>           * If the instruction was valid but Xen could not emulate the 
> instruction
>           * then it should configure the page tables to set the correct 
> page table
>           * entry corresponding to the faulting address. If it was 
> successful, it
>           * should return to the guest to retry the instruction (hoping 
> that the
>           * instruction will not be trapped to Xen again).
>           * However, if Xen was not successful in setting the page 
> tables, then
>           * it should forward the abort to the guest.
>           */

I would shorten to:

If the instruction syndrome was invalid, then we already checked if this 
was due to a P2M fault. So no point to check again as the result will be 
the same.

Cheers,

-- 
Julien Grall