Re: [XEN v7 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 Stefano,

Thanks for your feedback.

On 09/02/2022 01:50, Stefano Stabellini wrote:
> On Sat, 5 Feb 2022, Ayan Kumar Halder wrote:
<snip>
> > @@ -95,6 +97,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
> >   bool arch_ioreq_complete_mmio(void)
> >   {
> >       struct vcpu *v = current;
> > +    struct instr_details *dabt_instr = v->io.info.dabt_instr;
> >       struct cpu_user_regs *regs = guest_cpu_user_regs();
> >       const union hsr hsr = { .bits = regs->hsr };
> I don't think that v->io.info.dabt_instr should be a pointer because at
> this point the original structure is long gone. The original field was a
> local variable mmio_info_t info in do_trap_stage2_abort_guest. So by the
> time arch_ioreq_complete_mmio is called, the original variable has been
> deallocated.
>
> Instead, v->io.info.dabt_instr should be a "struct instr_details" (no
> pointer).
I see what you mean.

arch_ioreq_complete_mmio() is called from leave_hypervisor_to_guest(). 
That is after do_trap_stage2_abort_guest
()  has been invoked. So the original variable is no longer valid.

<snip>

> +
> +            /*
> +             * When the instruction needs to be retried by the guest after
> +             * resolving the translation fault.
> +             */
> +            else if ( info.dabt_instr.state == INSTR_RETRY )
> +                goto set_page_tables;
As discussed with Julien on IRC, when hsr_dabt.s1ptw == 1, Xen should 
only invoke p2m_resolve_translation_fault(). If it returns an error, it 
should send an abort to the guest. The reason being there is no need to 
invoke try_map_mmio() as the gpa is not a mmio address. Also, Xen does 
not support the use case when the guest places the page tables in the 
MMIO region.
I will wait for Julien's comments before sending out a v8 patch.

- Ayan

> +
> +            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:
> @@ -1985,18 +2056,12 @@ static void do_trap_stage2_abort_guest(struct 
> cpu_user_regs *regs,
>               }
>           }
>   
> -        /*
> -         * First check if the translation fault can be resolved by the
> -         * P2M subsystem. If that's the case nothing else to do.
> -         */
> -        if ( p2m_resolve_translation_fault(current->domain,
> -                                           gaddr_to_gfn(gpa)) )
> -            return;
> -
> -        if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
> + set_page_tables:
> +        if ( check_p2m(is_data, gpa) )
>               return;
>   
>           break;
> +    }
>       default:
>           gprintk(XENLOG_WARNING,
>                   "Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
> diff --git a/xen/arch/x86/include/asm/ioreq.h b/xen/arch/x86/include/asm/ioreq.h
> index d06ce9a6ea..ecfe7f9fdb 100644
> --- a/xen/arch/x86/include/asm/ioreq.h
> +++ b/xen/arch/x86/include/asm/ioreq.h
> @@ -26,6 +26,9 @@
>   #include <asm/hvm/ioreq.h>
>   #endif
>   
> +struct arch_vcpu_io {
> +};
> +
>   #endif /* __ASM_X86_IOREQ_H__ */
>   
>   /*
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 37f78cc4c4..afe5508be8 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -160,6 +160,8 @@ struct vcpu_io {
>       /* I/O request in flight to device model. */
>       enum vio_completion  completion;
>       ioreq_t              req;
> +    /* Arch specific info pertaining to the io request */
> +    struct arch_vcpu_io  info;
>   };
>   
>   struct vcpu
> --
> 2.17.1