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

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

Hi Julien,

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.
<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.
         */
        if ( info.dabt.valid && check_p2m(is_data, gpa) )
            return;

        break;
    }
    default:

<snip>

- Ayan

> Cheers,