[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


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.


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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.