Re: [PATCH v3 08/18] xen/arm32: head: Introduce an helper to flush the TLBs

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 12/12/2022 10:55, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The sequence for flushing the TLBs is 4 instruction long and often
> requires an explanation how it works.
> 
> So create an helper and use it in the boot code (switch_ttbr() is left
Here and in title: s/an helper/a helper/

> alone for now).
Could you explain why?

> 
> Note that in secondary_switched, we were also flushing the instruction
> cache and branch predictor. Neither of them was necessary because:
>     * We are only supporting IVIPT cache on arm32, so the instruction
>       cache flush is only necessary when executable code is modified.
>       None of the boot code is doing that.
>     * The instruction cache is not invalidated and misprediction is not
>       a problem at boot.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Apart from that, the patch is good, so:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

> 
> ---
>     Changes in v3:
>         * Fix typo
>         * Update the documentation
>         * Rename the argument from tmp1 to tmp
> ---
>  xen/arch/arm/arm32/head.S | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 40c1d7502007..315abbbaebec 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -66,6 +66,20 @@
>          add   \rb, \rb, r10
>  .endm
> 
> +/*
> + * Flush local TLBs
> + *
> + * @tmp:    Scratch register
As you are respinning a series anyway, could you add just one space after @tmp:?

~Michal