Re: [Xen-devel] [PATCH 1/3] xen/arm: io: Distinguish unhandled IO from aborted one

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 31 Jan 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 30/01/18 19:09, Stefano Stabellini wrote:
> > On Tue, 30 Jan 2018, Julien Grall wrote:
> > > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > > index c8534d6cff..843adf4959 100644
> > > > > --- a/xen/arch/arm/traps.c
> > > > > +++ b/xen/arch/arm/traps.c
> > > > > @@ -1864,10 +1864,10 @@ static inline bool hpfar_is_valid(bool s1ptw,
> > > > > uint8_t fsc)
> > > > >        return s1ptw || (fsc == FSC_FLT_TRANS &&
> > > > > !check_workaround_834220());
> > > > >    }
> > > > >    -static bool try_handle_mmio(struct cpu_user_regs *regs,
> > > > > -                            const union hsr hsr,
> > > > > -                            paddr_t gpa)
> > > > > -{
> > > > > +static enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> > > > > +                                     const union hsr hsr,
> > > > > +                                     paddr_t gpa)
> > > > > + {
> > > > >        const struct hsr_dabt dabt = hsr.dabt;
> > > > >        mmio_info_t info = {
> > > > >            .gpa = gpa,
> > > > > @@ -1879,11 +1879,11 @@ static bool try_handle_mmio(struct
> > > > > cpu_user_regs
> > > > > *regs,
> > > > >          /* stage-1 page table should never live in an emulated MMIO
> > > > > region
> > > > > */
> > > > >        if ( dabt.s1ptw )
> > > > > -        return false;
> > > > > +        return IO_UNHANDLED;
> > > > >          /* All the instructions used on emulated MMIO region should
> > > > > be
> > > > > valid */
> > > > >        if ( !dabt.valid )
> > > > > -        return false;
> > > > > +        return IO_UNHANDLED;
> > > > >          /*
> > > > >         * Erratum 766422: Thumb store translation fault to Hypervisor
> > > > > may
> > > > > @@ -1896,11 +1896,11 @@ static bool try_handle_mmio(struct
> > > > > cpu_user_regs
> > > > > *regs,
> > > > >            if ( rc )
> > > > >            {
> > > > >                gprintk(XENLOG_DEBUG, "Unable to decode
> > > > > instruction\n");
> > > > > -            return false;
> > > > > +            return IO_ABORT;
> > > > >            }
> > > > >        }
> > > > 
> > > > Why do the first two error checks result in IO_UNHANDLED, while the
> > > > third result in IO_ABORT? Specifically in relation to pagetable walk
> > > > failures due to someone else changing stage-2 entry simultaneously (see
> > > > comment toward the end of do_trap_stage2_abort_guest)?
> > > 
> > > Good question. Somehow I considered the first two as part of looking up
> > > the
> > > proper handler and not the device itself.
> > > 
> > > But I guess that at this stage we know that IO was targeting an emulated
> > > region. So we can return IO_ABORT.
> > 
> > That is what I thought as well
> 
> Actually, I have said something completely wrong yesterday. Must have been too
> tired :/.
> 
> At the time you call try_handle_mmio, you still don't know whether the fault
> was because of accessing an emulated MMIO region. You will only be sure when
> find_mmio_handler() has returned a non-NULL pointer (see handle_mmio()).
> 
> So returning IO_UNHANDLED here is correct as you want to try another method to
> handle the fault.
> 
> However, it also means that even bad access to emulated region will result to
> fallback on another method. While this should not be issue, I don't think this
> is future proof (I am mostly worry on the ACPI case where MMIO are mapped
> on-demand).
> 
> So I will send a patch to fold try_handle_mmio() into handle_mmio().

All right


> > 
> > 
> > > On a side note, it looks like the check dabt.s1ptw is unnecessary because
> > > a
> > > stage 2 abort on stage 1 translation table lookup will not return a valid
> > > instruction syndrome (see B3-1433 in DDI 0406C.c and D10-2460 in DDI
> > > 0487C.a).
> > 
> > in that case, go ahead and remove it as part of this patch, mention it
> > in the commit message
> 
> I will do that in a patch that fold try_handle_mmio() in handle_mmio().
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel