[15/17] xen/arm64: head: Rework and document setup_fixmap()
diff mbox series

Message ID 20190610193215.23704-16-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
At the moment, the fixmap table is only hooked when earlyprintk is used.
This is fine today because in C land, the fixmap is not used by anyone
until the the boot CPU is switching to the runtime page-tables.

In the future, the boot CPU will not switch between page-tables to avoid
TLB conflict. This means the fixmap table will need to be hooked before
any use. For simplicity, setup_fixmap() will now do that job.

Lastly, document the behavior and the main registers usage within the
function.

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

Comments

Stefano Stabellini June 26, 2019, 7:01 p.m. UTC | #1
On Mon, 10 Jun 2019, Julien Grall wrote:
> At the moment, the fixmap table is only hooked when earlyprintk is used.
> This is fine today because in C land, the fixmap is not used by anyone
> until the the boot CPU is switching to the runtime page-tables.
> 
> In the future, the boot CPU will not switch between page-tables to avoid
> TLB conflict. This means the fixmap table will need to be hooked before
> any use. For simplicity, setup_fixmap() will now do that job.

Can I ask you to reword this slightly, especially the last sentence? It
took me a while to understand what you meant. I suggest:

 In the future, the boot CPU will not switch between page-tables to
 avoid any TLB conflicts. Thus, the fixmap table will need to be always
 hooked before any use. Let's start doing it now in setup_fixmap().

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 96e85f8834..4f7fa6769f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -700,8 +700,17 @@ id_map_removed:
>          ret
>  ENDPROC(remove_id_map)
>  
> +/*
> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
> + * fixmap table in the page tables.
> + *
> + * The fixmap cannot be mapped in create_page_tables because it may
> + * clash with the ID map.
> + *
> + * Clobbers x0 - x1
> + */
>  setup_fixmap:
> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> +#ifdef CONFIG_EARLY_PRINTK

I am curious why you made this code style change


>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
>          lsr   x2, x23, #THIRD_SHIFT
> @@ -709,6 +718,7 @@ setup_fixmap:
>          mov   x3, #PT_DEV_L3
>          orr   x2, x2, x3             /* x2 := 4K dev map including UART */
>          str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> +#endif
>  
>          /* Map fixmap into boot_second */
>          ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> @@ -721,7 +731,6 @@ setup_fixmap:
>  
>          /* Ensure any page table updates made above have occurred */
>          dsb   nshst
> -#endif
>          ret
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
>
Stefano Stabellini June 26, 2019, 7:02 p.m. UTC | #2
On Mon, 10 Jun 2019, Julien Grall wrote:
> At the moment, the fixmap table is only hooked when earlyprintk is used.
> This is fine today because in C land, the fixmap is not used by anyone
> until the the boot CPU is switching to the runtime page-tables.
> 
> In the future, the boot CPU will not switch between page-tables to avoid
> TLB conflict. This means the fixmap table will need to be hooked before
> any use. For simplicity, setup_fixmap() will now do that job.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 96e85f8834..4f7fa6769f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -700,8 +700,17 @@ id_map_removed:
>          ret
>  ENDPROC(remove_id_map)
>  
> +/*
> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
> + * fixmap table in the page tables.
> + *
> + * The fixmap cannot be mapped in create_page_tables because it may
> + * clash with the ID map.
> + *
> + * Clobbers x0 - x1

I missed this in the last email: it should be x0 - x4?


> + */
>  setup_fixmap:
> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> +#ifdef CONFIG_EARLY_PRINTK
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
>          lsr   x2, x23, #THIRD_SHIFT
> @@ -709,6 +718,7 @@ setup_fixmap:
>          mov   x3, #PT_DEV_L3
>          orr   x2, x2, x3             /* x2 := 4K dev map including UART */
>          str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> +#endif
>  
>          /* Map fixmap into boot_second */
>          ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> @@ -721,7 +731,6 @@ setup_fixmap:
>  
>          /* Ensure any page table updates made above have occurred */
>          dsb   nshst
> -#endif
>          ret
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
>
Julien Grall June 26, 2019, 7:30 p.m. UTC | #3
Hi Stefano,

On 6/26/19 8:01 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> At the moment, the fixmap table is only hooked when earlyprintk is used.
>> This is fine today because in C land, the fixmap is not used by anyone
>> until the the boot CPU is switching to the runtime page-tables.
>>
>> In the future, the boot CPU will not switch between page-tables to avoid
>> TLB conflict. This means the fixmap table will need to be hooked before
>> any use. For simplicity, setup_fixmap() will now do that job.
> 
> Can I ask you to reword this slightly, especially the last sentence? It
> took me a while to understand what you meant. I suggest:
> 
>   In the future, the boot CPU will not switch between page-tables to
>   avoid any TLB conflicts. Thus, the fixmap table will need to be always
>   hooked before any use. Let's start doing it now in setup_fixmap().
> 

I will update the commit message.

> Acked-by: Stefano Stabellini <sstabellini@kernel.org> >
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 96e85f8834..4f7fa6769f 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -700,8 +700,17 @@ id_map_removed:
>>           ret
>>   ENDPROC(remove_id_map)
>>   
>> +/*
>> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
>> + * fixmap table in the page tables.
>> + *
>> + * The fixmap cannot be mapped in create_page_tables because it may
>> + * clash with the ID map.
>> + *
>> + * Clobbers x0 - x1
>> + */
>>   setup_fixmap:
>> -#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>> +#ifdef CONFIG_EARLY_PRINTK
> 
> I am curious why you made this code style change

This is the only #if defined within head.S (the rest use #ifdef) so for 
consistency. Also, it is simpler to read :).

Cheers,
Julien Grall June 27, 2019, 9:19 a.m. UTC | #4
Hi Stefano,

On 26/06/2019 20:02, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> At the moment, the fixmap table is only hooked when earlyprintk is used.
>> This is fine today because in C land, the fixmap is not used by anyone
>> until the the boot CPU is switching to the runtime page-tables.
>>
>> In the future, the boot CPU will not switch between page-tables to avoid
>> TLB conflict. This means the fixmap table will need to be hooked before
>> any use. For simplicity, setup_fixmap() will now do that job.
>>
>> Lastly, document the behavior and the main registers usage within the
>> function.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 96e85f8834..4f7fa6769f 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -700,8 +700,17 @@ id_map_removed:
>>           ret
>>   ENDPROC(remove_id_map)
>>   
>> +/*
>> + * Map the UART in the fixmap (when earlyprintk is used) and hook the
>> + * fixmap table in the page tables.
>> + *
>> + * The fixmap cannot be mapped in create_page_tables because it may
>> + * clash with the ID map.
>> + *
>> + * Clobbers x0 - x1
> 
> I missed this in the last email: it should be x0 - x4?

x0 is not used in the setup_fixmap. So it should be x1 - x4.

Cheers,
Julien Grall June 27, 2019, 9:29 a.m. UTC | #5
Hi Stefano,

On 26/06/2019 20:30, Julien Grall wrote:
> On 6/26/19 8:01 PM, Stefano Stabellini wrote:
>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>> At the moment, the fixmap table is only hooked when earlyprintk is used.
>>> This is fine today because in C land, the fixmap is not used by anyone
>>> until the the boot CPU is switching to the runtime page-tables.
>>>
>>> In the future, the boot CPU will not switch between page-tables to avoid
>>> TLB conflict. This means the fixmap table will need to be hooked before
>>> any use. For simplicity, setup_fixmap() will now do that job.
>>
>> Can I ask you to reword this slightly, especially the last sentence? It
>> took me a while to understand what you meant. I suggest:
>>
>>   In the future, the boot CPU will not switch between page-tables to
>>   avoid any TLB conflicts. Thus, the fixmap table will need to be always
>>   hooked before any use. Let's start doing it now in setup_fixmap().
>>
> 
> I will update the commit message.

I realized the commit message I wrote is inaccurate and reflected to your rewording.

Not all the platforms will generate a TLB conflict abort. Some of them may just 
decide to use an amalgamation of two entries (see "TLB matching" page D5-2500 in 
ARM DDI 0487D.b).

I will replace "any TLB conflicts" by "TLB incoherency".

> 
>> Acked-by: Stefano Stabellini <sstabellini@kernel.org> >

Let me know if you are happy with the change suggested.

Cheers,
Stefano Stabellini June 27, 2019, 3:38 p.m. UTC | #6
On Thu, 27 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 26/06/2019 20:30, Julien Grall wrote:
> > On 6/26/19 8:01 PM, Stefano Stabellini wrote:
> > > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > > At the moment, the fixmap table is only hooked when earlyprintk is used.
> > > > This is fine today because in C land, the fixmap is not used by anyone
> > > > until the the boot CPU is switching to the runtime page-tables.
> > > > 
> > > > In the future, the boot CPU will not switch between page-tables to avoid
> > > > TLB conflict. This means the fixmap table will need to be hooked before
> > > > any use. For simplicity, setup_fixmap() will now do that job.
> > > 
> > > Can I ask you to reword this slightly, especially the last sentence? It
> > > took me a while to understand what you meant. I suggest:
> > > 
> > >   In the future, the boot CPU will not switch between page-tables to
> > >   avoid any TLB conflicts. Thus, the fixmap table will need to be always
> > >   hooked before any use. Let's start doing it now in setup_fixmap().
> > > 
> > 
> > I will update the commit message.
> 
> I realized the commit message I wrote is inaccurate and reflected to your
> rewording.
> 
> Not all the platforms will generate a TLB conflict abort. Some of them may
> just decide to use an amalgamation of two entries (see "TLB matching" page
> D5-2500 in ARM DDI 0487D.b).
> 
> I will replace "any TLB conflicts" by "TLB incoherency".
> 
> > 
> > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> >
> 
> Let me know if you are happy with the change suggested.

Yes, that's fine

Patch
diff mbox series

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 96e85f8834..4f7fa6769f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -700,8 +700,17 @@  id_map_removed:
         ret
 ENDPROC(remove_id_map)
 
+/*
+ * Map the UART in the fixmap (when earlyprintk is used) and hook the
+ * fixmap table in the page tables.
+ *
+ * The fixmap cannot be mapped in create_page_tables because it may
+ * clash with the ID map.
+ *
+ * Clobbers x0 - x1
+ */
 setup_fixmap:
-#if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
+#ifdef CONFIG_EARLY_PRINTK
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
         lsr   x2, x23, #THIRD_SHIFT
@@ -709,6 +718,7 @@  setup_fixmap:
         mov   x3, #PT_DEV_L3
         orr   x2, x2, x3             /* x2 := 4K dev map including UART */
         str   x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
+#endif
 
         /* Map fixmap into boot_second */
         ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
@@ -721,7 +731,6 @@  setup_fixmap:
 
         /* Ensure any page table updates made above have occurred */
         dsb   nshst
-#endif
         ret
 ENDPROC(setup_fixmap)