Re: [PATCH v5 1/5] xen/arm32: head: Widen the use of the temporary mapping

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 27/01/2023 20:55, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> At the moment, the temporary mapping is only used when the virtual
> runtime region of Xen is clashing with the physical region.
> 
> In follow-up patches, we will rework how secondary CPU bring-up works
> and it will be convenient to use the fixmap area for accessing
> the root page-table (it is per-cpu).
> 
> Rework the code to use temporary mapping when the Xen physical address
> is not overlapping with the temporary mapping.
> 
> This also has the advantage to simplify the logic to identity map
> Xen.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ---
> 
> Even if this patch is rewriting part of the previous patch, I decided
> to keep them separated to help the review.
> 
> The "follow-up patches" are still in draft at the moment. I still haven't
> find a way to split them nicely and not require too much more work
> in the coloring side.
> 
> I have provided some medium-term goal in the cover letter.
> 
>     Changes in v5:
>         - Fix typo in a comment
>         - No need to link boot_{second, third}_id again if we need to
>           create a temporary area.
> 
>     Changes in v3:
>         - Resolve conflicts after switching from "ldr rX, <label>" to
>           "mov_w rX, <label>" in a previous patch
> 
>     Changes in v2:
>         - Patch added
> ---
>  xen/arch/arm/arm32/head.S | 85 +++++++--------------------------------
>  1 file changed, 15 insertions(+), 70 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index df51550baa8a..93b0af114b0c 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
...
> @@ -675,33 +641,12 @@ remove_identity_mapping:
>          /* r2:r3 := invalid page-table entry */
>          mov   r2, #0x0
>          mov   r3, #0x0
> -        /*
> -         * Find the first slot used. Remove the entry for the first
> -         * table if the slot is not XEN_FIRST_SLOT.
> -         */
Could you please add an empty line here to improve readability?

> +        /* Find the first slot used and remove it */

Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal