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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 02/02/18 14:47, Julien Grall wrote:
> 
> 
> 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.

Yeah, fair enough. Fine for me, then.

Cheers,
Andre.

>> 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@xxxxxxx>
>>
>> Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> 
> Thanks!
> 
> Cheers,
> 

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