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

[Julien Grall <julien.grall@xxxxxxx>]

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

Cheers,

--
Julien Grall

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