[10/17] xen/arm64: head: Improve coding style and document create_pages_tables()
diff mbox series

Message ID 20190610193215.23704-11-julien.grall@arm.com
State New, archived
Headers show
Series
  • xen/arm64: Rework head.S to make it more compliant with the Arm Arm
Related show

Commit Message

Julien Grall June 10, 2019, 7:32 p.m. UTC
Adjust the coding style used in the comments within create_pages_tables()

Lastly, document the behavior and the main registers usage within the
function. Note that x25 is now only used within the function, so it does
not need to be part of the common register.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm64/head.S | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

Comments

Stefano Stabellini June 26, 2019, 1:03 a.m. UTC | #1
On Mon, 10 Jun 2019, Julien Grall wrote:
> Adjust the coding style used in the comments within create_pages_tables()
> 
> Lastly, document the behavior and the main registers usage within the
> function. Note that x25 is now only used within the function, so it does
> not need to be part of the common register.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index ee0024173e..7b92c1c8eb 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -70,7 +70,7 @@
>   *  x22 - is_secondary_cpu
>   *  x23 - UART address
>   *  x24 -
> - *  x25 - identity map in place
> + *  x25 -
>   *  x26 - skip_zero_bss (boot cpu only)
>   *  x27 -
>   *  x28 -
> @@ -443,16 +443,27 @@ cpu_init:
>          ret
>  ENDPROC(cpu_init)
>  
> +/*
> + * Rebuild the boot pagetable's first-level entries. The structure
> + * is described in mm.c.
> + *
> + * After the CPU enables paging it will add the fixmap mapping
> + * to these page tables, however this may clash with the 1:1
> + * mapping. So each CPU must rebuild the page tables here with
> + * the 1:1 in place.
> + *
> + * Inputs:
> + *   x19: paddr(start)
> + *   x20: phys offset

Is x20 actually used?

The rest looks fine.


> + * Clobbers x0 - x4, x25
> + *
> + * Register usage within this function:
> + *   x25: Identity map in place
> + */
>  create_page_tables:
> -        /* Rebuild the boot pagetable's first-level entries. The structure
> -         * is described in mm.c.
> -         *
> -         * After the CPU enables paging it will add the fixmap mapping
> -         * to these page tables, however this may clash with the 1:1
> -         * mapping. So each CPU must rebuild the page tables here with
> -         * the 1:1 in place. */
> -
> -        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
> +        /*
> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
>           * need an additional 1:1 mapping, the virtual mapping will
>           * suffice.
>           */
> @@ -476,7 +487,8 @@ create_page_tables:
>          cbz   x1, 1f                 /* It's in slot 0, map in boot_first
>                                        * or boot_second later on */
>  
> -        /* Level zero does not support superpage mappings, so we have
> +        /*
> +         * Level zero does not support superpage mappings, so we have
>           * to use an extra first level page in which we create a 1GB mapping.
>           */
>          load_paddr x2, boot_first_id
> -- 
> 2.11.0
>
Julien Grall June 26, 2019, 11:20 a.m. UTC | #2
Hi Stefano,

On 26/06/2019 02:03, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> Adjust the coding style used in the comments within create_pages_tables()
>>
>> Lastly, document the behavior and the main registers usage within the
>> function. Note that x25 is now only used within the function, so it does
>> not need to be part of the common register.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 34 +++++++++++++++++++++++-----------
>>   1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index ee0024173e..7b92c1c8eb 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -70,7 +70,7 @@
>>    *  x22 - is_secondary_cpu
>>    *  x23 - UART address
>>    *  x24 -
>> - *  x25 - identity map in place
>> + *  x25 -
>>    *  x26 - skip_zero_bss (boot cpu only)
>>    *  x27 -
>>    *  x28 -
>> @@ -443,16 +443,27 @@ cpu_init:
>>           ret
>>   ENDPROC(cpu_init)
>>   
>> +/*
>> + * Rebuild the boot pagetable's first-level entries. The structure
>> + * is described in mm.c.
>> + *
>> + * After the CPU enables paging it will add the fixmap mapping
>> + * to these page tables, however this may clash with the 1:1
>> + * mapping. So each CPU must rebuild the page tables here with
>> + * the 1:1 in place.
>> + *
>> + * Inputs:
>> + *   x19: paddr(start)
>> + *   x20: phys offset
> 
> Is x20 actually used?

Yes via the macro load_paddr.

> 
> The rest looks fine.
> 
> 
>> + * Clobbers x0 - x4, x25
>> + *
>> + * Register usage within this function:
>> + *   x25: Identity map in place
>> + */
>>   create_page_tables:
>> -        /* Rebuild the boot pagetable's first-level entries. The structure
>> -         * is described in mm.c.
>> -         *
>> -         * After the CPU enables paging it will add the fixmap mapping
>> -         * to these page tables, however this may clash with the 1:1
>> -         * mapping. So each CPU must rebuild the page tables here with
>> -         * the 1:1 in place. */
>> -
>> -        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
>> +        /*
>> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
>>            * need an additional 1:1 mapping, the virtual mapping will
>>            * suffice.
>>            */
>> @@ -476,7 +487,8 @@ create_page_tables:
>>           cbz   x1, 1f                 /* It's in slot 0, map in boot_first
>>                                         * or boot_second later on */
>>   
>> -        /* Level zero does not support superpage mappings, so we have
>> +        /*
>> +         * Level zero does not support superpage mappings, so we have
>>            * to use an extra first level page in which we create a 1GB mapping.
>>            */
>>           load_paddr x2, boot_first_id
>> -- 
>> 2.11.0
>>

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ee0024173e..7b92c1c8eb 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -70,7 +70,7 @@ 
  *  x22 - is_secondary_cpu
  *  x23 - UART address
  *  x24 -
- *  x25 - identity map in place
+ *  x25 -
  *  x26 - skip_zero_bss (boot cpu only)
  *  x27 -
  *  x28 -
@@ -443,16 +443,27 @@  cpu_init:
         ret
 ENDPROC(cpu_init)
 
+/*
+ * Rebuild the boot pagetable's first-level entries. The structure
+ * is described in mm.c.
+ *
+ * After the CPU enables paging it will add the fixmap mapping
+ * to these page tables, however this may clash with the 1:1
+ * mapping. So each CPU must rebuild the page tables here with
+ * the 1:1 in place.
+ *
+ * Inputs:
+ *   x19: paddr(start)
+ *   x20: phys offset
+ *
+ * Clobbers x0 - x4, x25
+ *
+ * Register usage within this function:
+ *   x25: Identity map in place
+ */
 create_page_tables:
-        /* Rebuild the boot pagetable's first-level entries. The structure
-         * is described in mm.c.
-         *
-         * After the CPU enables paging it will add the fixmap mapping
-         * to these page tables, however this may clash with the 1:1
-         * mapping. So each CPU must rebuild the page tables here with
-         * the 1:1 in place. */
-
-        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
+        /*
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
          * need an additional 1:1 mapping, the virtual mapping will
          * suffice.
          */
@@ -476,7 +487,8 @@  create_page_tables:
         cbz   x1, 1f                 /* It's in slot 0, map in boot_first
                                       * or boot_second later on */
 
-        /* Level zero does not support superpage mappings, so we have
+        /*
+         * Level zero does not support superpage mappings, so we have
          * to use an extra first level page in which we create a 1GB mapping.
          */
         load_paddr x2, boot_first_id