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

[Julien Grall <julien.grall@arm.com>]

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().

> 
> 
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel