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 18:14, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Julien Grall wrote:
>> Currently, Xen is considering that an IO could either be handled or
>> unhandled. When unhandled, the stage-2 abort function will try another
>> way to resolve the abort.
>>
>> However, the MMIO emulation may return unhandled when the address
>> belongs to an emulated range but was not correct. In that case, Xen
>> should avodi to try another way and directly inject a guest data abort.
>           ^ avoid
> 
> 
>> Introduce a tri-state return to distinguish the following state:
>>      * IO_ABORT: The IO was handled but resulted to an abort
>                                                    ^ in an abort
> 
> 
>>      * IO_HANDLED: The IO was handled
>>      * IO_UNHANDLED: The IO was unhandled
>>
>> For now, it is considered that an IO belonging to an emulated range
>> could either be handled or inject an abort. This could be revisit in the
>> future if overlapped region exist (or we want to try another way to
>> resolve the abort).
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/io.c          | 24 ++++++++++++++----------
>>   xen/arch/arm/traps.c       | 34 +++++++++++++++++++++++-----------
>>   xen/include/asm-arm/mmio.h |  9 ++++++++-
>>   3 files changed, 45 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index c748d8f5bf..a74ec21b86 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -23,8 +23,9 @@
>>   #include <asm/current.h>
>>   #include <asm/mmio.h>
>>   
>> -static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
>> -                       mmio_info_t *info)
>> +static enum io_state handle_read(const struct mmio_handler *handler,
>> +                                 struct vcpu *v,
>> +                                 mmio_info_t *info)
>>   {
>>       const struct hsr_dabt dabt = info->dabt;
>>       struct cpu_user_regs *regs = guest_cpu_user_regs();
>> @@ -37,7 +38,7 @@ static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
>>       uint8_t size = (1 << dabt.size) * 8;
>>   
>>       if ( !handler->ops->read(v, info, &r, handler->priv) )
>> -        return 0;
>> +        return IO_ABORT;
>>   
>>       /*
>>        * Sign extend if required.
>> @@ -57,17 +58,20 @@ static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
>>   
>>       set_user_reg(regs, dabt.reg, r);
>>   
>> -    return 1;
>> +    return IO_HANDLED;
>>   }
>>   
>> -static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
>> -                        mmio_info_t *info)
>> +static enum io_state handle_write(const struct mmio_handler *handler,
>> +                                  struct vcpu *v,
>> +                                  mmio_info_t *info)
>>   {
>>       const struct hsr_dabt dabt = info->dabt;
>>       struct cpu_user_regs *regs = guest_cpu_user_regs();
>> +    int ret;
>>   
>> -    return handler->ops->write(v, info, get_user_reg(regs, dabt.reg),
>> -                               handler->priv);
>> +    ret = handler->ops->write(v, info, get_user_reg(regs, dabt.reg),
>> +                              handler->priv);
>> +    return ( ret ) ? IO_HANDLED : IO_ABORT;
>>   }
>>   
>>   /* This function assumes that mmio regions are not overlapped */
>> @@ -100,14 +104,14 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
>>       return handler;
>>   }
>>   
>> -int handle_mmio(mmio_info_t *info)
>> +enum io_state handle_mmio(mmio_info_t *info)
>>   {
>>       struct vcpu *v = current;
>>       const struct mmio_handler *handler = NULL;
>>   
>>       handler = find_mmio_handler(v->domain, info->gpa);
>>       if ( !handler )
>> -        return 0;
>> +        return IO_UNHANDLED;
>>   
>>       if ( info->dabt.write )
>>           return handle_write(handler, v, info);
>> 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.

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

Cheers,

-- 
Julien Grall

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