Re: [PATCH v3 1/4] xen/arm: traps: Merge try_handle_mmio() and handle_mmio()

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

On 02/02/18 14:34, Andre Przywara wrote:
> Hi,

Hi,

> On 02/02/18 10:14, Julien Grall wrote:
>> At the moment, try_handle_mmio() will do check on the HSR and bail out
>> if one check fail. This means that another method will be tried to
>> handle the fault even for bad access on emulated region. While this
>> should not be an issue, this is not future proof.
>>
>> Move the checks of try_handle_mmio() in handle_mmio() after we identified
>> the fault to target an emulated MMIO. While this does not fix the potential
>> fall-through, a follow-up patch will do by distinguish the potential error.
>>
>> Note that the handle_mmio() was renamed to try_handle_mmio() and the
>> prototype adapted.
> 
> Why is that? I think the prefix "try_" only makes sense if you have a
> non-try version as well. To some degree most functions "try" something,
> when they check for and return errors.
> I think the return type makes it clear what the semantics are.
> So personally I would prefer just "handle_mmio" as the function name.
Because we have another function called try_map_mmio() just below, so I 
wanted to keep similar.

Also, I think "try_" makes sense here because if you don't succeed, you 
will fallback to another method. Most of the times, this is not the case 
of other functions.

> 
> But this only a nit, definitely not worth a respin on its own.
> 
>> While merging the 2 functions, remove the check whether the fault is
>> stage-2 abort on stage-1 translation walk because the instruction
>> syndrome will always be invalid (see B3-1433 in DDI 0406C.c and
>> D10-2460 in DDI 0487C.a).
> 
> Yes, that looks correct to me.
> 
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks!

Cheers,

-- 
Julien Grall

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