[Xen-devel] [PATCH 00/17] xen/arm64: Rework head.S to make it more compliant with the Arm Arm

[Xen-devel] [PATCH 00/17] xen/arm64: Rework head.S to make it more compliant with the Arm Arm

[Julien Grall]

Hi all,

This is part of the boot/memory rework for Xen on Arm, but not sent as
MM-PARTx as this is focusing on the boot code.

Similar to the memory code, the boot code is not following the Arm Arm and
could lead to memory corruption/TLB conflict abort. I am not aware
of any platforms where Xen fails to boot, yet it should be fixed sooner
rather than later.

While making the code more compliant, I have also took the opportunity
to simplify the boot and also add more documentation.

After this series, the boot CPU and secondary CPUs path is mostly compliant
with the Arm Arm. The only non-compliant places I am aware of are:
    1) create_page_tables: Some rework is necessary to update the page-tables
    safely without the MMU on.
    2) The switches between boot and runtime page-tables (for both boot CPU
       and secondary CPUs) are not safe.

Both will be addressed in follow-up work.

Lastly, only Arm64 has been modified so far. Arm32 requires the same
modifications. It will be sent once I gathered feedback on the approach.

Note that the series have a minor clash with MM-PART3 and reference some
change done in MM-PART1. Yet the code is mostly self-contained to
xen/arch/arm64/head.S.

For convenience I provided a branch based on staging:
    git://xenbits.xen.org/people/julieng/xen-unstable.git branch boot/arm64/v1

Cheers,

Julien Grall (17):
  xen/arm64: head Mark the end of subroutines with ENDPROC
  xen/arm64: head: Don't clobber x30/lr in the macro PRINT
  xen/arm64: head: Rework UART initialization on boot CPU
  xen/arm64: head: Don't "reserve" x24 for the CPUID
  xen/arm64: head: Introduce print_reg
  xen/arm64: head: Introduce distinct paths for the boot CPU and
    secondary CPUs
  xen/arm64: head: Rework and document check_cpu_mode()
  xen/arm64: head: Rework and document zero_bss()
  xen/arm64: head: Improve coding style and document cpu_init()
  xen/arm64: head: Improve coding style and document
    create_pages_tables()
  xen/arm64: head: Document enable_mmu()
  xen/arm64: head: Move assembly switch to the runtime PT in secondary
    CPUs path
  xen/arm64: head: Don't setup the fixmap on secondary CPUs
  xen/arm64: head: Remove ID map as soon as it is not used
  xen/arm64: head: Rework and document setup_fixmap()
  xen/arm64: head: Rework and document launch()
  xen/arm64: Zero BSS after the MMU and D-cache is turned on

 xen/arch/arm/arm64/head.S | 396 +++++++++++++++++++++++++++++++++-------------
 xen/arch/arm/mm.c         |  23 ++-
 2 files changed, 306 insertions(+), 113 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH 01/17] xen/arm64: head Mark the end of subroutines with ENDPROC

[Julien Grall]

putn() and puts() are two subroutines. Add ENDPROC for the benefits of
static analysis tools and the reader.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ddd3a33108..c8bbdf05a6 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -646,6 +646,7 @@ puts:
         b     puts
 1:
         ret
+ENDPROC(puts)
 
 /* Print a 32-bit number in hex.  Specific to the PL011 UART.
  * x0: Number to print.
@@ -664,6 +665,7 @@ putn:
         subs  x3, x3, #1
         b.ne  1b
         ret
+ENDPROC(putn)
 
 hex:    .ascii "0123456789abcdef"
         .align 2
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 01/17] xen/arm64: head Mark the end of subroutines with ENDPROC

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> putn() and puts() are two subroutines. Add ENDPROC for the benefits of
> static analysis tools and the reader.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/arm64/head.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index ddd3a33108..c8bbdf05a6 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -646,6 +646,7 @@ puts:
>          b     puts
>  1:
>          ret
> +ENDPROC(puts)
>  
>  /* Print a 32-bit number in hex.  Specific to the PL011 UART.
>   * x0: Number to print.
> @@ -664,6 +665,7 @@ putn:
>          subs  x3, x3, #1
>          b.ne  1b
>          ret
> +ENDPROC(putn)
>  
>  hex:    .ascii "0123456789abcdef"
>          .align 2
> -- 
> 2.11.0
> 

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

[Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall]

The current implementation of the macro PRINT will clobber x30/lr. This
means the user should save lr if it cares about it.

Follow-up patches will introduce more use of PRINT in place where lr
should be preserved. Rather than requiring all the users to preserve lr,
the macro PRINT is modified to save and restore it.

While the comment state x3 will be clobbered, this is not the case. So
PRINT will use x3 to preserve lr.

Lastly, take the opportunity to move the comment on top of PRINT and use
PRINT in init_uart. Both changes will be helpful in a follow-up patch.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index c8bbdf05a6..a5147c8d80 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -78,12 +78,17 @@
  *  x30 - lr
  */
 
-/* Macro to print a string to the UART, if there is one.
- * Clobbers x0-x3. */
 #ifdef CONFIG_EARLY_PRINTK
+/*
+ * Macro to print a string to the UART, if there is one.
+ *
+ * Clobbers x0 - x3
+ */
 #define PRINT(_s)           \
+        mov   x3, lr  ;     \
         adr   x0, 98f ;     \
         bl    puts    ;     \
+        mov   lr, x3  ;     \
         RODATA_STR(98, _s)
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
@@ -630,9 +635,8 @@ init_uart:
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif
-        adr   x0, 1f
-        b     puts
-RODATA_STR(1, "- UART enabled -\r\n")
+        PRINT("- UART enabled -\r\n")
+        ret
 
 /* Print early debug messages.
  * x0: Nul-terminated string to print.
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
>>  The current implementation of the macro PRINT will clobber x30/lr. This
> means the user should save lr if it cares about it.

By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
expression.

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


> Follow-up patches will introduce more use of PRINT in place where lr
> should be preserved. Rather than requiring all the users to preserve lr,
> the macro PRINT is modified to save and restore it.
> 
> While the comment state x3 will be clobbered, this is not the case. So
> PRINT will use x3 to preserve lr.
> 
> Lastly, take the opportunity to move the comment on top of PRINT and use
> PRINT in init_uart. Both changes will be helpful in a follow-up patch.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index c8bbdf05a6..a5147c8d80 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -78,12 +78,17 @@
>   *  x30 - lr
>   */
>  
> -/* Macro to print a string to the UART, if there is one.
> - * Clobbers x0-x3. */
>  #ifdef CONFIG_EARLY_PRINTK
> +/*
> + * Macro to print a string to the UART, if there is one.
> + *
> + * Clobbers x0 - x3
> + */
>  #define PRINT(_s)           \
> +        mov   x3, lr  ;     \
>          adr   x0, 98f ;     \
>          bl    puts    ;     \
> +        mov   lr, x3  ;     \
>          RODATA_STR(98, _s)
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> @@ -630,9 +635,8 @@ init_uart:
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init x23, 0
>  #endif
> -        adr   x0, 1f
> -        b     puts
> -RODATA_STR(1, "- UART enabled -\r\n")
> +        PRINT("- UART enabled -\r\n")
> +        ret
>  
>  /* Print early debug messages.
>   * x0: Nul-terminated string to print.
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Stefano Stabellini]

On Tue, 25 Jun 2019, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
> >>  The current implementation of the macro PRINT will clobber x30/lr. This
> > means the user should save lr if it cares about it.
> 
> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
> expression.

No of course not! You meant x30 which is a synonym of lr! It is just
that in this case it is also supposed to clobber x0-x3 -- I got
confused! The commit message is also fine as is then. More below.


> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
> > Follow-up patches will introduce more use of PRINT in place where lr
> > should be preserved. Rather than requiring all the users to preserve lr,
> > the macro PRINT is modified to save and restore it.
> > 
> > While the comment state x3 will be clobbered, this is not the case. So
> > PRINT will use x3 to preserve lr.
> > 
> > Lastly, take the opportunity to move the comment on top of PRINT and use
> > PRINT in init_uart. Both changes will be helpful in a follow-up patch.
> > 
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > ---
> >  xen/arch/arm/arm64/head.S | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > index c8bbdf05a6..a5147c8d80 100644
> > --- a/xen/arch/arm/arm64/head.S
> > +++ b/xen/arch/arm/arm64/head.S
> > @@ -78,12 +78,17 @@
> >   *  x30 - lr
> >   */
> >  
> > -/* Macro to print a string to the UART, if there is one.
> > - * Clobbers x0-x3. */
> >  #ifdef CONFIG_EARLY_PRINTK
> > +/*
> > + * Macro to print a string to the UART, if there is one.
> > + *
> > + * Clobbers x0 - x3
> > + */
> >  #define PRINT(_s)           \
> > +        mov   x3, lr  ;     \
> >          adr   x0, 98f ;     \
> >          bl    puts    ;     \
> > +        mov   lr, x3  ;     \
> >          RODATA_STR(98, _s)

Strangely enough I get a build error with gcc 7.3.1, but if I use x30
instead of lr, it builds fine. Have you seen this before?
The error is:

arm64/head.S: Assembler messages:
arm64/head.S:272: Error: operand 1 must be an integer register -- `mov lr,x3'
[...]
arm64/head.S:272: Error: undefined symbol lr used as an immediate value
[...]

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall]

Hi Stefano,

On 26/06/2019 00:59, Stefano Stabellini wrote:
> On Tue, 25 Jun 2019, Stefano Stabellini wrote:
>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>>   The current implementation of the macro PRINT will clobber x30/lr. This
>>> means the user should save lr if it cares about it.
>>
>> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
>> expression.
> 
> No of course not! You meant x30 which is a synonym of lr! It is just
> that in this case it is also supposed to clobber x0-x3 -- I got
> confused! The commit message is also fine as is then. More below.
> 
> 
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>>
>>> Follow-up patches will introduce more use of PRINT in place where lr
>>> should be preserved. Rather than requiring all the users to preserve lr,
>>> the macro PRINT is modified to save and restore it.
>>>
>>> While the comment state x3 will be clobbered, this is not the case. So
>>> PRINT will use x3 to preserve lr.
>>>
>>> Lastly, take the opportunity to move the comment on top of PRINT and use
>>> PRINT in init_uart. Both changes will be helpful in a follow-up patch.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/arch/arm/arm64/head.S | 14 +++++++++-----
>>>   1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index c8bbdf05a6..a5147c8d80 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -78,12 +78,17 @@
>>>    *  x30 - lr
>>>    */
>>>   
>>> -/* Macro to print a string to the UART, if there is one.
>>> - * Clobbers x0-x3. */
>>>   #ifdef CONFIG_EARLY_PRINTK
>>> +/*
>>> + * Macro to print a string to the UART, if there is one.
>>> + *
>>> + * Clobbers x0 - x3
>>> + */
>>>   #define PRINT(_s)           \
>>> +        mov   x3, lr  ;     \
>>>           adr   x0, 98f ;     \
>>>           bl    puts    ;     \
>>> +        mov   lr, x3  ;     \
>>>           RODATA_STR(98, _s)
> 
> Strangely enough I get a build error with gcc 7.3.1, but if I use x30
> instead of lr, it builds fine. Have you seen this before?

Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not all 
the assembler is able to understand "lr".

In the file entry.S we have the following line:

lr      .req    x30             // link register


Could you give a try to add the line in head.S?

> The error is:
> 
> arm64/head.S: Assembler messages:
> arm64/head.S:272: Error: operand 1 must be an integer register -- `mov lr,x3'
> [...]
> arm64/head.S:272: Error: undefined symbol lr used as an immediate value
> [...]
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Stefano Stabellini]

On Wed, 26 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 26/06/2019 00:59, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2019, Stefano Stabellini wrote:
> > > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > > >   The current implementation of the macro PRINT will clobber x30/lr.
> > > > > This
> > > > means the user should save lr if it cares about it.
> > > 
> > > By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
> > > expression.
> > 
> > No of course not! You meant x30 which is a synonym of lr! It is just
> > that in this case it is also supposed to clobber x0-x3 -- I got
> > confused! The commit message is also fine as is then. More below.
> > 
> > 
> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > 
> > > 
> > > > Follow-up patches will introduce more use of PRINT in place where lr
> > > > should be preserved. Rather than requiring all the users to preserve lr,
> > > > the macro PRINT is modified to save and restore it.
> > > > 
> > > > While the comment state x3 will be clobbered, this is not the case. So
> > > > PRINT will use x3 to preserve lr.
> > > > 
> > > > Lastly, take the opportunity to move the comment on top of PRINT and use
> > > > PRINT in init_uart. Both changes will be helpful in a follow-up patch.
> > > > 
> > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > ---
> > > >   xen/arch/arm/arm64/head.S | 14 +++++++++-----
> > > >   1 file changed, 9 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > index c8bbdf05a6..a5147c8d80 100644
> > > > --- a/xen/arch/arm/arm64/head.S
> > > > +++ b/xen/arch/arm/arm64/head.S
> > > > @@ -78,12 +78,17 @@
> > > >    *  x30 - lr
> > > >    */
> > > >   -/* Macro to print a string to the UART, if there is one.
> > > > - * Clobbers x0-x3. */
> > > >   #ifdef CONFIG_EARLY_PRINTK
> > > > +/*
> > > > + * Macro to print a string to the UART, if there is one.
> > > > + *
> > > > + * Clobbers x0 - x3
> > > > + */
> > > >   #define PRINT(_s)           \
> > > > +        mov   x3, lr  ;     \
> > > >           adr   x0, 98f ;     \
> > > >           bl    puts    ;     \
> > > > +        mov   lr, x3  ;     \
> > > >           RODATA_STR(98, _s)
> > 
> > Strangely enough I get a build error with gcc 7.3.1, but if I use x30
> > instead of lr, it builds fine. Have you seen this before?
> 
> Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not
> all the assembler is able to understand "lr".
> 
> In the file entry.S we have the following line:
> 
> lr      .req    x30             // link register
> 
> 
> Could you give a try to add the line in head.S?

That works

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall]

Hi Stefano,

On 26/06/2019 16:27, Stefano Stabellini wrote:
> On Wed, 26 Jun 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 26/06/2019 00:59, Stefano Stabellini wrote:
>>> On Tue, 25 Jun 2019, Stefano Stabellini wrote:
>>>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>>>>    The current implementation of the macro PRINT will clobber x30/lr.
>>>>>> This
>>>>> means the user should save lr if it cares about it.
>>>>
>>>> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
>>>> expression.
>>>
>>> No of course not! You meant x30 which is a synonym of lr! It is just
>>> that in this case it is also supposed to clobber x0-x3 -- I got
>>> confused! The commit message is also fine as is then. More below.
>>>
>>>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>
>>>>
>>>>> Follow-up patches will introduce more use of PRINT in place where lr
>>>>> should be preserved. Rather than requiring all the users to preserve lr,
>>>>> the macro PRINT is modified to save and restore it.
>>>>>
>>>>> While the comment state x3 will be clobbered, this is not the case. So
>>>>> PRINT will use x3 to preserve lr.
>>>>>
>>>>> Lastly, take the opportunity to move the comment on top of PRINT and use
>>>>> PRINT in init_uart. Both changes will be helpful in a follow-up patch.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>> ---
>>>>>    xen/arch/arm/arm64/head.S | 14 +++++++++-----
>>>>>    1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>>>> index c8bbdf05a6..a5147c8d80 100644
>>>>> --- a/xen/arch/arm/arm64/head.S
>>>>> +++ b/xen/arch/arm/arm64/head.S
>>>>> @@ -78,12 +78,17 @@
>>>>>     *  x30 - lr
>>>>>     */
>>>>>    -/* Macro to print a string to the UART, if there is one.
>>>>> - * Clobbers x0-x3. */
>>>>>    #ifdef CONFIG_EARLY_PRINTK
>>>>> +/*
>>>>> + * Macro to print a string to the UART, if there is one.
>>>>> + *
>>>>> + * Clobbers x0 - x3
>>>>> + */
>>>>>    #define PRINT(_s)           \
>>>>> +        mov   x3, lr  ;     \
>>>>>            adr   x0, 98f ;     \
>>>>>            bl    puts    ;     \
>>>>> +        mov   lr, x3  ;     \
>>>>>            RODATA_STR(98, _s)
>>>
>>> Strangely enough I get a build error with gcc 7.3.1, but if I use x30
>>> instead of lr, it builds fine. Have you seen this before?
>>
>> Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is not
>> all the assembler is able to understand "lr".
>>
>> In the file entry.S we have the following line:
>>
>> lr      .req    x30             // link register
>>
>>
>> Could you give a try to add the line in head.S?
> 
> That works

Thank you.

I thought a bit more during the day and decided to use "x30" directly rather 
than lr. We can decide to revisit it if we think the code is too difficult to read.

Cheers,
-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Stefano Stabellini]

On Wed, 26 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 26/06/2019 16:27, Stefano Stabellini wrote:
> > On Wed, 26 Jun 2019, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 26/06/2019 00:59, Stefano Stabellini wrote:
> > > > On Tue, 25 Jun 2019, Stefano Stabellini wrote:
> > > > > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > > > > >    The current implementation of the macro PRINT will clobber
> > > > > > > x30/lr.
> > > > > > > This
> > > > > > means the user should save lr if it cares about it.
> > > > > 
> > > > > By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
> > > > > expression.
> > > > 
> > > > No of course not! You meant x30 which is a synonym of lr! It is just
> > > > that in this case it is also supposed to clobber x0-x3 -- I got
> > > > confused! The commit message is also fine as is then. More below.
> > > > 
> > > > 
> > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > > > 
> > > > > 
> > > > > > Follow-up patches will introduce more use of PRINT in place where lr
> > > > > > should be preserved. Rather than requiring all the users to preserve
> > > > > > lr,
> > > > > > the macro PRINT is modified to save and restore it.
> > > > > > 
> > > > > > While the comment state x3 will be clobbered, this is not the case.
> > > > > > So
> > > > > > PRINT will use x3 to preserve lr.
> > > > > > 
> > > > > > Lastly, take the opportunity to move the comment on top of PRINT and
> > > > > > use
> > > > > > PRINT in init_uart. Both changes will be helpful in a follow-up
> > > > > > patch.
> > > > > > 
> > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > > ---
> > > > > >    xen/arch/arm/arm64/head.S | 14 +++++++++-----
> > > > > >    1 file changed, 9 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > > > > index c8bbdf05a6..a5147c8d80 100644
> > > > > > --- a/xen/arch/arm/arm64/head.S
> > > > > > +++ b/xen/arch/arm/arm64/head.S
> > > > > > @@ -78,12 +78,17 @@
> > > > > >     *  x30 - lr
> > > > > >     */
> > > > > >    -/* Macro to print a string to the UART, if there is one.
> > > > > > - * Clobbers x0-x3. */
> > > > > >    #ifdef CONFIG_EARLY_PRINTK
> > > > > > +/*
> > > > > > + * Macro to print a string to the UART, if there is one.
> > > > > > + *
> > > > > > + * Clobbers x0 - x3
> > > > > > + */
> > > > > >    #define PRINT(_s)           \
> > > > > > +        mov   x3, lr  ;     \
> > > > > >            adr   x0, 98f ;     \
> > > > > >            bl    puts    ;     \
> > > > > > +        mov   lr, x3  ;     \
> > > > > >            RODATA_STR(98, _s)
> > > > 
> > > > Strangely enough I get a build error with gcc 7.3.1, but if I use x30
> > > > instead of lr, it builds fine. Have you seen this before?
> > > 
> > > Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is
> > > not
> > > all the assembler is able to understand "lr".
> > > 
> > > In the file entry.S we have the following line:
> > > 
> > > lr      .req    x30             // link register
> > > 
> > > 
> > > Could you give a try to add the line in head.S?
> > 
> > That works
> 
> Thank you.
> 
> I thought a bit more during the day and decided to use "x30" directly rather
> than lr. We can decide to revisit it if we think the code is too difficult to
> read.

I was going to suggest x30 too yesterday, but if we can make `lr' work
then that would be my preference because it makes it more immediately
obvious what the code is doing.

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

Re: [Xen-devel] [PATCH 02/17] xen/arm64: head: Don't clobber x30/lr in the macro PRINT

[Julien Grall]

Hi Stefano,

On 6/26/19 7:32 PM, Stefano Stabellini wrote:
> On Wed, 26 Jun 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 26/06/2019 16:27, Stefano Stabellini wrote:
>>> On Wed, 26 Jun 2019, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 26/06/2019 00:59, Stefano Stabellini wrote:
>>>>> On Tue, 25 Jun 2019, Stefano Stabellini wrote:
>>>>>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>>>>>>     The current implementation of the macro PRINT will clobber
>>>>>>>> x30/lr.
>>>>>>>> This
>>>>>>> means the user should save lr if it cares about it.
>>>>>>
>>>>>> By x30/lr, do you mean x0-x3 and lr? I would prefer a clearer
>>>>>> expression.
>>>>>
>>>>> No of course not! You meant x30 which is a synonym of lr! It is just
>>>>> that in this case it is also supposed to clobber x0-x3 -- I got
>>>>> confused! The commit message is also fine as is then. More below.
>>>>>
>>>>>
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>
>>>>>>
>>>>>>> Follow-up patches will introduce more use of PRINT in place where lr
>>>>>>> should be preserved. Rather than requiring all the users to preserve
>>>>>>> lr,
>>>>>>> the macro PRINT is modified to save and restore it.
>>>>>>>
>>>>>>> While the comment state x3 will be clobbered, this is not the case.
>>>>>>> So
>>>>>>> PRINT will use x3 to preserve lr.
>>>>>>>
>>>>>>> Lastly, take the opportunity to move the comment on top of PRINT and
>>>>>>> use
>>>>>>> PRINT in init_uart. Both changes will be helpful in a follow-up
>>>>>>> patch.
>>>>>>>
>>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>>> ---
>>>>>>>     xen/arch/arm/arm64/head.S | 14 +++++++++-----
>>>>>>>     1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>>>>>> index c8bbdf05a6..a5147c8d80 100644
>>>>>>> --- a/xen/arch/arm/arm64/head.S
>>>>>>> +++ b/xen/arch/arm/arm64/head.S
>>>>>>> @@ -78,12 +78,17 @@
>>>>>>>      *  x30 - lr
>>>>>>>      */
>>>>>>>     -/* Macro to print a string to the UART, if there is one.
>>>>>>> - * Clobbers x0-x3. */
>>>>>>>     #ifdef CONFIG_EARLY_PRINTK
>>>>>>> +/*
>>>>>>> + * Macro to print a string to the UART, if there is one.
>>>>>>> + *
>>>>>>> + * Clobbers x0 - x3
>>>>>>> + */
>>>>>>>     #define PRINT(_s)           \
>>>>>>> +        mov   x3, lr  ;     \
>>>>>>>             adr   x0, 98f ;     \
>>>>>>>             bl    puts    ;     \
>>>>>>> +        mov   lr, x3  ;     \
>>>>>>>             RODATA_STR(98, _s)
>>>>>
>>>>> Strangely enough I get a build error with gcc 7.3.1, but if I use x30
>>>>> instead of lr, it builds fine. Have you seen this before?
>>>>
>>>> Hmmm, I can't to reproduce it even on older compiler (4.9). My guess is
>>>> not
>>>> all the assembler is able to understand "lr".
>>>>
>>>> In the file entry.S we have the following line:
>>>>
>>>> lr      .req    x30             // link register
>>>>
>>>>
>>>> Could you give a try to add the line in head.S?
>>>
>>> That works
>>
>> Thank you.
>>
>> I thought a bit more during the day and decided to use "x30" directly rather
>> than lr. We can decide to revisit it if we think the code is too difficult to
>> read.
> 
> I was going to suggest x30 too yesterday, but if we can make `lr' work
> then that would be my preference because it makes it more immediately
> obvious what the code is doing.

I will have a look to move the line in an header.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 03/17] xen/arm64: head: Rework UART initialization on boot CPU

[Julien Grall]

Anything executed after the label common_start can be executed on all
CPUs. However most of the instructions executed between the label
common_start and init_uart are not executed on the boot CPU.

The only instructions executed are to lookup the CPUID so it can be
printed on the console (if earlyprintk is enabled). Printing the CPUID
is not entirely useful to have for the boot CPU and requires a
conditional branch to bypass unused instructions.

Furthermore, the function init_uart is only called for boot CPU
requiring another conditional branch. This makes the code a bit tricky
to follow.

The UART initialization is now moved before the label common_start. This
now requires to have a slightly altered print for the boot CPU and set
the early UART base address in each the two path (boot CPU and
secondary CPUs).

This has the nice effect to remove a couple of conditional branch in
the code.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a5147c8d80..fd432ee15d 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -265,6 +265,12 @@ real_start_efi:
         load_paddr x21, _sdtb
 #endif
 
+        /* Initialize the UART if earlyprintk has been enabled. */
+#ifdef CONFIG_EARLY_PRINTK
+        bl    init_uart
+#endif
+        PRINT("- Boot CPU booting -\r\n")
+
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
         b     common_start
@@ -281,14 +287,11 @@ GLOBAL(init_secondary)
         /* Boot CPU already zero BSS so skip it on secondary CPUs. */
         mov   x26, #1                /* X26 := skip_zero_bss */
 
-common_start:
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
         bic   x24, x0, x13           /* Mask out flags to get CPU ID */
 
-        /* Non-boot CPUs wait here until __cpu_up is ready for them */
-        cbz   x22, 1f
-
+        /* Wait here until __cpu_up is ready to handle the CPU */
         load_paddr x0, smp_up_cpu
         dsb   sy
 2:      ldr   x1, [x0]
@@ -300,14 +303,14 @@ common_start:
 
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
-        cbnz  x22, 1f
-        bl    init_uart                 /* Boot CPU sets up the UART too */
-1:      PRINT("- CPU ")
+        PRINT("- CPU ")
         mov   x0, x24
         bl    putn
         PRINT(" booting -\r\n")
 #endif
 
+common_start:
+
         PRINT("- Current EL ")
         mrs   x4, CurrentEL
         mov   x0, x4
@@ -628,10 +631,16 @@ ENTRY(switch_ttbr)
         ret
 
 #ifdef CONFIG_EARLY_PRINTK
-/* Bring up the UART.
- * x23: Early UART base address
- * Clobbers x0-x1 */
+/*
+ * Initialize the UART. Should only be called on the boot CPU.
+ *
+ * Ouput:
+ *  x23: Early UART base physical address
+ *
+ * Clobbers x0 - x1
+ */
 init_uart:
+        ldr   x23, =EARLY_UART_BASE_ADDRESS
 #ifdef EARLY_PRINTK_INIT_UART
         early_uart_init x23, 0
 #endif
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 03/17] xen/arm64: head: Rework UART initialization on boot CPU

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> Anything executed after the label common_start can be executed on all
> CPUs. However most of the instructions executed between the label
> common_start and init_uart are not executed on the boot CPU.
> 
> The only instructions executed are to lookup the CPUID so it can be
> printed on the console (if earlyprintk is enabled). Printing the CPUID
> is not entirely useful to have for the boot CPU and requires a
> conditional branch to bypass unused instructions.
> 
> Furthermore, the function init_uart is only called for boot CPU
> requiring another conditional branch. This makes the code a bit tricky
> to follow.
> 
> The UART initialization is now moved before the label common_start. This
> now requires to have a slightly altered print for the boot CPU and set
> the early UART base address in each the two path (boot CPU and
> secondary CPUs).
> 
> This has the nice effect to remove a couple of conditional branch in
> the code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/arm64/head.S | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index a5147c8d80..fd432ee15d 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -265,6 +265,12 @@ real_start_efi:
>          load_paddr x21, _sdtb
>  #endif
>  
> +        /* Initialize the UART if earlyprintk has been enabled. */
> +#ifdef CONFIG_EARLY_PRINTK
> +        bl    init_uart
> +#endif
> +        PRINT("- Boot CPU booting -\r\n")
> +
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
>          b     common_start
> @@ -281,14 +287,11 @@ GLOBAL(init_secondary)
>          /* Boot CPU already zero BSS so skip it on secondary CPUs. */
>          mov   x26, #1                /* X26 := skip_zero_bss */
>  
> -common_start:
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
>          bic   x24, x0, x13           /* Mask out flags to get CPU ID */
>  
> -        /* Non-boot CPUs wait here until __cpu_up is ready for them */
> -        cbz   x22, 1f
> -
> +        /* Wait here until __cpu_up is ready to handle the CPU */
>          load_paddr x0, smp_up_cpu
>          dsb   sy
>  2:      ldr   x1, [x0]
> @@ -300,14 +303,14 @@ common_start:
>  
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
> -        cbnz  x22, 1f
> -        bl    init_uart                 /* Boot CPU sets up the UART too */
> -1:      PRINT("- CPU ")
> +        PRINT("- CPU ")
>          mov   x0, x24
>          bl    putn
>          PRINT(" booting -\r\n")
>  #endif
>  
> +common_start:
> +
>          PRINT("- Current EL ")
>          mrs   x4, CurrentEL
>          mov   x0, x4
> @@ -628,10 +631,16 @@ ENTRY(switch_ttbr)
>          ret
>  
>  #ifdef CONFIG_EARLY_PRINTK
> -/* Bring up the UART.
> - * x23: Early UART base address
> - * Clobbers x0-x1 */
> +/*
> + * Initialize the UART. Should only be called on the boot CPU.
> + *
> + * Ouput:
> + *  x23: Early UART base physical address
> + *
> + * Clobbers x0 - x1
> + */
>  init_uart:
> +        ldr   x23, =EARLY_UART_BASE_ADDRESS
>  #ifdef EARLY_PRINTK_INIT_UART
>          early_uart_init x23, 0
>  #endif
> -- 
> 2.11.0
> 

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

[Xen-devel] [PATCH 04/17] xen/arm64: head: Don't "reserve" x24 for the CPUID

[Julien Grall]

After the recent rework, the CPUID is only used at the very beginning of
the secondary CPUs boot path. So there is no need to "reserve" x24 for
he CPUID.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index fd432ee15d..84e26582c4 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -69,7 +69,7 @@
  *  x21 - DTB address (boot cpu only)
  *  x22 - is_secondary_cpu
  *  x23 - UART address
- *  x24 - cpuid
+ *  x24 -
  *  x25 - identity map in place
  *  x26 - skip_zero_bss
  *  x27 -
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 04/17] xen/arm64: head: Don't "reserve" x24 for the CPUID

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> After the recent rework, the CPUID is only used at the very beginning of
> the secondary CPUs boot path. So there is no need to "reserve" x24 for
> he CPUID.

If you are going to resend the series it would probably make sense to
fold it in the previous patch, but it is also OK as is

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


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index fd432ee15d..84e26582c4 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -69,7 +69,7 @@
>   *  x21 - DTB address (boot cpu only)
>   *  x22 - is_secondary_cpu
>   *  x23 - UART address
> - *  x24 - cpuid
> + *  x24 -
>   *  x25 - identity map in place
>   *  x26 - skip_zero_bss
>   *  x27 -
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 04/17] xen/arm64: head: Don't "reserve" x24 for the CPUID

[Julien Grall]

Hi Stefano,

On 26/06/2019 01:01, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> After the recent rework, the CPUID is only used at the very beginning of
>> the secondary CPUs boot path. So there is no need to "reserve" x24 for
>> he CPUID.
> 
> If you are going to resend the series it would probably make sense to
> fold it in the previous patch, but it is also OK as is

I am planning to resend the series. So I will fold it.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 05/17] xen/arm64: head: Introduce print_reg

[Julien Grall]

At the moment, the user should save x30/lr if it cares about it.

Follow-up patches will introduce more use of putn in place where lr
should be preserved.

Furthermore, any user of putn should also move the value to register x0
if it was stored in a different register.

For convenience, a new macro is introduced to print a given register.
The macro will take care for us to move the value to x0 and also
preserve lr.

Lastly the new macro is used to replace all the callsite of putn. This
will simplify rework/review later on.

Note that CurrentEL is now stored in x5 instead of x4 because the latter
will be clobbered by the macro print_reg.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 84e26582c4..9142b4a774 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -90,8 +90,25 @@
         bl    puts    ;     \
         mov   lr, x3  ;     \
         RODATA_STR(98, _s)
+
+/*
+ * Macro to print the value of register \xb
+ *
+ * Clobbers x0 - x4
+ */
+.macro print_reg xb
+        mov   x4, lr
+        mov   x0, \xb
+        bl    putn
+        mov   lr, x4
+.endm
+
 #else /* CONFIG_EARLY_PRINTK */
 #define PRINT(s)
+
+.macro print_reg xb
+.endm
+
 #endif /* !CONFIG_EARLY_PRINTK */
 
 /* Load the physical address of a symbol into xb */
@@ -304,22 +321,20 @@ GLOBAL(init_secondary)
 #ifdef CONFIG_EARLY_PRINTK
         ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
         PRINT("- CPU ")
-        mov   x0, x24
-        bl    putn
+        print_reg x24
         PRINT(" booting -\r\n")
 #endif
 
 common_start:
 
         PRINT("- Current EL ")
-        mrs   x4, CurrentEL
-        mov   x0, x4
-        bl    putn
+        mrs   x5, CurrentEL
+        print_reg x5
         PRINT(" -\r\n")
 
         /* Are we in EL2 */
-        cmp   x4, #PSR_MODE_EL2t
-        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
+        cmp   x5, #PSR_MODE_EL2t
+        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
         b.eq  el2 /* Yes */
 
         /* OK, we're boned. */
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 05/17] xen/arm64: head: Introduce print_reg

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> At the moment, the user should save x30/lr if it cares about it.
> 
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
> 
> Furthermore, any user of putn should also move the value to register x0
> if it was stored in a different register.
> 
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to x0 and also
> preserve lr.
> 
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
> 
> Note that CurrentEL is now stored in x5 instead of x4 because the latter
> will be clobbered by the macro print_reg.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 84e26582c4..9142b4a774 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -90,8 +90,25 @@
>          bl    puts    ;     \
>          mov   lr, x3  ;     \
>          RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \xb
> + *
> + * Clobbers x0 - x4
> + */
> +.macro print_reg xb
> +        mov   x4, lr
> +        mov   x0, \xb
> +        bl    putn
> +        mov   lr, x4

I have the same weird issues with my compiler as before, replacing 'lr'
with 'x30' solves the problem.

This patch looks OK though.


> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg xb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>  
>  /* Load the physical address of a symbol into xb */
> @@ -304,22 +321,20 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
> -        mov   x0, x24
> -        bl    putn
> +        print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
>  
>  common_start:
>  
>          PRINT("- Current EL ")
> -        mrs   x4, CurrentEL
> -        mov   x0, x4
> -        bl    putn
> +        mrs   x5, CurrentEL
> +        print_reg x5
>          PRINT(" -\r\n")
>  
>          /* Are we in EL2 */
> -        cmp   x4, #PSR_MODE_EL2t
> -        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
> +        cmp   x5, #PSR_MODE_EL2t
> +        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
>          b.eq  el2 /* Yes */
>  
>          /* OK, we're boned. */
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 05/17] xen/arm64: head: Introduce print_reg

[Julien Grall]

Hi Stefano,

On 26/06/2019 01:09, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> At the moment, the user should save x30/lr if it cares about it.
>>
>> Follow-up patches will introduce more use of putn in place where lr
>> should be preserved.
>>
>> Furthermore, any user of putn should also move the value to register x0
>> if it was stored in a different register.
>>
>> For convenience, a new macro is introduced to print a given register.
>> The macro will take care for us to move the value to x0 and also
>> preserve lr.
>>
>> Lastly the new macro is used to replace all the callsite of putn. This
>> will simplify rework/review later on.
>>
>> Note that CurrentEL is now stored in x5 instead of x4 because the latter
>> will be clobbered by the macro print_reg.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 84e26582c4..9142b4a774 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -90,8 +90,25 @@
>>           bl    puts    ;     \
>>           mov   lr, x3  ;     \
>>           RODATA_STR(98, _s)
>> +
>> +/*
>> + * Macro to print the value of register \xb
>> + *
>> + * Clobbers x0 - x4
>> + */
>> +.macro print_reg xb
>> +        mov   x4, lr
>> +        mov   x0, \xb
>> +        bl    putn
>> +        mov   lr, x4
> 
> I have the same weird issues with my compiler as before, replacing 'lr'
> with 'x30' solves the problem.

Can you have a try with the following line?

lr      .req    x30             // link register

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 05/17] xen/arm64: head: Introduce print_reg

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> At the moment, the user should save x30/lr if it cares about it.
>
> Follow-up patches will introduce more use of putn in place where lr
> should be preserved.
>
> Furthermore, any user of putn should also move the value to register x0
> if it was stored in a different register.
>
> For convenience, a new macro is introduced to print a given register.
> The macro will take care for us to move the value to x0 and also
> preserve lr.
>
> Lastly the new macro is used to replace all the callsite of putn. This
> will simplify rework/review later on.
>
> Note that CurrentEL is now stored in x5 instead of x4 because the latter
> will be clobbered by the macro print_reg.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 84e26582c4..9142b4a774 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -90,8 +90,25 @@
>          bl    puts    ;     \
>          mov   lr, x3  ;     \
>          RODATA_STR(98, _s)
> +
> +/*
> + * Macro to print the value of register \xb
> + *
> + * Clobbers x0 - x4
> + */

Despite its name, this macro can't print x4. I would recommend adding at
least comment about this. Static assertion would be even better, but
looks like we don't have them for asm code.

> +.macro print_reg xb
> +        mov   x4, lr
> +        mov   x0, \xb
> +        bl    putn
> +        mov   lr, x4
> +.endm
> +
>  #else /* CONFIG_EARLY_PRINTK */
>  #define PRINT(s)
> +
> +.macro print_reg xb
> +.endm
> +
>  #endif /* !CONFIG_EARLY_PRINTK */
>
>  /* Load the physical address of a symbol into xb */
> @@ -304,22 +321,20 @@ GLOBAL(init_secondary)
>  #ifdef CONFIG_EARLY_PRINTK
>          ldr   x23, =EARLY_UART_BASE_ADDRESS /* x23 := UART base address */
>          PRINT("- CPU ")
> -        mov   x0, x24
> -        bl    putn
> +        print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
>
>  common_start:
>
>          PRINT("- Current EL ")
> -        mrs   x4, CurrentEL
> -        mov   x0, x4
> -        bl    putn
> +        mrs   x5, CurrentEL
> +        print_reg x5
>          PRINT(" -\r\n")
>
>          /* Are we in EL2 */
> -        cmp   x4, #PSR_MODE_EL2t
> -        ccmp  x4, #PSR_MODE_EL2h, #0x4, ne
> +        cmp   x5, #PSR_MODE_EL2t
> +        ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
>          b.eq  el2 /* Yes */
>
>          /* OK, we're boned. */


--
Best regards, Volodymyr Babchuk
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 05/17] xen/arm64: head: Introduce print_reg

[Julien Grall]

On 7/15/19 7:46 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi,

> Julien Grall writes:
> 
>> At the moment, the user should save x30/lr if it cares about it.
>>
>> Follow-up patches will introduce more use of putn in place where lr
>> should be preserved.
>>
>> Furthermore, any user of putn should also move the value to register x0
>> if it was stored in a different register.
>>
>> For convenience, a new macro is introduced to print a given register.
>> The macro will take care for us to move the value to x0 and also
>> preserve lr.
>>
>> Lastly the new macro is used to replace all the callsite of putn. This
>> will simplify rework/review later on.
>>
>> Note that CurrentEL is now stored in x5 instead of x4 because the latter
>> will be clobbered by the macro print_reg.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 29 ++++++++++++++++++++++-------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 84e26582c4..9142b4a774 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -90,8 +90,25 @@
>>           bl    puts    ;     \
>>           mov   lr, x3  ;     \
>>           RODATA_STR(98, _s)
>> +
>> +/*
>> + * Macro to print the value of register \xb
>> + *
>> + * Clobbers x0 - x4
>> + */
> 
> Despite its name, this macro can't print x4. I would recommend adding at
> least comment about this. Static assertion would be even better, but
> looks like we don't have them for asm code.

I have a new version of the function allowing to print x4. It is 
basically just swapping "mov x4, lr" with "mov x0, \xb".

> 
>> +.macro print_reg xb
>> +        mov   x4, lr
>> +        mov   x0, \xb
>> +        bl    putn
>> +        mov   lr, x4
>> +.endm
>> +
>>   #else /* CONFIG_EARLY_PRINTK */
>>   #define PRINT(s)

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 06/17] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Julien Grall]

The boot code is currently quite difficult to go through because of the
lack of documentation and a number of indirection to avoid executing
some path in either the boot CPU or secondary CPUs.

In an attempt to make the boot code easier to follow, each parts of the
boot are now in separate functions. Furthermore, the paths for the boot
CPU and secondary CPUs are now distincted and for now will call each
functions.

Follow-ups will remove unecessary calls and do further improvement
(such as adding documentation and reshuffling).

Note that the switch from using the ID mapping to the runtime mapping
is duplicated for each path. This is because in the future we will need
to stay longer in the ID mapping for the boot CPU.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 9142b4a774..ccd8a1b0a8 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -290,7 +290,19 @@ real_start_efi:
 
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
-        b     common_start
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the ID map. Jump to the runtime Virtual Address. */
+        ldr   x0, =primary_switched
+        br    x0
+primary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(real_start)
 
 GLOBAL(init_secondary)
         msr   DAIFSet, 0xf           /* Disable all interrupts */
@@ -324,9 +336,21 @@ GLOBAL(init_secondary)
         print_reg x24
         PRINT(" booting -\r\n")
 #endif
-
-common_start:
-
+        bl    check_cpu_mode
+        bl    zero_bss
+        bl    cpu_init
+        bl    create_page_tables
+        bl    enable_mmu
+
+        /* We are still in the ID map. Jump to the runtime Virtual Address. */
+        ldr   x0, =secondary_switched
+        br    x0
+secondary_switched:
+        bl    setup_fixmap
+        b     launch
+ENDPROC(init_secondary)
+
+check_cpu_mode:
         PRINT("- Current EL ")
         mrs   x5, CurrentEL
         print_reg x5
@@ -343,7 +367,10 @@ common_start:
         b fail
 
 el2:    PRINT("- Xen starting at EL2 -\r\n")
+        ret
+ENDPROC(check_cpu_mode)
 
+zero_bss:
         /* Zero BSS only when requested */
         cbnz  x26, skip_bss
 
@@ -356,6 +383,10 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
         b.lo  1b
 
 skip_bss:
+        ret
+ENDPROC(zero_bss)
+
+cpu_init:
         PRINT("- Setting up control registers -\r\n")
 
         /* Set up memory attribute type tables */
@@ -390,7 +421,10 @@ skip_bss:
          * are handled using the EL2 stack pointer, rather
          * than SP_EL0. */
         msr spsel, #1
+        ret
+ENDPROC(cpu_init)
 
+create_page_tables:
         /* Rebuild the boot pagetable's first-level entries. The structure
          * is described in mm.c.
          *
@@ -515,6 +549,10 @@ virtphys_clash:
         b     fail
 
 1:
+        ret
+ENDPROC(create_page_tables)
+
+enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
         /*
@@ -524,16 +562,16 @@ virtphys_clash:
         tlbi  alle2                  /* Flush hypervisor TLBs */
         dsb   nsh
 
-        ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
         mrs   x0, SCTLR_EL2
         orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
         orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
         dsb   sy                     /* Flush PTE writes and finish reads */
         msr   SCTLR_EL2, x0          /* now paging is enabled */
         isb                          /* Now, flush the icache */
-        br    x1                     /* Get a proper vaddr into PC */
-paging:
+        ret
+ENDPROC(enable_mmu)
 
+setup_fixmap:
         /* Now we can install the fixmap and dtb mappings, since we
          * don't need the 1:1 map any more */
         dsb   sy
@@ -575,7 +613,10 @@ paging:
         tlbi  alle2
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
+        ret
+ENDPROC(setup_fixmap)
 
+launch:
         PRINT("- Ready -\r\n")
 
         /* The boot CPU should go straight into C now */
@@ -594,7 +635,6 @@ paging:
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
 
-launch:
         ldr   x0, =init_data
         add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
         ldr   x0, [x0]
@@ -609,6 +649,7 @@ launch:
         b     start_xen              /* and disappear into the land of C */
 1:
         b     start_secondary        /* (to the appropriate entry point) */
+ENDPROC(launch)
 
 /* Fail-stop */
 fail:   PRINT("- Boot failed -\r\n")
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 06/17] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> The boot code is currently quite difficult to go through because of the
> lack of documentation and a number of indirection to avoid executing
> some path in either the boot CPU or secondary CPUs.
> 
> In an attempt to make the boot code easier to follow, each parts of the
> boot are now in separate functions. Furthermore, the paths for the boot
> CPU and secondary CPUs are now distincted and for now will call each
> functions.
> 
> Follow-ups will remove unecessary calls and do further improvement
> (such as adding documentation and reshuffling).
> 
> Note that the switch from using the ID mapping to the runtime mapping
> is duplicated for each path. This is because in the future we will need
> to stay longer in the ID mapping for the boot CPU.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 57 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 9142b4a774..ccd8a1b0a8 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -290,7 +290,19 @@ real_start_efi:
>  
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
> -        b     common_start
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */
> +        ldr   x0, =primary_switched
> +        br    x0
> +primary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(real_start)
>  
>  GLOBAL(init_secondary)
>          msr   DAIFSet, 0xf           /* Disable all interrupts */
> @@ -324,9 +336,21 @@ GLOBAL(init_secondary)
>          print_reg x24
>          PRINT(" booting -\r\n")
>  #endif
> -
> -common_start:
> -
> +        bl    check_cpu_mode
> +        bl    zero_bss
> +        bl    cpu_init
> +        bl    create_page_tables
> +        bl    enable_mmu
> +
> +        /* We are still in the ID map. Jump to the runtime Virtual Address. */
> +        ldr   x0, =secondary_switched
> +        br    x0
> +secondary_switched:
> +        bl    setup_fixmap
> +        b     launch
> +ENDPROC(init_secondary)
> +
> +check_cpu_mode:
>          PRINT("- Current EL ")
>          mrs   x5, CurrentEL
>          print_reg x5
> @@ -343,7 +367,10 @@ common_start:
>          b fail
>  
>  el2:    PRINT("- Xen starting at EL2 -\r\n")
> +        ret
> +ENDPROC(check_cpu_mode)
>  
> +zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss
>  
> @@ -356,6 +383,10 @@ el2:    PRINT("- Xen starting at EL2 -\r\n")
>          b.lo  1b
>  
>  skip_bss:
> +        ret
> +ENDPROC(zero_bss)
> +
> +cpu_init:
>          PRINT("- Setting up control registers -\r\n")
>  
>          /* Set up memory attribute type tables */
> @@ -390,7 +421,10 @@ skip_bss:
>           * are handled using the EL2 stack pointer, rather
>           * than SP_EL0. */
>          msr spsel, #1
> +        ret
> +ENDPROC(cpu_init)
>  
> +create_page_tables:
>          /* Rebuild the boot pagetable's first-level entries. The structure
>           * is described in mm.c.
>           *
> @@ -515,6 +549,10 @@ virtphys_clash:
>          b     fail
>  
>  1:
> +        ret
> +ENDPROC(create_page_tables)
> +
> +enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
>          /*
> @@ -524,16 +562,16 @@ virtphys_clash:
>          tlbi  alle2                  /* Flush hypervisor TLBs */
>          dsb   nsh
>  
> -        ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
>          mrs   x0, SCTLR_EL2
>          orr   x0, x0, #SCTLR_Axx_ELx_M  /* Enable MMU */
>          orr   x0, x0, #SCTLR_Axx_ELx_C  /* Enable D-cache */
>          dsb   sy                     /* Flush PTE writes and finish reads */
>          msr   SCTLR_EL2, x0          /* now paging is enabled */
>          isb                          /* Now, flush the icache */
> -        br    x1                     /* Get a proper vaddr into PC */
> -paging:
> +        ret
> +ENDPROC(enable_mmu)
>  
> +setup_fixmap:
>          /* Now we can install the fixmap and dtb mappings, since we
>           * don't need the 1:1 map any more */
>          dsb   sy
> @@ -575,7 +613,10 @@ paging:
>          tlbi  alle2
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
> +        ret
> +ENDPROC(setup_fixmap)
>  
> +launch:
>          PRINT("- Ready -\r\n")
>  
>          /* The boot CPU should go straight into C now */
> @@ -594,7 +635,6 @@ paging:
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
>  
> -launch:

Just below PRINT("- Ready -\r\n"), there is still a:

  cbz   x22, launch

moving the launch label up it looks like it will cause an infinite loop?

Everything else looks good, and I like the reorg of the code.


>          ldr   x0, =init_data
>          add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>          ldr   x0, [x0]
> @@ -609,6 +649,7 @@ launch:
>          b     start_xen              /* and disappear into the land of C */
>  1:
>          b     start_secondary        /* (to the appropriate entry point) */
> +ENDPROC(launch)


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

Re: [Xen-devel] [PATCH 06/17] xen/arm64: head: Introduce distinct paths for the boot CPU and secondary CPUs

[Julien Grall]

Hi Stefano,

On 26/06/2019 02:00, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> The boot code is currently quite difficult to go through because of the
>> lack of documentation and a number of indirection to avoid executing
>> some path in either the boot CPU or secondary CPUs.
>>
>> In an attempt to make the boot code easier to follow, each parts of the
>> boot are now in separate functions. Furthermore, the paths for the boot
>> CPU and secondary CPUs are now distincted and for now will call each

I notice a few typo in my commit message:

s/distincted/distinct/

>> functions.
>>
>> Follow-ups will remove unecessary calls and do further improvement

s/unecessary/unnecessary/

>> +launch:
>>           PRINT("- Ready -\r\n")
>>   
>>           /* The boot CPU should go straight into C now */
>> @@ -594,7 +635,6 @@ paging:
>>           dsb   sy                     /* Ensure completion of TLB flush */
>>           isb
>>   
>> -launch:
> 
> Just below PRINT("- Ready -\r\n"), there is still a:
> 
>    cbz   x22, launch
> 
> moving the launch label up it looks like it will cause an infinite loop?

Urgh. this line is dropped in a later patch, so the issue only would happen 
during bisection.

I will update the code to avoid the infinite loop here.

> 
> Everything else looks good, and I like the reorg of the code.

Thank you! I will rework the arm32 code the same way then :).

> 
> 
>>           ldr   x0, =init_data
>>           add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>>           ldr   x0, [x0]
>> @@ -609,6 +649,7 @@ launch:
>>           b     start_xen              /* and disappear into the land of C */
>>   1:
>>           b     start_secondary        /* (to the appropriate entry point) */
>> +ENDPROC(launch)
> 

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 07/17] xen/arm64: head: Rework and document check_cpu_mode()

[Julien Grall]

A branch in the success case can be avoided by inverting the branch
condition. At the same time, remove a pointless comment as Xen can only
run at EL2.

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index ccd8a1b0a8..87fcd3be6c 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -350,6 +350,13 @@ secondary_switched:
         b     launch
 ENDPROC(init_secondary)
 
+/*
+ * Check if the CPU has been booted in Hypervisor mode.
+ * This function will never return when the CPU is booted in another mode
+ * than Hypervisor mode.
+ *
+ * Clobbers x0 - x5
+ */
 check_cpu_mode:
         PRINT("- Current EL ")
         mrs   x5, CurrentEL
@@ -359,15 +366,13 @@ check_cpu_mode:
         /* Are we in EL2 */
         cmp   x5, #PSR_MODE_EL2t
         ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
-        b.eq  el2 /* Yes */
-
+        b.ne  1f /* No */
+        ret
+1:
         /* OK, we're boned. */
         PRINT("- Xen must be entered in NS EL2 mode -\r\n")
         PRINT("- Please update the bootloader -\r\n")
         b fail
-
-el2:    PRINT("- Xen starting at EL2 -\r\n")
-        ret
 ENDPROC(check_cpu_mode)
 
 zero_bss:
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 07/17] xen/arm64: head: Rework and document check_cpu_mode()

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> A branch in the success case can be avoided by inverting the branch
> condition. At the same time, remove a pointless comment as Xen can only
> run at EL2.
> 
> Lastly, document the behavior and the main registers usage within the
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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

> ---
>  xen/arch/arm/arm64/head.S | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index ccd8a1b0a8..87fcd3be6c 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -350,6 +350,13 @@ secondary_switched:
>          b     launch
>  ENDPROC(init_secondary)
>  
> +/*
> + * Check if the CPU has been booted in Hypervisor mode.
> + * This function will never return when the CPU is booted in another mode
> + * than Hypervisor mode.
> + *
> + * Clobbers x0 - x5
> + */
>  check_cpu_mode:
>          PRINT("- Current EL ")
>          mrs   x5, CurrentEL
> @@ -359,15 +366,13 @@ check_cpu_mode:
>          /* Are we in EL2 */
>          cmp   x5, #PSR_MODE_EL2t
>          ccmp  x5, #PSR_MODE_EL2h, #0x4, ne
> -        b.eq  el2 /* Yes */
> -
> +        b.ne  1f /* No */
> +        ret
> +1:
>          /* OK, we're boned. */
>          PRINT("- Xen must be entered in NS EL2 mode -\r\n")
>          PRINT("- Please update the bootloader -\r\n")
>          b fail
> -
> -el2:    PRINT("- Xen starting at EL2 -\r\n")
> -        ret
>  ENDPROC(check_cpu_mode)
>  
>  zero_bss:
> -- 
> 2.11.0
> 

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

[Xen-devel] [PATCH 08/17] xen/arm64: head: Rework and document zero_bss()

[Julien Grall]

On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
zeroed once at boot. So the call in the secondary CPUs path can be
removed. It also means that x26 does not need to set and is now only
used by the boot CPU.

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, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 87fcd3be6c..6aa3148192 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -71,7 +71,7 @@
  *  x23 - UART address
  *  x24 -
  *  x25 - identity map in place
- *  x26 - skip_zero_bss
+ *  x26 - skip_zero_bss (boot cpu only)
  *  x27 -
  *  x28 -
  *  x29 -
@@ -313,8 +313,6 @@ GLOBAL(init_secondary)
         sub   x20, x19, x0           /* x20 := phys-offset */
 
         mov   x22, #1                /* x22 := is_secondary_cpu */
-        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
-        mov   x26, #1                /* X26 := skip_zero_bss */
 
         mrs   x0, mpidr_el1
         ldr   x13, =(~MPIDR_HWID_MASK)
@@ -337,7 +335,6 @@ GLOBAL(init_secondary)
         PRINT(" booting -\r\n")
 #endif
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -375,6 +372,14 @@ check_cpu_mode:
         b fail
 ENDPROC(check_cpu_mode)
 
+/*
+ * Zero BSS
+ *
+ * Inputs:
+ *   x26: Do we need to zero BSS?
+ *
+ * Clobbers x0 - x3
+ */
 zero_bss:
         /* Zero BSS only when requested */
         cbnz  x26, skip_bss
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 08/17] xen/arm64: head: Rework and document zero_bss()

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
> zeroed once at boot. So the call in the secondary CPUs path can be
> removed. It also means that x26 does not need to set and is now only
> used by the boot CPU.
> 
> 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, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 87fcd3be6c..6aa3148192 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -71,7 +71,7 @@
>   *  x23 - UART address
>   *  x24 -
>   *  x25 - identity map in place
> - *  x26 - skip_zero_bss
> + *  x26 - skip_zero_bss (boot cpu only)

you could remove this, see below...


>   *  x27 -
>   *  x28 -
>   *  x29 -
> @@ -313,8 +313,6 @@ GLOBAL(init_secondary)
>          sub   x20, x19, x0           /* x20 := phys-offset */
>  
>          mov   x22, #1                /* x22 := is_secondary_cpu */
> -        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
> -        mov   x26, #1                /* X26 := skip_zero_bss */
>  
>          mrs   x0, mpidr_el1
>          ldr   x13, =(~MPIDR_HWID_MASK)
> @@ -337,7 +335,6 @@ GLOBAL(init_secondary)
>          PRINT(" booting -\r\n")
>  #endif
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -375,6 +372,14 @@ check_cpu_mode:
>          b fail
>  ENDPROC(check_cpu_mode)
>  
> +/*
> + * Zero BSS
> + *
> + * Inputs:
> + *   x26: Do we need to zero BSS?
> + *
> + * Clobbers x0 - x3
> + */
>  zero_bss:
>          /* Zero BSS only when requested */
>          cbnz  x26, skip_bss

In the commit message you wrote: "It also means that x26 does not need
to set and is now only used by the boot CPU." I think this statement is
correct, so you could also remove this "cbnz  x26, skip_bss" and also
the skip_bss label below.

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

Re: [Xen-devel] [PATCH 08/17] xen/arm64: head: Rework and document zero_bss()

[Julien Grall]

Hi Stefano,

On 26/06/2019 02:01, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> On secondary CPUs, zero_bss() will be a NOP because BSS only need to be
>> zeroed once at boot. So the call in the secondary CPUs path can be
>> removed. It also means that x26 does not need to set and is now only
>> used by the boot CPU.
>>
>> 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, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 87fcd3be6c..6aa3148192 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -71,7 +71,7 @@
>>    *  x23 - UART address
>>    *  x24 -
>>    *  x25 - identity map in place
>> - *  x26 - skip_zero_bss
>> + *  x26 - skip_zero_bss (boot cpu only)
> 
> you could remove this, see below...
> 
> 
>>    *  x27 -
>>    *  x28 -
>>    *  x29 -
>> @@ -313,8 +313,6 @@ GLOBAL(init_secondary)
>>           sub   x20, x19, x0           /* x20 := phys-offset */
>>   
>>           mov   x22, #1                /* x22 := is_secondary_cpu */
>> -        /* Boot CPU already zero BSS so skip it on secondary CPUs. */
>> -        mov   x26, #1                /* X26 := skip_zero_bss */
>>   
>>           mrs   x0, mpidr_el1
>>           ldr   x13, =(~MPIDR_HWID_MASK)
>> @@ -337,7 +335,6 @@ GLOBAL(init_secondary)
>>           PRINT(" booting -\r\n")
>>   #endif
>>           bl    check_cpu_mode
>> -        bl    zero_bss
>>           bl    cpu_init
>>           bl    create_page_tables
>>           bl    enable_mmu
>> @@ -375,6 +372,14 @@ check_cpu_mode:
>>           b fail
>>   ENDPROC(check_cpu_mode)
>>   
>> +/*
>> + * Zero BSS
>> + *
>> + * Inputs:
>> + *   x26: Do we need to zero BSS?
>> + *
>> + * Clobbers x0 - x3
>> + */
>>   zero_bss:
>>           /* Zero BSS only when requested */
>>           cbnz  x26, skip_bss
> 
> In the commit message you wrote: "It also means that x26 does not need
> to set and is now only used by the boot CPU." I think this statement is
> correct, so you could also remove this "cbnz  x26, skip_bss" and also
> the skip_bss label below.

I meant x26 does not need to be set on the secondary CPUs. However, we still 
need to keep it for boot CPU as we don't want to zero BSS when booting via EFI.

This is because the EFI stub will store information in BSS. Note that BSS was 
zeroed by EFI loader before hand.

I will reword the commit message.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 09/17] xen/arm64: head: Improve coding style and document cpu_init()

[Julien Grall]

Adjust the coding style used in the comments within cpu_init(). Take the
opportunity to alter the early print to match the function name.

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 | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 6aa3148192..ee0024173e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -396,19 +396,26 @@ skip_bss:
         ret
 ENDPROC(zero_bss)
 
+/*
+ * Initialize the processor for turning the MMU on.
+ *
+ * Clobbers x0 - x4
+ */
 cpu_init:
-        PRINT("- Setting up control registers -\r\n")
+        PRINT("- Initialize CPU -\r\n")
 
         /* Set up memory attribute type tables */
         ldr   x0, =MAIRVAL
         msr   mair_el2, x0
 
-        /* Set up TCR_EL2:
+        /*
+         * Set up TCR_EL2:
          * PS -- Based on ID_AA64MMFR0_EL1.PARange
          * Top byte is used
          * PT walks use Inner-Shareable accesses,
          * PT walks are write-back, write-allocate in both cache levels,
-         * 48-bit virtual address space goes through this table. */
+         * 48-bit virtual address space goes through this table.
+         */
         ldr   x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48))
         /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */
         mrs   x1, ID_AA64MMFR0_EL1
@@ -427,9 +434,11 @@ cpu_init:
         ldr   x0, =(HSCTLR_BASE)
         msr   SCTLR_EL2, x0
 
-        /* Ensure that any exceptions encountered at EL2
+        /*
+         * Ensure that any exceptions encountered at EL2
          * are handled using the EL2 stack pointer, rather
-         * than SP_EL0. */
+         * than SP_EL0.
+         */
         msr spsel, #1
         ret
 ENDPROC(cpu_init)
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 09/17] xen/arm64: head: Improve coding style and document cpu_init()

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> Adjust the coding style used in the comments within cpu_init(). Take the
> opportunity to alter the early print to match the function name.
> 
> 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 | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6aa3148192..ee0024173e 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -396,19 +396,26 @@ skip_bss:
>          ret
>  ENDPROC(zero_bss)
>  
> +/*
> + * Initialize the processor for turning the MMU on.
> + *
> + * Clobbers x0 - x4

Shouldn't it be x0 - x3?

The rest looks fine.


> + */
>  cpu_init:
> -        PRINT("- Setting up control registers -\r\n")
> +        PRINT("- Initialize CPU -\r\n")
>  
>          /* Set up memory attribute type tables */
>          ldr   x0, =MAIRVAL
>          msr   mair_el2, x0
>  
> -        /* Set up TCR_EL2:
> +        /*
> +         * Set up TCR_EL2:
>           * PS -- Based on ID_AA64MMFR0_EL1.PARange
>           * Top byte is used
>           * PT walks use Inner-Shareable accesses,
>           * PT walks are write-back, write-allocate in both cache levels,
> -         * 48-bit virtual address space goes through this table. */
> +         * 48-bit virtual address space goes through this table.
> +         */
>          ldr   x0, =(TCR_RES1|TCR_SH0_IS|TCR_ORGN0_WBWA|TCR_IRGN0_WBWA|TCR_T0SZ(64-48))
>          /* ID_AA64MMFR0_EL1[3:0] (PARange) corresponds to TCR_EL2[18:16] (PS) */
>          mrs   x1, ID_AA64MMFR0_EL1
> @@ -427,9 +434,11 @@ cpu_init:
>          ldr   x0, =(HSCTLR_BASE)
>          msr   SCTLR_EL2, x0
>  
> -        /* Ensure that any exceptions encountered at EL2
> +        /*
> +         * Ensure that any exceptions encountered at EL2
>           * are handled using the EL2 stack pointer, rather
> -         * than SP_EL0. */
> +         * than SP_EL0.
> +         */
>          msr spsel, #1
>          ret
>  ENDPROC(cpu_init)
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 09/17] xen/arm64: head: Improve coding style and document cpu_init()

[Julien Grall]

Hi Stefano,

On 26/06/2019 02:01, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> Adjust the coding style used in the comments within cpu_init(). Take the
>> opportunity to alter the early print to match the function name.
>>
>> 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 | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 6aa3148192..ee0024173e 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -396,19 +396,26 @@ skip_bss:
>>           ret
>>   ENDPROC(zero_bss)
>>   
>> +/*
>> + * Initialize the processor for turning the MMU on.
>> + *
>> + * Clobbers x0 - x4
> 
> Shouldn't it be x0 - x3?

Yes it should be. I will update the comment.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 10/17] xen/arm64: head: Improve coding style and document create_pages_tables()

[Julien Grall]

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
+ *
+ * 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


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

Re: [Xen-devel] [PATCH 10/17] xen/arm64: head: Improve coding style and document create_pages_tables()

[Stefano Stabellini]

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
> 

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

Re: [Xen-devel] [PATCH 10/17] xen/arm64: head: Improve coding style and document create_pages_tables()

[Julien Grall]

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,

-- 
Julien Grall

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

[Xen-devel] [PATCH 11/17] xen/arm64: head: Document enable_mmu()

[Julien Grall]

Document the behavior and the main registers usage within enable_mmu().

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 7b92c1c8eb..d673f7c0d8 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -583,6 +583,13 @@ virtphys_clash:
         ret
 ENDPROC(create_page_tables)
 
+/*
+ * Turn on the Data Cache and the MMU. The function will return on the ID
+ * mapping. In other word, the caller is responsible to switch to the runtime
+ * mapping.
+ *
+ * Clobbers x0 - x1
+ */
 enable_mmu:
         PRINT("- Turning on paging -\r\n")
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 11/17] xen/arm64: head: Document enable_mmu()

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> Document the behavior and the main registers usage within enable_mmu().
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 7b92c1c8eb..d673f7c0d8 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -583,6 +583,13 @@ virtphys_clash:
>          ret
>  ENDPROC(create_page_tables)
>  
> +/*
> + * Turn on the Data Cache and the MMU. The function will return on the ID
> + * mapping. In other word, the caller is responsible to switch to the runtime
> + * mapping.
> + *
> + * Clobbers x0 - x1
> + */

as it calls PRINT, shouldn't it be x0 - x3?

>  enable_mmu:
>          PRINT("- Turning on paging -\r\n")
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 11/17] xen/arm64: head: Document enable_mmu()

[Julien Grall]

Hi Stefano,

On 26/06/2019 02:03, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> Document the behavior and the main registers usage within enable_mmu().
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/arm64/head.S | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 7b92c1c8eb..d673f7c0d8 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -583,6 +583,13 @@ virtphys_clash:
>>           ret
>>   ENDPROC(create_page_tables)
>>   
>> +/*
>> + * Turn on the Data Cache and the MMU. The function will return on the ID
>> + * mapping. In other word, the caller is responsible to switch to the runtime
>> + * mapping.
>> + *
>> + * Clobbers x0 - x1
>> + */
> 
> as it calls PRINT, shouldn't it be x0 - x3?

You are right. I will update the comment.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 12/17] xen/arm64: head: Move assembly switch to the runtime PT in secondary CPUs path

[Julien Grall]

The assembly switch to the runtime PT is only necessary for the
secondary CPUs. So move the code in the secondary CPUs path.

While this is definitely not compliant with the Arm Arm as we are
switching between two differents set of page-tables without turning off
the MMU. Turning off the MMU is impossible here as the ID map may clash
with other mappings in the runtime page-tables. This will require more
rework to avoid the problem. So for now add a TODO in the code.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index d673f7c0d8..6be4af7579 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -344,6 +344,23 @@ GLOBAL(init_secondary)
         br    x0
 secondary_switched:
         bl    setup_fixmap
+
+        /*
+         * Non-boot CPUs need to move on to the proper pagetables, which were
+         * setup in init_secondary_pagetables.
+         *
+         * XXX: This is not compliant with the Arm Arm.
+         */
+        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
+        ldr   x4, [x4]               /* Actual value */
+        dsb   sy
+        msr   TTBR0_EL2, x4
+        dsb   sy
+        isb
+        tlbi  alle2
+        dsb   sy                     /* Ensure completion of TLB flush */
+        isb
+
         b     launch
 ENDPROC(init_secondary)
 
@@ -657,22 +674,6 @@ ENDPROC(setup_fixmap)
 launch:
         PRINT("- Ready -\r\n")
 
-        /* The boot CPU should go straight into C now */
-        cbz   x22, launch
-
-        /* Non-boot CPUs need to move on to the proper pagetables, which were
-         * setup in init_secondary_pagetables. */
-
-        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
-        ldr   x4, [x4]               /* Actual value */
-        dsb   sy
-        msr   TTBR0_EL2, x4
-        dsb   sy
-        isb
-        tlbi  alle2
-        dsb   sy                     /* Ensure completion of TLB flush */
-        isb
-
         ldr   x0, =init_data
         add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
         ldr   x0, [x0]
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 12/17] xen/arm64: head: Move assembly switch to the runtime PT in secondary CPUs path

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> The assembly switch to the runtime PT is only necessary for the
> secondary CPUs. So move the code in the secondary CPUs path.
> 
> While this is definitely not compliant with the Arm Arm as we are
> switching between two differents set of page-tables without turning off
> the MMU. Turning off the MMU is impossible here as the ID map may clash
> with other mappings in the runtime page-tables. This will require more
> rework to avoid the problem. So for now add a TODO in the code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/arm64/head.S | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d673f7c0d8..6be4af7579 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -344,6 +344,23 @@ GLOBAL(init_secondary)
>          br    x0
>  secondary_switched:
>          bl    setup_fixmap
> +
> +        /*
> +         * Non-boot CPUs need to move on to the proper pagetables, which were
> +         * setup in init_secondary_pagetables.
> +         *
> +         * XXX: This is not compliant with the Arm Arm.
> +         */
> +        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
> +        ldr   x4, [x4]               /* Actual value */
> +        dsb   sy
> +        msr   TTBR0_EL2, x4
> +        dsb   sy
> +        isb
> +        tlbi  alle2
> +        dsb   sy                     /* Ensure completion of TLB flush */
> +        isb
> +
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -657,22 +674,6 @@ ENDPROC(setup_fixmap)
>  launch:
>          PRINT("- Ready -\r\n")
>  
> -        /* The boot CPU should go straight into C now */
> -        cbz   x22, launch
> -
> -        /* Non-boot CPUs need to move on to the proper pagetables, which were
> -         * setup in init_secondary_pagetables. */
> -
> -        ldr   x4, =init_ttbr         /* VA of TTBR0_EL2 stashed by CPU 0 */
> -        ldr   x4, [x4]               /* Actual value */
> -        dsb   sy
> -        msr   TTBR0_EL2, x4
> -        dsb   sy
> -        isb
> -        tlbi  alle2
> -        dsb   sy                     /* Ensure completion of TLB flush */
> -        isb
> -
>          ldr   x0, =init_data
>          add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>          ldr   x0, [x0]
> -- 
> 2.11.0
> 

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

[Xen-devel] [PATCH 13/17] xen/arm64: head: Don't setup the fixmap on secondary CPUs

[Julien Grall]

setup_fixmap() will setup the fixmap in the boot page tables in order to
use earlyprintk and also update the register x23 holding the address to
the UART.

However, secondary CPUs are switching to the runtime page tables before
using the earlyprintk again. So settting up the fixmap in the boot pages
tables is pointless.

This means most of setup_fixmap() is not necessary for the secondary
CPUs. The update of UART address is now moved out of setup_fixmap() and
duplicated in the CPU boot and secondary CPUs boot. Additionally, the
call to setup_fixmap() is removed from secondary CPUs boot.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 6be4af7579..192af3e8a2 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -301,6 +301,10 @@ real_start_efi:
         br    x0
 primary_switched:
         bl    setup_fixmap
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(real_start)
 
@@ -343,8 +347,6 @@ GLOBAL(init_secondary)
         ldr   x0, =secondary_switched
         br    x0
 secondary_switched:
-        bl    setup_fixmap
-
         /*
          * Non-boot CPUs need to move on to the proper pagetables, which were
          * setup in init_secondary_pagetables.
@@ -361,6 +363,10 @@ secondary_switched:
         dsb   sy                     /* Ensure completion of TLB flush */
         isb
 
+#ifdef CONFIG_EARLY_PRINTK
+        /* Use a virtual address to access the UART. */
+        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
+#endif
         b     launch
 ENDPROC(init_secondary)
 
@@ -631,10 +637,6 @@ setup_fixmap:
          * don't need the 1:1 map any more */
         dsb   sy
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
-        /* Non-boot CPUs don't need to rebuild the fixmap itself, just
-         * the mapping from boot_second to xen_fixmap */
-        cbnz  x22, 1f
-
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
         lsr   x2, x23, #THIRD_SHIFT
@@ -642,7 +644,6 @@ 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 */
-1:
 
         /* Map fixmap into boot_second */
         ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
@@ -652,9 +653,6 @@ setup_fixmap:
         ldr   x1, =FIXMAP_ADDR(0)
         lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
         str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-
-        /* Use a virtual address to access the UART. */
-        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
 
         /*
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 13/17] xen/arm64: head: Don't setup the fixmap on secondary CPUs

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> setup_fixmap() will setup the fixmap in the boot page tables in order to
> use earlyprintk and also update the register x23 holding the address to
> the UART.
> 
> However, secondary CPUs are switching to the runtime page tables before
> using the earlyprintk again. So settting up the fixmap in the boot pages
> tables is pointless.

Typo: settting

Also, you could add that it is "impossible" to find ourselves in the
position of needing earlyprintk for secondary CPUs before the runtime
page tables are up, because it is done right after in the boot sequence.

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


> This means most of setup_fixmap() is not necessary for the secondary
> CPUs. The update of UART address is now moved out of setup_fixmap() and
> duplicated in the CPU boot and secondary CPUs boot. Additionally, the
> call to setup_fixmap() is removed from secondary CPUs boot.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm64/head.S | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6be4af7579..192af3e8a2 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -301,6 +301,10 @@ real_start_efi:
>          br    x0
>  primary_switched:
>          bl    setup_fixmap
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Use a virtual address to access the UART. */
> +        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> +#endif
>          b     launch
>  ENDPROC(real_start)
>  
> @@ -343,8 +347,6 @@ GLOBAL(init_secondary)
>          ldr   x0, =secondary_switched
>          br    x0
>  secondary_switched:
> -        bl    setup_fixmap
> -
>          /*
>           * Non-boot CPUs need to move on to the proper pagetables, which were
>           * setup in init_secondary_pagetables.
> @@ -361,6 +363,10 @@ secondary_switched:
>          dsb   sy                     /* Ensure completion of TLB flush */
>          isb
>  
> +#ifdef CONFIG_EARLY_PRINTK
> +        /* Use a virtual address to access the UART. */
> +        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
> +#endif
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -631,10 +637,6 @@ setup_fixmap:
>           * don't need the 1:1 map any more */
>          dsb   sy
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
> -        /* Non-boot CPUs don't need to rebuild the fixmap itself, just
> -         * the mapping from boot_second to xen_fixmap */
> -        cbnz  x22, 1f
> -
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
>          lsr   x2, x23, #THIRD_SHIFT
> @@ -642,7 +644,6 @@ 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 */
> -1:
>  
>          /* Map fixmap into boot_second */
>          ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> @@ -652,9 +653,6 @@ setup_fixmap:
>          ldr   x1, =FIXMAP_ADDR(0)
>          lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
> -
> -        /* Use a virtual address to access the UART. */
> -        ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
>  
>          /*
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 13/17] xen/arm64: head: Don't setup the fixmap on secondary CPUs

[Julien Grall]

Hi,

On 6/26/19 7:51 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> setup_fixmap() will setup the fixmap in the boot page tables in order to
>> use earlyprintk and also update the register x23 holding the address to
>> the UART.
>>
>> However, secondary CPUs are switching to the runtime page tables before
>> using the earlyprintk again. So settting up the fixmap in the boot pages
>> tables is pointless.
> 
> Typo: settting

ok.

> 
> Also, you could add that it is "impossible" to find ourselves in the
> position of needing earlyprintk for secondary CPUs before the runtime
> page tables are up, because it is done right after in the boot sequence.

It is kind of implied by the comment and the code below. But I can 
clearly state it.

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

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

The ID map may clash with other parts of the Xen virtual memory layout.
At the moment, Xen is handling the clash by only creating a mapping to
the runtime virtual address before enabling the MMU.

The rest of the mappings (such as the fixmap) will be mapped after the
MMU is enabled. However, the code doing the mapping is not safe as it
replace mapping without using the Break-Before-Make sequence.

As the ID map can be anywhere in the memory, it is easier to remove all
the entries added as soon as the ID map is not used rather than adding
the Break-Before-Make sequence everywhere.

It is difficult to track where exactly the ID map was created without a
full rework of create_page_tables(). Instead, introduce a new function
remove_id_map() will look where is the top-level entry for the ID map
and remove it.

The new function is only called for the boot CPU. Secondary CPUs will
switch directly to the runtime page-tables so there are no need to
remove the ID mapping. Note that this still doesn't make the Secondary
CPUs path safe but it is not making it worst.

---
    Note that the comment refers to the patch  "xen/arm: tlbflush: Rework
    TLB helpers" under review (see [1]).

    Furthermore, it is very likely we will need to re-introduce the ID
    map to cater secondary CPUs boot and suspend/resume. For now, the
    attempt is to make boot CPU path fully Arm Arm compliant.

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html
---
 xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 192af3e8a2..96e85f8834 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -300,6 +300,13 @@ real_start_efi:
         ldr   x0, =primary_switched
         br    x0
 primary_switched:
+        /*
+         * The ID map may clash with other parts of the Xen virtual memory
+         * layout. As it is not used anymore, remove it completely to
+         * avoid having to worry about replacing existing mapping
+         * afterwards.
+         */
+        bl    remove_id_map
         bl    setup_fixmap
 #ifdef CONFIG_EARLY_PRINTK
         /* Use a virtual address to access the UART. */
@@ -632,10 +639,68 @@ enable_mmu:
         ret
 ENDPROC(enable_mmu)
 
+/*
+ * Remove the ID map for the page-tables. It is not easy to keep track
+ * where the ID map was mapped, so we will look for the top-level entry
+ * exclusive to the ID Map and remove it.
+ *
+ * Inputs:
+ *   x19: paddr(start)
+ *
+ * Clobbers x0 - x1
+ */
+remove_id_map:
+        /*
+         * Find the zeroeth slot used. Remove the entry from zeroeth
+         * table if the slot is not 0. For slot 0, the ID map was either
+         * done in first or second table.
+         */
+        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
+        cbz   x1, 1f
+        /* It is not in slot 0, remove the entry */
+        ldr   x0, =boot_pgtable         /* x0 := root table */
+        str   xzr, [x0, x1, lsl #3]
+        b     id_map_removed
+
+1:
+        /*
+         * Find the first slot used. Remove the entry for the first
+         * table if the slot is not 0. For slot 0, the ID map was done
+         * in the second table.
+         */
+        lsr   x1, x19, #FIRST_SHIFT
+        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
+        cbz   x1, 1f
+        /* It is not in slot 0, remove the entry */
+        ldr   x0, =boot_first           /* x0 := first table */
+        str   xzr, [x0, x1, lsl #3]
+        b     id_map_removed
+
+1:
+        /*
+         * Find the second slot used. Remove the entry for the first
+         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, it means the ID map was not created.
+         */
+        lsr   x1, x19, #SECOND_SHIFT
+        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
+        cmp   x1, #1
+        beq   id_map_removed
+        /* It is not in slot 1, remove the entry */
+        ldr   x0, =boot_second          /* x0 := second table */
+        str   xzr, [x0, x1, lsl #3]
+
+id_map_removed:
+        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
+        dsb   nshst
+        tlbi  alle2
+        dsb   nsh
+        isb
+
+        ret
+ENDPROC(remove_id_map)
+
 setup_fixmap:
-        /* Now we can install the fixmap and dtb mappings, since we
-         * don't need the 1:1 map any more */
-        dsb   sy
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
         /* Add UART to the fixmap table */
         ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
@@ -653,19 +718,10 @@ setup_fixmap:
         ldr   x1, =FIXMAP_ADDR(0)
         lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
         str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-#endif
 
-        /*
-         * Flush the TLB in case the 1:1 mapping happens to clash with
-         * the virtual addresses used by the fixmap or DTB.
-         */
-        dsb   sy                     /* Ensure any page table updates made above
-                                      * have occurred. */
-
-        isb
-        tlbi  alle2
-        dsb   sy                     /* Ensure completion of TLB flush */
-        isb
+        /* Ensure any page table updates made above have occurred */
+        dsb   nshst
+#endif
         ret
 ENDPROC(setup_fixmap)
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> The ID map may clash with other parts of the Xen virtual memory layout.
> At the moment, Xen is handling the clash by only creating a mapping to
> the runtime virtual address before enabling the MMU.
> 
> The rest of the mappings (such as the fixmap) will be mapped after the
> MMU is enabled. However, the code doing the mapping is not safe as it
> replace mapping without using the Break-Before-Make sequence.
> 
> As the ID map can be anywhere in the memory, it is easier to remove all
> the entries added as soon as the ID map is not used rather than adding
> the Break-Before-Make sequence everywhere.

I think it is a good idea, but I would ask you to mention 1:1 map
instead of "ID map" in comments and commit messages because that is the
wording we used in all comments so far (see the one in
create_page_tables and mm.c). It is easier to grep and refer to if we
use the same nomenclature. Note that I don't care about which
nomenclature we decide to use, I am only asking for consistency.
Otherwise, it would also work if you say it both way at least once:

 The ID map (1:1 map) may clash [...]


> It is difficult to track where exactly the ID map was created without a
> full rework of create_page_tables(). Instead, introduce a new function
> remove_id_map() will look where is the top-level entry for the ID map
> and remove it.

Do you think it would be worth simplifying this code below by preserving
where/how the ID map was created? We could repurpose x25 for that,
carrying for instance the address of the ID map section slot or a code
number to specify which case we are dealing with. We should be able to
turn remove_id_map into only ~5 lines?


> The new function is only called for the boot CPU. Secondary CPUs will
> switch directly to the runtime page-tables so there are no need to
> remove the ID mapping. Note that this still doesn't make the Secondary
> CPUs path safe but it is not making it worst.
> 
> ---
>     Note that the comment refers to the patch  "xen/arm: tlbflush: Rework
>     TLB helpers" under review (see [1]).
> 
>     Furthermore, it is very likely we will need to re-introduce the ID
>     map to cater secondary CPUs boot and suspend/resume. For now, the
>     attempt is to make boot CPU path fully Arm Arm compliant.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html
> ---
>  xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 192af3e8a2..96e85f8834 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -300,6 +300,13 @@ real_start_efi:
>          ldr   x0, =primary_switched
>          br    x0
>  primary_switched:
> +        /*
> +         * The ID map may clash with other parts of the Xen virtual memory
> +         * layout. As it is not used anymore, remove it completely to
> +         * avoid having to worry about replacing existing mapping
> +         * afterwards.
> +         */
> +        bl    remove_id_map
>          bl    setup_fixmap
>  #ifdef CONFIG_EARLY_PRINTK
>          /* Use a virtual address to access the UART. */
> @@ -632,10 +639,68 @@ enable_mmu:
>          ret
>  ENDPROC(enable_mmu)
>  
> +/*
> + * Remove the ID map for the page-tables. It is not easy to keep track
> + * where the ID map was mapped, so we will look for the top-level entry
> + * exclusive to the ID Map and remove it.
> + *
> + * Inputs:
> + *   x19: paddr(start)
> + *
> + * Clobbers x0 - x1
> + */
> +remove_id_map:
> +        /*
> +         * Find the zeroeth slot used. Remove the entry from zeroeth
> +         * table if the slot is not 0. For slot 0, the ID map was either
> +         * done in first or second table.
> +         */
> +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
> +        cbz   x1, 1f
> +        /* It is not in slot 0, remove the entry */
> +        ldr   x0, =boot_pgtable         /* x0 := root table */
> +        str   xzr, [x0, x1, lsl #3]
> +        b     id_map_removed
> +
> +1:
> +        /*
> +         * Find the first slot used. Remove the entry for the first
> +         * table if the slot is not 0. For slot 0, the ID map was done
> +         * in the second table.
> +         */
> +        lsr   x1, x19, #FIRST_SHIFT
> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> +        cbz   x1, 1f
> +        /* It is not in slot 0, remove the entry */
> +        ldr   x0, =boot_first           /* x0 := first table */
> +        str   xzr, [x0, x1, lsl #3]
> +        b     id_map_removed
> +
> +1:
> +        /*
> +         * Find the second slot used. Remove the entry for the first
> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
> +         * For slot 1, it means the ID map was not created.
> +         */
> +        lsr   x1, x19, #SECOND_SHIFT
> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> +        cmp   x1, #1
> +        beq   id_map_removed
> +        /* It is not in slot 1, remove the entry */
> +        ldr   x0, =boot_second          /* x0 := second table */
> +        str   xzr, [x0, x1, lsl #3]
> +
> +id_map_removed:
> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
> +        dsb   nshst
> +        tlbi  alle2
> +        dsb   nsh
> +        isb
> +
> +        ret
> +ENDPROC(remove_id_map)
> +
>  setup_fixmap:
> -        /* Now we can install the fixmap and dtb mappings, since we
> -         * don't need the 1:1 map any more */
> -        dsb   sy
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
> @@ -653,19 +718,10 @@ setup_fixmap:
>          ldr   x1, =FIXMAP_ADDR(0)
>          lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
> -#endif
>  
> -        /*
> -         * Flush the TLB in case the 1:1 mapping happens to clash with
> -         * the virtual addresses used by the fixmap or DTB.
> -         */
> -        dsb   sy                     /* Ensure any page table updates made above
> -                                      * have occurred. */
> -
> -        isb
> -        tlbi  alle2
> -        dsb   sy                     /* Ensure completion of TLB flush */
> -        isb
> +        /* Ensure any page table updates made above have occurred */
> +        dsb   nshst
> +#endif
>          ret
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

Hi Stefano,

On 6/26/19 9:25 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> The ID map may clash with other parts of the Xen virtual memory layout.
>> At the moment, Xen is handling the clash by only creating a mapping to
>> the runtime virtual address before enabling the MMU.
>>
>> The rest of the mappings (such as the fixmap) will be mapped after the
>> MMU is enabled. However, the code doing the mapping is not safe as it
>> replace mapping without using the Break-Before-Make sequence.
>>
>> As the ID map can be anywhere in the memory, it is easier to remove all
>> the entries added as soon as the ID map is not used rather than adding
>> the Break-Before-Make sequence everywhere.
> 
> I think it is a good idea, but I would ask you to mention 1:1 map
> instead of "ID map" in comments and commit messages because that is the
> wording we used in all comments so far (see the one in
> create_page_tables and mm.c). It is easier to grep and refer to if we
> use the same nomenclature. Note that I don't care about which
> nomenclature we decide to use, I am only asking for consistency.
> Otherwise, it would also work if you say it both way at least once:
> 
>   The ID map (1:1 map) may clash [...]

I would rather drop the wording 1:1 as this is confusing. It is also not 
trivial to find anything on google typing "1:1".

> 
> 
>> It is difficult to track where exactly the ID map was created without a
>> full rework of create_page_tables(). Instead, introduce a new function
>> remove_id_map() will look where is the top-level entry for the ID map
>> and remove it.
> 
> Do you think it would be worth simplifying this code below by preserving
> where/how the ID map was created? We could repurpose x25 for that,
> carrying for instance the address of the ID map section slot or a code
> number to specify which case we are dealing with. We should be able to
> turn remove_id_map into only ~5 lines?

I thought about it but the current implementation of create_map_tables() 
is quite awful to read. So the less I touch this function, the better I 
feel :).

I have some rework for the create_page_tables() which simplify it a lot. 
Yet, I am not entirely sure it is worth to spend time trying to simplify 
remove_id_map. This is unlikely to make the boot significantly faster 
and I don't expect the function to survive more than a release as the ID 
map as to be kept in place (for secondary boot and suspend/resume).

The only reason it is removed now is because it clashes with other 
mapping we may do.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Andrew Cooper]

On 26/06/2019 21:39, Julien Grall wrote:
> On 6/26/19 9:25 PM, Stefano Stabellini wrote:
>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>> The ID map may clash with other parts of the Xen virtual memory layout.
>>> At the moment, Xen is handling the clash by only creating a mapping to
>>> the runtime virtual address before enabling the MMU.
>>>
>>> The rest of the mappings (such as the fixmap) will be mapped after the
>>> MMU is enabled. However, the code doing the mapping is not safe as it
>>> replace mapping without using the Break-Before-Make sequence.
>>>
>>> As the ID map can be anywhere in the memory, it is easier to remove all
>>> the entries added as soon as the ID map is not used rather than adding
>>> the Break-Before-Make sequence everywhere.
>>
>> I think it is a good idea, but I would ask you to mention 1:1 map
>> instead of "ID map" in comments and commit messages because that is the
>> wording we used in all comments so far (see the one in
>> create_page_tables and mm.c). It is easier to grep and refer to if we
>> use the same nomenclature. Note that I don't care about which
>> nomenclature we decide to use, I am only asking for consistency.
>> Otherwise, it would also work if you say it both way at least once:
>>
>>   The ID map (1:1 map) may clash [...]
>
> I would rather drop the wording 1:1 as this is confusing. It is also
> not trivial to find anything on google typing "1:1".

"one-to-one mapping", or "identity map" are both common terminology. 
1:1 is a common representation for the former, whereas ID is not a
abbreviation of "Identity".

If you don't want to use 1:1, then you need to say "The identity map" to
retain clarity.

~Andrew

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini]

On Wed, 26 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/26/19 9:25 PM, Stefano Stabellini wrote:
> > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > The ID map may clash with other parts of the Xen virtual memory layout.
> > > At the moment, Xen is handling the clash by only creating a mapping to
> > > the runtime virtual address before enabling the MMU.
> > > 
> > > The rest of the mappings (such as the fixmap) will be mapped after the
> > > MMU is enabled. However, the code doing the mapping is not safe as it
> > > replace mapping without using the Break-Before-Make sequence.
> > > 
> > > As the ID map can be anywhere in the memory, it is easier to remove all
> > > the entries added as soon as the ID map is not used rather than adding
> > > the Break-Before-Make sequence everywhere.
> > 
> > I think it is a good idea, but I would ask you to mention 1:1 map
> > instead of "ID map" in comments and commit messages because that is the
> > wording we used in all comments so far (see the one in
> > create_page_tables and mm.c). It is easier to grep and refer to if we
> > use the same nomenclature. Note that I don't care about which
> > nomenclature we decide to use, I am only asking for consistency.
> > Otherwise, it would also work if you say it both way at least once:
> > 
> >   The ID map (1:1 map) may clash [...]
> 
> I would rather drop the wording 1:1 as this is confusing. It is also not
> trivial to find anything on google typing "1:1".

That's fine too

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> The ID map may clash with other parts of the Xen virtual memory layout.
> At the moment, Xen is handling the clash by only creating a mapping to
> the runtime virtual address before enabling the MMU.
> 
> The rest of the mappings (such as the fixmap) will be mapped after the
> MMU is enabled. However, the code doing the mapping is not safe as it
> replace mapping without using the Break-Before-Make sequence.
> 
> As the ID map can be anywhere in the memory, it is easier to remove all
> the entries added as soon as the ID map is not used rather than adding
> the Break-Before-Make sequence everywhere.
> 
> It is difficult to track where exactly the ID map was created without a
> full rework of create_page_tables(). Instead, introduce a new function
> remove_id_map() will look where is the top-level entry for the ID map
> and remove it.
> 
> The new function is only called for the boot CPU. Secondary CPUs will
> switch directly to the runtime page-tables so there are no need to
> remove the ID mapping. Note that this still doesn't make the Secondary
> CPUs path safe but it is not making it worst.
> 
> ---
>     Note that the comment refers to the patch  "xen/arm: tlbflush: Rework
>     TLB helpers" under review (see [1]).
> 
>     Furthermore, it is very likely we will need to re-introduce the ID
>     map to cater secondary CPUs boot and suspend/resume. For now, the
>     attempt is to make boot CPU path fully Arm Arm compliant.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-05/msg01134.html
> ---
>  xen/arch/arm/arm64/head.S | 86 ++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 192af3e8a2..96e85f8834 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -300,6 +300,13 @@ real_start_efi:
>          ldr   x0, =primary_switched
>          br    x0
>  primary_switched:
> +        /*
> +         * The ID map may clash with other parts of the Xen virtual memory
> +         * layout. As it is not used anymore, remove it completely to
> +         * avoid having to worry about replacing existing mapping
> +         * afterwards.
> +         */
> +        bl    remove_id_map
>          bl    setup_fixmap
>  #ifdef CONFIG_EARLY_PRINTK
>          /* Use a virtual address to access the UART. */
> @@ -632,10 +639,68 @@ enable_mmu:
>          ret
>  ENDPROC(enable_mmu)
>  
> +/*
> + * Remove the ID map for the page-tables. It is not easy to keep track
> + * where the ID map was mapped, so we will look for the top-level entry
> + * exclusive to the ID Map and remove it.
> + *
> + * Inputs:
> + *   x19: paddr(start)
> + *
> + * Clobbers x0 - x1
> + */
> +remove_id_map:
> +        /*
> +         * Find the zeroeth slot used. Remove the entry from zeroeth
> +         * table if the slot is not 0. For slot 0, the ID map was either
> +         * done in first or second table.
> +         */
> +        lsr   x1, x19, #ZEROETH_SHIFT   /* x1 := zeroeth slot */
> +        cbz   x1, 1f
> +        /* It is not in slot 0, remove the entry */
> +        ldr   x0, =boot_pgtable         /* x0 := root table */
> +        str   xzr, [x0, x1, lsl #3]
> +        b     id_map_removed
> +
> +1:
> +        /*
> +         * Find the first slot used. Remove the entry for the first
> +         * table if the slot is not 0. For slot 0, the ID map was done
> +         * in the second table.
> +         */
> +        lsr   x1, x19, #FIRST_SHIFT
> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> +        cbz   x1, 1f
> +        /* It is not in slot 0, remove the entry */
> +        ldr   x0, =boot_first           /* x0 := first table */
> +        str   xzr, [x0, x1, lsl #3]
> +        b     id_map_removed
> +
> +1:
> +        /*
> +         * Find the second slot used. Remove the entry for the first
> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
> +         * For slot 1, it means the ID map was not created.
> +         */
> +        lsr   x1, x19, #SECOND_SHIFT
> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> +        cmp   x1, #1
> +        beq   id_map_removed
> +        /* It is not in slot 1, remove the entry */
> +        ldr   x0, =boot_second          /* x0 := second table */
> +        str   xzr, [x0, x1, lsl #3]

Wouldn't it be a bit more reliable if we checked whether the slot in
question for x19 (whether zero, first, second) is a pagetable pointer or
section map, then zero it if it is a section map, otherwise go down one
level? If we did it this way it would be independent from the way
create_page_tables is written.

With the current code, we are somewhat reliant on the behavior of
create_page_tables, because we rely on the position of the slot for
the ID map? Where the assumption for instance is that at level one, if
the slot is zero, then we need to go down a level, etc. Instead, if we
checked if the slot is a section map, we could remove it immediately, if
it is a pagetable pointer, we proceed. The code should be similar in
complexity and LOC, but it would be more robust.

Something like the following, in pseudo-uncompiled assembly:

     lsr   x1, x19, #FIRST_SHIFT
     ldr   x0, =boot_first           /* x0 := first table */
     ldr   x2, [x0, x1, lsl #3]
     # check x2 against #PT_MEM
     cbz   x2, 1f
     str   xzr, [x0, x1, lsl #3]
     b     id_map_removed


> +id_map_removed:
> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */

Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the
explanation you are referring to.


> +        dsb   nshst
> +        tlbi  alle2
> +        dsb   nsh
> +        isb
> +
> +        ret
> +ENDPROC(remove_id_map)
> +
>  setup_fixmap:
> -        /* Now we can install the fixmap and dtb mappings, since we
> -         * don't need the 1:1 map any more */
> -        dsb   sy
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>          /* Add UART to the fixmap table */
>          ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
> @@ -653,19 +718,10 @@ setup_fixmap:
>          ldr   x1, =FIXMAP_ADDR(0)
>          lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
>          str   x2, [x4, x1]           /* Map it in the fixmap's slot */
> -#endif
>  
> -        /*
> -         * Flush the TLB in case the 1:1 mapping happens to clash with
> -         * the virtual addresses used by the fixmap or DTB.
> -         */
> -        dsb   sy                     /* Ensure any page table updates made above
> -                                      * have occurred. */
> -
> -        isb
> -        tlbi  alle2
> -        dsb   sy                     /* Ensure completion of TLB flush */
> -        isb
> +        /* Ensure any page table updates made above have occurred */
> +        dsb   nshst
> +#endif
>          ret
>  ENDPROC(setup_fixmap)
>  
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

Hi Stefano,

On 6/27/19 7:55 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> +1:
>> +        /*
>> +         * Find the second slot used. Remove the entry for the first
>> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
>> +         * For slot 1, it means the ID map was not created.
>> +         */
>> +        lsr   x1, x19, #SECOND_SHIFT
>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>> +        cmp   x1, #1
>> +        beq   id_map_removed
>> +        /* It is not in slot 1, remove the entry */
>> +        ldr   x0, =boot_second          /* x0 := second table */
>> +        str   xzr, [x0, x1, lsl #3]
> 
> Wouldn't it be a bit more reliable if we checked whether the slot in
> question for x19 (whether zero, first, second) is a pagetable pointer or
> section map, then zero it if it is a section map, otherwise go down one
> level? If we did it this way it would be independent from the way
> create_page_tables is written.

Your suggestion will not comply with the architecture compliance and how 
Xen is/will be working after the full rework. We want to remove 
everything (mapping + table) added specifically for the 1:1 mapping.

Otherwise, you may end up in a position where boot_first_id is still in 
place. We would need to use the break-before-make sequence in subsequent 
code if we were about to insert 1GB mapping at the same place.

After my rework, we would have virtually no place where 
break-before-make will be necessary as it will enforce all the mappings 
to be destroyed before hand. So I would rather avoid to make a specific 
case for the 1:1 mapping.

As a side note, the current code for the 1:1 mapping is completely wrong 
as using 1GB (or even 2MB) mapping may result to map MMIO region (or 
reserved-region). This may result to cache problem. I have this 
partially fixed on for the next version of series (see [1]).

> 
> With the current code, we are somewhat reliant on the behavior of
> create_page_tables, because we rely on the position of the slot for
> the ID map? Where the assumption for instance is that at level one, if
> the slot is zero, then we need to go down a level, etc. Instead, if we
> checked if the slot is a section map, we could remove it immediately, if
> it is a pagetable pointer, we proceed. The code should be similar in
> complexity and LOC, but it would be more robust.

See above :).

> 
> Something like the following, in pseudo-uncompiled assembly:
> 
>       lsr   x1, x19, #FIRST_SHIFT
>       ldr   x0, =boot_first           /* x0 := first table */
>       ldr   x2, [x0, x1, lsl #3]
>       # check x2 against #PT_MEM
>       cbz   x2, 1f
>       str   xzr, [x0, x1, lsl #3]
>       b     id_map_removed
> 
> 
>> +id_map_removed:
>> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the sequence. */
> 
> Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the
> explanation you are referring to.

The big comment at the top of the header:

/*
  * Every invalidation operation use the following patterns:
  *
  * DSB ISHST        // Ensure prior page-tables updates have completed
  * TLBI...          // Invalidate the TLB
  * DSB ISH          // Ensure the TLB invalidation has completed
  * ISB              // See explanation below
  *
  * For Xen page-tables the ISB will discard any instructions fetched
  * from the old mappings.
  *
  * For the Stage-2 page-tables the ISB ensures the completion of the DSB
  * (and therefore the TLB invalidation) before continuing. So we know
  * the TLBs cannot contain an entry for a mapping we may have removed.
  */

Note that we are using nsh (and not ish) because we are using local TLB 
flush (see page D5-230 ARM DDI 0487D.a). For convenience here is the text:

"In all cases in this section where a DMB or DSB is referred to, it 
refers to a DMB or DSB whose required access type is
both loads and stores. A DSB NSH is sufficient to ensure completion of 
TLB maintenance instructions that apply to a
single PE. A DSB ISH is sufficient to ensure completion of TLB 
maintenance instructions that apply to PEs in the
same Inner Shareable domain."

I discovered this section after the changes in flushtlb.h has been 
merged. But I am thinking to do a follow-up the local TLB flush code.

> 
> 
>> +        dsb   nshst
>> +        tlbi  alle2
>> +        dsb   nsh
>> +        isb
>> +
>> +        ret
>> +ENDPROC(remove_id_map)

[...]

[1] Rework for create_page_tables

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index a79ae54822..c019dd3e04 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -483,6 +483,60 @@ cpu_init:
  ENDPROC(cpu_init)

  /*
+ * Macro to create a page table entry in \ptbl to \tbl
+ *
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     table symbol to point to
+ * virt:    virtual address
+ * shift:   #imm page table shift
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ *
+ * Preserves \virt
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Also use x20 for the phys offset.
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
+        lsr   \tmp1, \virt, #\shift
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+        load_paddr \tmp2, \tbl
+        mov   \tmp3, #PT_PT                 /* \tmp3 := right for 
linear PT */
+        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
+        adr_l \tmp2, \ptbl
+        str   \tmp3, [\tmp2, \tmp1, lsl #3]
+.endm
+
+/*
+ * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
+ * level table is supported.
+ *
+ * tbl:     table symbol where the entry will be created
+ * virt:    virtual address
+ * paddr:   physical address (should be page aligned)
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ * tmp3:    scratch register
+ * type:    mapping type. If not specified it will be normal memory 
(PT_MEM_L3)
+ *
+ * Preserves \virt, \paddr
+ * Clobbers \tmp1, \tmp2, \tmp3
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_mapping_entry, tbl, virt, paddr, tmp1, tmp2, tmp3, 
type=PT_MEM_L3
+        lsr   \tmp1, \virt, #THIRD_SHIFT
+        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+        mov   \tmp2, #\type                 /* \tmp2 := right for 
section PT */
+        orr   \tmp2, \tmp2, \paddr          /*          + paddr */
+        adr_l \tmp3, \tbl
+        str   \tmp2, [\tmp3, \tmp1, lsl #3]
+.endm
+
+/*
   * Rebuild the boot pagetable's first-level entries. The structure
   * is described in mm.c.
   *
@@ -495,100 +549,17 @@ ENDPROC(cpu_init)
   *   x19: paddr(start)
   *   x20: phys offset
   *
- * Clobbers x0 - x4, x25
- *
- * Register usage within this function:
- *   x25: Identity map in place
+ * Clobbers x0 - x4
   */
  create_page_tables:
-        /*
-         * If Xen is loaded at exactly XEN_VIRT_START then we don't
-         * need an additional 1:1 mapping, the virtual mapping will
-         * suffice.
-         */
-        cmp   x19, #XEN_VIRT_START
-        cset  x25, eq                /* x25 := identity map in place, 
or not */
-
-        load_paddr x4, boot_pgtable
-
-        /* Setup boot_pgtable: */
-        load_paddr x1, boot_first
-
-        /* ... map boot_first in boot_pgtable[0] */
-        mov   x3, #PT_PT             /* x2 := table map of boot_first */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
-        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in 
boot_pgtable */
-        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
-         * to use an extra first level page in which we create a 1GB 
mapping.
-         */
-        load_paddr x2, boot_first_id
-
-        mov   x3, #PT_PT             /* x2 := table map of boot_first_id */
-        orr   x2, x2, x3             /*       + rights for linear PT */
-        str   x2, [x4, x1, lsl #3]
-
-        load_paddr x4, boot_first_id
-
-        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in 
boot_first_id */
-        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB 
mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        and   x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */
-        str   x2, [x4, x1, lsl #3]   /* Mapping of paddr(start) */
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_first: */
-        load_paddr x4, boot_first   /* Next level into boot_first */
-
-        /* ... map boot_second in boot_first[0] */
-        load_paddr x1, boot_second
-        mov   x3, #PT_PT             /* x2 := table map of boot_second */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #0]           /* Map it in slot 0 */
-
-        /* ... map of paddr(start) in boot_first */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in 
boot_first */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cbz   x1, 1f                 /* It's in slot 0, map in 
boot_second */
-
-        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_second: */
-        load_paddr x4, boot_second
-
-        /* ... map boot_third in boot_second[1] */
-        load_paddr x1, boot_third
-        mov   x3, #PT_PT             /* x2 := table map of boot_third */
-        orr   x2, x1, x3             /*       + rights for linear PT */
-        str   x2, [x4, #8]           /* Map it in slot 1 */
-
-        /* ... map of paddr(start) in boot_second */
-        cbnz  x25, 1f                /* x25 is set if already created */
-        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in 
boot_second */
-        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
-        cmp   x1, #1
-        b.eq  virtphys_clash         /* It's in slot 1, which we cannot 
handle */
+        /* Prepare the page-tables for mapping Xen */
+        ldr   x0, =XEN_VIRT_START
+        create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, 
x1, x2, x3
+        create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, 
x1, x2, x3
+        create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, 
x1, x2, x3

-        lsl   x2, x2, #SECOND_SHIFT  /* Base address for 2MB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section map */
-        orr   x2, x2, x3
-        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
-        mov   x25, #1                /* x25 := identity map now in place */
-
-1:      /* Setup boot_third: */
-        load_paddr x4, boot_third
+        /* Map Xen */
+        adr_l x4, boot_third

          lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
          lsl   x2, x2, #THIRD_SHIFT
@@ -603,21 +574,68 @@ create_page_tables:
          cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
          b.lt  1b

-        /* Defer fixmap and dtb mapping until after paging enabled, to
-         * avoid them clashing with the 1:1 mapping. */
+        /*
+         * If Xen is loaded at exactly XEN_VIRT_START then we don't
+         * need an additional 1:1 mapping, the virtual mapping will
+         * suffice.
+         */
+        cmp   x19, #XEN_VIRT_START
+        bne   1f
+        ret
+1:
+        /*
+         * Only the first page of Xen will be part of the 1:1 mapping.
+         * All the boot_*_id tables are linked together even if they may
+         * not be all used. They will then be linked to the boot page
+         * tables at the correct level.
+         */
+        create_table_entry boot_first_id, boot_second_id, x19, 
FIRST_SHIFT, x0, x1, x2
+        create_table_entry boot_second_id, boot_third_id, x19, 
SECOND_SHIFT, x0, x1, x2
+        create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
+
+        /*
+         * Find the zeroeth slot used. Link boot_first_id into
+         * boot_pgtable if the slot is not 0. For slot 0, the tables
+         * associated with the 1:1 mapping will need to be linked in
+         * boot_first or boot_second.
+         */
+        lsr   x0, x19, #ZEROETH_SHIFT   /* x0 := zeroeth slot */
+        cbz   x0, 1f
+        /* It is not in slot 0, Link boot_first_id into boot_pgtable */
+        create_table_entry boot_pgtable, boot_first_id, x19, 
ZEROETH_SHIFT, x0, x1, x2
+        ret
+
+1:
+        /*
+         * Find the first slot used. Link boot_second_id into boot_first
+         * if the slot is not 0. For slot 0, the tables associated with
+         * the 1:1 mapping will need to be linked in boot_second.
+         */
+        lsr   x0, x19, #FIRST_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cbz   x0, 1f
+        /* It is not in slot 0, Link boot_second_id into boot_first */
+        create_table_entry boot_first, boot_second_id, x19, 
FIRST_SHIFT, x0, x1, x2
+        ret

-        /* boot pagetable setup complete */
+1:
+        /*
+         * Find the second slot used. Link boot_third_id into boot_second
+         * if the slot is not 1 (runtime Xen mapping is 2M - 4M).
+         * For slot 1, Xen is not yet able to handle it.
+         */
+        lsr   x0, x19, #SECOND_SHIFT
+        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
+        cmp   x0, #1
+        beq   virtphys_clash
+        /* It is not in slot 1, link boot_third_id into boot_second */
+        create_table_entry boot_second, boot_third_id, x19, 
SECOND_SHIFT, x0, x1, x2
+        ret

-        cbnz  x25, 1f                /* Did we manage to create an 
identity mapping ? */
-        PRINT("Unable to build boot page tables - Failed to identity 
map Xen.\r\n")
-        b     fail
  virtphys_clash:
          /* Identity map clashes with boot_third, which we cannot 
handle yet */
          PRINT("- Unable to build boot page tables - virt and phys 
addresses clash. -\r\n")
          b     fail
-
-1:
-        ret
  ENDPROC(create_page_tables)

  /*
@@ -719,28 +737,15 @@ ENDPROC(remove_identity_mapping)
   * The fixmap cannot be mapped in create_page_tables because it may
   * clash with the 1:1 mapping.
   *
- * Clobbers x1 - x4
+ * Clobbers x0 - x3
   */
  setup_fixmap:
  #ifdef CONFIG_EARLY_PRINTK
-        /* Add UART to the fixmap table */
-        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
-        lsr   x2, x23, #THIRD_SHIFT
-        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
-        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 */
+        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
+        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, 
type=PT_DEV_L3
  #endif
-
-        /* Map fixmap into boot_second */
-        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
-        load_paddr x2, xen_fixmap
-        mov   x3, #PT_PT
-        orr   x2, x2, x3             /* x2 := table map of xen_fixmap */
-        ldr   x1, =FIXMAP_ADDR(0)
-        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
-        str   x2, [x4, x1]           /* Map it in the fixmap's slot */
-
+        ldr   x0, =FIXMAP_ADDR(0)
+        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, 
x1, x2, x3
          /* Ensure any page table updates made above have occurred */
          dsb   nshst
          ret
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c2f1795a71..bc1824d3ca 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -107,6 +107,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
  DEFINE_BOOT_PAGE_TABLE(boot_first);
  DEFINE_BOOT_PAGE_TABLE(boot_first_id);
  #endif
+DEFINE_BOOT_PAGE_TABLE(boot_second_id);
+DEFINE_BOOT_PAGE_TABLE(boot_third_id);
  DEFINE_BOOT_PAGE_TABLE(boot_second);
  DEFINE_BOOT_PAGE_TABLE(boot_third);


-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

Hi,

@Stefano, I am going through the series and noticed you didn't give any 
update. Could you confirm if my reply makes sense?

Cheers,

On 6/27/19 8:30 PM, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/27/19 7:55 PM, Stefano Stabellini wrote:
>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>> +1:
>>> +        /*
>>> +         * Find the second slot used. Remove the entry for the first
>>> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
>>> +         * For slot 1, it means the ID map was not created.
>>> +         */
>>> +        lsr   x1, x19, #SECOND_SHIFT
>>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>>> +        cmp   x1, #1
>>> +        beq   id_map_removed
>>> +        /* It is not in slot 1, remove the entry */
>>> +        ldr   x0, =boot_second          /* x0 := second table */
>>> +        str   xzr, [x0, x1, lsl #3]
>>
>> Wouldn't it be a bit more reliable if we checked whether the slot in
>> question for x19 (whether zero, first, second) is a pagetable pointer or
>> section map, then zero it if it is a section map, otherwise go down one
>> level? If we did it this way it would be independent from the way
>> create_page_tables is written.
> 
> Your suggestion will not comply with the architecture compliance and how 
> Xen is/will be working after the full rework. We want to remove 
> everything (mapping + table) added specifically for the 1:1 mapping.
> 
> Otherwise, you may end up in a position where boot_first_id is still in 
> place. We would need to use the break-before-make sequence in subsequent 
> code if we were about to insert 1GB mapping at the same place.
> 
> After my rework, we would have virtually no place where 
> break-before-make will be necessary as it will enforce all the mappings 
> to be destroyed before hand. So I would rather avoid to make a specific 
> case for the 1:1 mapping.
> 
> As a side note, the current code for the 1:1 mapping is completely wrong 
> as using 1GB (or even 2MB) mapping may result to map MMIO region (or 
> reserved-region). This may result to cache problem. I have this 
> partially fixed on for the next version of series (see [1]).
> 
>>
>> With the current code, we are somewhat reliant on the behavior of
>> create_page_tables, because we rely on the position of the slot for
>> the ID map? Where the assumption for instance is that at level one, if
>> the slot is zero, then we need to go down a level, etc. Instead, if we
>> checked if the slot is a section map, we could remove it immediately, if
>> it is a pagetable pointer, we proceed. The code should be similar in
>> complexity and LOC, but it would be more robust.
> 
> See above :).
> 
>>
>> Something like the following, in pseudo-uncompiled assembly:
>>
>>       lsr   x1, x19, #FIRST_SHIFT
>>       ldr   x0, =boot_first           /* x0 := first table */
>>       ldr   x2, [x0, x1, lsl #3]
>>       # check x2 against #PT_MEM
>>       cbz   x2, 1f
>>       str   xzr, [x0, x1, lsl #3]
>>       b     id_map_removed
>>
>>
>>> +id_map_removed:
>>> +        /* See asm-arm/arm64/flushtlb.h for the explanation of the 
>>> sequence. */
>>
>> Do you mean xen/include/asm-arm/arm64/flushtlb.h? I can't find the
>> explanation you are referring to.
> 
> The big comment at the top of the header:
> 
> /*
>   * Every invalidation operation use the following patterns:
>   *
>   * DSB ISHST        // Ensure prior page-tables updates have completed
>   * TLBI...          // Invalidate the TLB
>   * DSB ISH          // Ensure the TLB invalidation has completed
>   * ISB              // See explanation below
>   *
>   * For Xen page-tables the ISB will discard any instructions fetched
>   * from the old mappings.
>   *
>   * For the Stage-2 page-tables the ISB ensures the completion of the DSB
>   * (and therefore the TLB invalidation) before continuing. So we know
>   * the TLBs cannot contain an entry for a mapping we may have removed.
>   */
> 
> Note that we are using nsh (and not ish) because we are using local TLB 
> flush (see page D5-230 ARM DDI 0487D.a). For convenience here is the text:
> 
> "In all cases in this section where a DMB or DSB is referred to, it 
> refers to a DMB or DSB whose required access type is
> both loads and stores. A DSB NSH is sufficient to ensure completion of 
> TLB maintenance instructions that apply to a
> single PE. A DSB ISH is sufficient to ensure completion of TLB 
> maintenance instructions that apply to PEs in the
> same Inner Shareable domain."
> 
> I discovered this section after the changes in flushtlb.h has been 
> merged. But I am thinking to do a follow-up the local TLB flush code.
> 
>>
>>
>>> +        dsb   nshst
>>> +        tlbi  alle2
>>> +        dsb   nsh
>>> +        isb
>>> +
>>> +        ret
>>> +ENDPROC(remove_id_map)
> 
> [...]
> 
> [1] Rework for create_page_tables
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index a79ae54822..c019dd3e04 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -483,6 +483,60 @@ cpu_init:
>   ENDPROC(cpu_init)
> 
>   /*
> + * Macro to create a page table entry in \ptbl to \tbl
> + *
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     table symbol to point to
> + * virt:    virtual address
> + * shift:   #imm page table shift
> + * tmp1:    scratch register
> + * tmp2:    scratch register
> + * tmp3:    scratch register
> + *
> + * Preserves \virt
> + * Clobbers \tmp1, \tmp2, \tmp3
> + *
> + * Also use x20 for the phys offset.
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro create_table_entry, ptbl, tbl, virt, shift, tmp1, tmp2, tmp3
> +        lsr   \tmp1, \virt, #\shift
> +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +        load_paddr \tmp2, \tbl
> +        mov   \tmp3, #PT_PT                 /* \tmp3 := right for 
> linear PT */
> +        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
> +        adr_l \tmp2, \ptbl
> +        str   \tmp3, [\tmp2, \tmp1, lsl #3]
> +.endm
> +
> +/*
> + * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
> + * level table is supported.
> + *
> + * tbl:     table symbol where the entry will be created
> + * virt:    virtual address
> + * paddr:   physical address (should be page aligned)
> + * tmp1:    scratch register
> + * tmp2:    scratch register
> + * tmp3:    scratch register
> + * type:    mapping type. If not specified it will be normal memory 
> (PT_MEM_L3)
> + *
> + * Preserves \virt, \paddr
> + * Clobbers \tmp1, \tmp2, \tmp3
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro create_mapping_entry, tbl, virt, paddr, tmp1, tmp2, tmp3, 
> type=PT_MEM_L3
> +        lsr   \tmp1, \virt, #THIRD_SHIFT
> +        and   \tmp1, \tmp1, #LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +        mov   \tmp2, #\type                 /* \tmp2 := right for 
> section PT */
> +        orr   \tmp2, \tmp2, \paddr          /*          + paddr */
> +        adr_l \tmp3, \tbl
> +        str   \tmp2, [\tmp3, \tmp1, lsl #3]
> +.endm
> +
> +/*
>    * Rebuild the boot pagetable's first-level entries. The structure
>    * is described in mm.c.
>    *
> @@ -495,100 +549,17 @@ ENDPROC(cpu_init)
>    *   x19: paddr(start)
>    *   x20: phys offset
>    *
> - * Clobbers x0 - x4, x25
> - *
> - * Register usage within this function:
> - *   x25: Identity map in place
> + * Clobbers x0 - x4
>    */
>   create_page_tables:
> -        /*
> -         * If Xen is loaded at exactly XEN_VIRT_START then we don't
> -         * need an additional 1:1 mapping, the virtual mapping will
> -         * suffice.
> -         */
> -        cmp   x19, #XEN_VIRT_START
> -        cset  x25, eq                /* x25 := identity map in place, 
> or not */
> -
> -        load_paddr x4, boot_pgtable
> -
> -        /* Setup boot_pgtable: */
> -        load_paddr x1, boot_first
> -
> -        /* ... map boot_first in boot_pgtable[0] */
> -        mov   x3, #PT_PT             /* x2 := table map of boot_first */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #0]           /* Map it in slot 0 */
> -
> -        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
> -        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in 
> boot_pgtable */
> -        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
> -         * to use an extra first level page in which we create a 1GB 
> mapping.
> -         */
> -        load_paddr x2, boot_first_id
> -
> -        mov   x3, #PT_PT             /* x2 := table map of 
> boot_first_id */
> -        orr   x2, x2, x3             /*       + rights for linear PT */
> -        str   x2, [x4, x1, lsl #3]
> -
> -        load_paddr x4, boot_first_id
> -
> -        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in 
> boot_first_id */
> -        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB 
> mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        and   x1, x1, #LPAE_ENTRY_MASK /* x1 := Slot offset */
> -        str   x2, [x4, x1, lsl #3]   /* Mapping of paddr(start) */
> -        mov   x25, #1                /* x25 := identity map now in 
> place */
> -
> -1:      /* Setup boot_first: */
> -        load_paddr x4, boot_first   /* Next level into boot_first */
> -
> -        /* ... map boot_second in boot_first[0] */
> -        load_paddr x1, boot_second
> -        mov   x3, #PT_PT             /* x2 := table map of boot_second */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #0]           /* Map it in slot 0 */
> -
> -        /* ... map of paddr(start) in boot_first */
> -        cbnz  x25, 1f                /* x25 is set if already created */
> -        lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in 
> boot_first */
> -        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
> -        cbz   x1, 1f                 /* It's in slot 0, map in 
> boot_second */
> -
> -        lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
> -        mov   x25, #1                /* x25 := identity map now in 
> place */
> -
> -1:      /* Setup boot_second: */
> -        load_paddr x4, boot_second
> -
> -        /* ... map boot_third in boot_second[1] */
> -        load_paddr x1, boot_third
> -        mov   x3, #PT_PT             /* x2 := table map of boot_third */
> -        orr   x2, x1, x3             /*       + rights for linear PT */
> -        str   x2, [x4, #8]           /* Map it in slot 1 */
> -
> -        /* ... map of paddr(start) in boot_second */
> -        cbnz  x25, 1f                /* x25 is set if already created */
> -        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in 
> boot_second */
> -        and   x1, x2, #LPAE_ENTRY_MASK /* x1 := Slot to use */
> -        cmp   x1, #1
> -        b.eq  virtphys_clash         /* It's in slot 1, which we cannot 
> handle */
> +        /* Prepare the page-tables for mapping Xen */
> +        ldr   x0, =XEN_VIRT_START
> +        create_table_entry boot_pgtable, boot_first, x0, ZEROETH_SHIFT, 
> x1, x2, x3
> +        create_table_entry boot_first, boot_second, x0, FIRST_SHIFT, 
> x1, x2, x3
> +        create_table_entry boot_second, boot_third, x0, SECOND_SHIFT, 
> x1, x2, x3
> 
> -        lsl   x2, x2, #SECOND_SHIFT  /* Base address for 2MB mapping */
> -        mov   x3, #PT_MEM            /* x2 := Section map */
> -        orr   x2, x2, x3
> -        str   x2, [x4, x1, lsl #3]   /* Create mapping of paddr(start)*/
> -        mov   x25, #1                /* x25 := identity map now in 
> place */
> -
> -1:      /* Setup boot_third: */
> -        load_paddr x4, boot_third
> +        /* Map Xen */
> +        adr_l x4, boot_third
> 
>           lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
>           lsl   x2, x2, #THIRD_SHIFT
> @@ -603,21 +574,68 @@ create_page_tables:
>           cmp   x1, #(LPAE_ENTRIES<<3) /* 512 entries per page */
>           b.lt  1b
> 
> -        /* Defer fixmap and dtb mapping until after paging enabled, to
> -         * avoid them clashing with the 1:1 mapping. */
> +        /*
> +         * If Xen is loaded at exactly XEN_VIRT_START then we don't
> +         * need an additional 1:1 mapping, the virtual mapping will
> +         * suffice.
> +         */
> +        cmp   x19, #XEN_VIRT_START
> +        bne   1f
> +        ret
> +1:
> +        /*
> +         * Only the first page of Xen will be part of the 1:1 mapping.
> +         * All the boot_*_id tables are linked together even if they may
> +         * not be all used. They will then be linked to the boot page
> +         * tables at the correct level.
> +         */
> +        create_table_entry boot_first_id, boot_second_id, x19, 
> FIRST_SHIFT, x0, x1, x2
> +        create_table_entry boot_second_id, boot_third_id, x19, 
> SECOND_SHIFT, x0, x1, x2
> +        create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
> +
> +        /*
> +         * Find the zeroeth slot used. Link boot_first_id into
> +         * boot_pgtable if the slot is not 0. For slot 0, the tables
> +         * associated with the 1:1 mapping will need to be linked in
> +         * boot_first or boot_second.
> +         */
> +        lsr   x0, x19, #ZEROETH_SHIFT   /* x0 := zeroeth slot */
> +        cbz   x0, 1f
> +        /* It is not in slot 0, Link boot_first_id into boot_pgtable */
> +        create_table_entry boot_pgtable, boot_first_id, x19, 
> ZEROETH_SHIFT, x0, x1, x2
> +        ret
> +
> +1:
> +        /*
> +         * Find the first slot used. Link boot_second_id into boot_first
> +         * if the slot is not 0. For slot 0, the tables associated with
> +         * the 1:1 mapping will need to be linked in boot_second.
> +         */
> +        lsr   x0, x19, #FIRST_SHIFT
> +        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
> +        cbz   x0, 1f
> +        /* It is not in slot 0, Link boot_second_id into boot_first */
> +        create_table_entry boot_first, boot_second_id, x19, 
> FIRST_SHIFT, x0, x1, x2
> +        ret
> 
> -        /* boot pagetable setup complete */
> +1:
> +        /*
> +         * Find the second slot used. Link boot_third_id into boot_second
> +         * if the slot is not 1 (runtime Xen mapping is 2M - 4M).
> +         * For slot 1, Xen is not yet able to handle it.
> +         */
> +        lsr   x0, x19, #SECOND_SHIFT
> +        and   x0, x0, #LPAE_ENTRY_MASK  /* x0 := first slot */
> +        cmp   x0, #1
> +        beq   virtphys_clash
> +        /* It is not in slot 1, link boot_third_id into boot_second */
> +        create_table_entry boot_second, boot_third_id, x19, 
> SECOND_SHIFT, x0, x1, x2
> +        ret
> 
> -        cbnz  x25, 1f                /* Did we manage to create an 
> identity mapping ? */
> -        PRINT("Unable to build boot page tables - Failed to identity 
> map Xen.\r\n")
> -        b     fail
>   virtphys_clash:
>           /* Identity map clashes with boot_third, which we cannot 
> handle yet */
>           PRINT("- Unable to build boot page tables - virt and phys 
> addresses clash. -\r\n")
>           b     fail
> -
> -1:
> -        ret
>   ENDPROC(create_page_tables)
> 
>   /*
> @@ -719,28 +737,15 @@ ENDPROC(remove_identity_mapping)
>    * The fixmap cannot be mapped in create_page_tables because it may
>    * clash with the 1:1 mapping.
>    *
> - * Clobbers x1 - x4
> + * Clobbers x0 - x3
>    */
>   setup_fixmap:
>   #ifdef CONFIG_EARLY_PRINTK
> -        /* Add UART to the fixmap table */
> -        ldr   x1, =xen_fixmap        /* x1 := vaddr (xen_fixmap) */
> -        lsr   x2, x23, #THIRD_SHIFT
> -        lsl   x2, x2, #THIRD_SHIFT   /* 4K aligned paddr of UART */
> -        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 */
> +        ldr   x0, =EARLY_UART_VIRTUAL_ADDRESS
> +        create_mapping_entry xen_fixmap, x0, x23, x1, x2, x3, 
> type=PT_DEV_L3
>   #endif
> -
> -        /* Map fixmap into boot_second */
> -        ldr   x4, =boot_second       /* x4 := vaddr (boot_second) */
> -        load_paddr x2, xen_fixmap
> -        mov   x3, #PT_PT
> -        orr   x2, x2, x3             /* x2 := table map of xen_fixmap */
> -        ldr   x1, =FIXMAP_ADDR(0)
> -        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x1 := Slot for FIXMAP(0) */
> -        str   x2, [x4, x1]           /* Map it in the fixmap's slot */
> -
> +        ldr   x0, =FIXMAP_ADDR(0)
> +        create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, 
> x1, x2, x3
>           /* Ensure any page table updates made above have occurred */
>           dsb   nshst
>           ret
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index c2f1795a71..bc1824d3ca 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -107,6 +107,8 @@ DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
>   DEFINE_BOOT_PAGE_TABLE(boot_first);
>   DEFINE_BOOT_PAGE_TABLE(boot_first_id);
>   #endif
> +DEFINE_BOOT_PAGE_TABLE(boot_second_id);
> +DEFINE_BOOT_PAGE_TABLE(boot_third_id);
>   DEFINE_BOOT_PAGE_TABLE(boot_second);
>   DEFINE_BOOT_PAGE_TABLE(boot_third);
> 
> 

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini]

On Thu, 27 Jun 2019, Julien Grall wrote:
> On 6/27/19 7:55 PM, Stefano Stabellini wrote:
> > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > +1:
> > > +        /*
> > > +         * Find the second slot used. Remove the entry for the first
> > > +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
> > > +         * For slot 1, it means the ID map was not created.
> > > +         */
> > > +        lsr   x1, x19, #SECOND_SHIFT
> > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > +        cmp   x1, #1
> > > +        beq   id_map_removed
> > > +        /* It is not in slot 1, remove the entry */
> > > +        ldr   x0, =boot_second          /* x0 := second table */
> > > +        str   xzr, [x0, x1, lsl #3]
> > 
> > Wouldn't it be a bit more reliable if we checked whether the slot in
> > question for x19 (whether zero, first, second) is a pagetable pointer or
> > section map, then zero it if it is a section map, otherwise go down one
> > level? If we did it this way it would be independent from the way
> > create_page_tables is written.
> 
> Your suggestion will not comply with the architecture compliance and how Xen
> is/will be working after the full rework. We want to remove everything
> (mapping + table) added specifically for the 1:1 mapping.
> 
> Otherwise, you may end up in a position where boot_first_id is still in place.
> We would need to use the break-before-make sequence in subsequent code if we
> were about to insert 1GB mapping at the same place.
> 
> After my rework, we would have virtually no place where break-before-make will
> be necessary as it will enforce all the mappings to be destroyed before hand.
> So I would rather avoid to make a specific case for the 1:1 mapping.

I don't fully understand your explanation. I understand the final goal
of "removing everything (mapping + table) added specifically for the 1:1
mapping". I don't understand why my suggestion would be a hindrance
toward that goal, compared to what it is done in this patch.

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

Hi Stefano,

On 7/30/19 6:33 PM, Stefano Stabellini wrote:
> On Thu, 27 Jun 2019, Julien Grall wrote:
>> On 6/27/19 7:55 PM, Stefano Stabellini wrote:
>>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>> +1:
>>>> +        /*
>>>> +         * Find the second slot used. Remove the entry for the first
>>>> +         * table if the slot is not 1 (runtime Xen mapping is 2M - 4M).
>>>> +         * For slot 1, it means the ID map was not created.
>>>> +         */
>>>> +        lsr   x1, x19, #SECOND_SHIFT
>>>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>>>> +        cmp   x1, #1
>>>> +        beq   id_map_removed
>>>> +        /* It is not in slot 1, remove the entry */
>>>> +        ldr   x0, =boot_second          /* x0 := second table */
>>>> +        str   xzr, [x0, x1, lsl #3]
>>>
>>> Wouldn't it be a bit more reliable if we checked whether the slot in
>>> question for x19 (whether zero, first, second) is a pagetable pointer or
>>> section map, then zero it if it is a section map, otherwise go down one
>>> level? If we did it this way it would be independent from the way
>>> create_page_tables is written.
>>
>> Your suggestion will not comply with the architecture compliance and how Xen
>> is/will be working after the full rework. We want to remove everything
>> (mapping + table) added specifically for the 1:1 mapping.
>>
>> Otherwise, you may end up in a position where boot_first_id is still in place.
>> We would need to use the break-before-make sequence in subsequent code if we
>> were about to insert 1GB mapping at the same place.
>>
>> After my rework, we would have virtually no place where break-before-make will
>> be necessary as it will enforce all the mappings to be destroyed before hand.
>> So I would rather avoid to make a specific case for the 1:1 mapping.
> 
> I don't fully understand your explanation. I understand the final goal
> of "removing everything (mapping + table) added specifically for the 1:1
> mapping". I don't understand why my suggestion would be a hindrance
> toward that goal, compared to what it is done in this patch.

Because, AFAICT, your suggestion will only remove the mapping and not 
the tables (such as boot_first_id). This is different from this patch 
where both mapping and tables are removed.

So yes, my suggestion is not generic, but at least it does the job that 
is expected by this function. I.e removing anything that was 
specifically created for the identity mapping.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini]

On Tue, 30 Jul 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 7/30/19 6:33 PM, Stefano Stabellini wrote:
> > On Thu, 27 Jun 2019, Julien Grall wrote:
> > > On 6/27/19 7:55 PM, Stefano Stabellini wrote:
> > > > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > > > +1:
> > > > > +        /*
> > > > > +         * Find the second slot used. Remove the entry for the first
> > > > > +         * table if the slot is not 1 (runtime Xen mapping is 2M -
> > > > > 4M).
> > > > > +         * For slot 1, it means the ID map was not created.
> > > > > +         */
> > > > > +        lsr   x1, x19, #SECOND_SHIFT
> > > > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > > > +        cmp   x1, #1
> > > > > +        beq   id_map_removed
> > > > > +        /* It is not in slot 1, remove the entry */
> > > > > +        ldr   x0, =boot_second          /* x0 := second table */
> > > > > +        str   xzr, [x0, x1, lsl #3]
> > > > 
> > > > Wouldn't it be a bit more reliable if we checked whether the slot in
> > > > question for x19 (whether zero, first, second) is a pagetable pointer or
> > > > section map, then zero it if it is a section map, otherwise go down one
> > > > level? If we did it this way it would be independent from the way
> > > > create_page_tables is written.
> > > 
> > > Your suggestion will not comply with the architecture compliance and how
> > > Xen
> > > is/will be working after the full rework. We want to remove everything
> > > (mapping + table) added specifically for the 1:1 mapping.
> > > 
> > > Otherwise, you may end up in a position where boot_first_id is still in
> > > place.
> > > We would need to use the break-before-make sequence in subsequent code if
> > > we
> > > were about to insert 1GB mapping at the same place.
> > > 
> > > After my rework, we would have virtually no place where break-before-make
> > > will
> > > be necessary as it will enforce all the mappings to be destroyed before
> > > hand.
> > > So I would rather avoid to make a specific case for the 1:1 mapping.
> > 
> > I don't fully understand your explanation. I understand the final goal
> > of "removing everything (mapping + table) added specifically for the 1:1
> > mapping". I don't understand why my suggestion would be a hindrance
> > toward that goal, compared to what it is done in this patch.
> 
> Because, AFAICT, your suggestion will only remove the mapping and not the
> tables (such as boot_first_id). This is different from this patch where both
> mapping and tables are removed.
>
> So yes, my suggestion is not generic, but at least it does the job that is
> expected by this function. I.e removing anything that was specifically created
> for the identity mapping.

I understand your comment now, and of course I agree that both mapping
and tables need to be removed.

I am careful making suggestions for assembly coding because I don't
really want to suggest something that doesn't work, or even if it works
that it's worse than the original.

It should be possible to remove both the table and the mapping in a
generic way. Instead of hardcoding the assemply equivalent of "It is not
in slot 0, remove the entry", we could check whether the table offset
matches the table offset of the mapping that we want to preserve. That
way, "slot 0" would be calculate instead of hardcoded, and the code
would be pretty generic. What do you think? It should only be a small
addition.

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

Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Julien Grall]

Hi Stefano,

On 7/31/19 9:40 PM, Stefano Stabellini wrote:
> On Tue, 30 Jul 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 7/30/19 6:33 PM, Stefano Stabellini wrote:
>>> On Thu, 27 Jun 2019, Julien Grall wrote:
>>>> On 6/27/19 7:55 PM, Stefano Stabellini wrote:
>>>>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>>>> +1:
>>>>>> +        /*
>>>>>> +         * Find the second slot used. Remove the entry for the first
>>>>>> +         * table if the slot is not 1 (runtime Xen mapping is 2M -
>>>>>> 4M).
>>>>>> +         * For slot 1, it means the ID map was not created.
>>>>>> +         */
>>>>>> +        lsr   x1, x19, #SECOND_SHIFT
>>>>>> +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
>>>>>> +        cmp   x1, #1
>>>>>> +        beq   id_map_removed
>>>>>> +        /* It is not in slot 1, remove the entry */
>>>>>> +        ldr   x0, =boot_second          /* x0 := second table */
>>>>>> +        str   xzr, [x0, x1, lsl #3]
>>>>>
>>>>> Wouldn't it be a bit more reliable if we checked whether the slot in
>>>>> question for x19 (whether zero, first, second) is a pagetable pointer or
>>>>> section map, then zero it if it is a section map, otherwise go down one
>>>>> level? If we did it this way it would be independent from the way
>>>>> create_page_tables is written.
>>>>
>>>> Your suggestion will not comply with the architecture compliance and how
>>>> Xen
>>>> is/will be working after the full rework. We want to remove everything
>>>> (mapping + table) added specifically for the 1:1 mapping.
>>>>
>>>> Otherwise, you may end up in a position where boot_first_id is still in
>>>> place.
>>>> We would need to use the break-before-make sequence in subsequent code if
>>>> we
>>>> were about to insert 1GB mapping at the same place.
>>>>
>>>> After my rework, we would have virtually no place where break-before-make
>>>> will
>>>> be necessary as it will enforce all the mappings to be destroyed before
>>>> hand.
>>>> So I would rather avoid to make a specific case for the 1:1 mapping.
>>>
>>> I don't fully understand your explanation. I understand the final goal
>>> of "removing everything (mapping + table) added specifically for the 1:1
>>> mapping". I don't understand why my suggestion would be a hindrance
>>> toward that goal, compared to what it is done in this patch.
>>
>> Because, AFAICT, your suggestion will only remove the mapping and not the
>> tables (such as boot_first_id). This is different from this patch where both
>> mapping and tables are removed.
>>
>> So yes, my suggestion is not generic, but at least it does the job that is
>> expected by this function. I.e removing anything that was specifically created
>> for the identity mapping.
> 
> I understand your comment now, and of course I agree that both mapping
> and tables need to be removed.
> 
> I am careful making suggestions for assembly coding because I don't
> really want to suggest something that doesn't work, or even if it works
> that it's worse than the original.
> 
> It should be possible to remove both the table and the mapping in a
> generic way. Instead of hardcoding the assemply equivalent of "It is not
> in slot 0, remove the entry", we could check whether the table offset
> matches the table offset of the mapping that we want to preserve. That
> way, "slot 0" would be calculate instead of hardcoded, and the code
> would be pretty generic. What do you think? It should only be a small
> addition.

It should be feasible and may actually help the next step in my plan 
where I need to make Xen relocatable.

I will have a look for both the arm32 and arm64 code.

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Julien Grall]

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
+ */
 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


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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Stefano Stabellini]

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
> 

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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Julien Grall]

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
I

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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

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

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Stefano Stabellini]

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
> 

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

Re: [Xen-devel] [PATCH 15/17] xen/arm64: head: Rework and document setup_fixmap()

[Julien Grall]

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

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

[Xen-devel] [PATCH 16/17] xen/arm64: head: Rework and document launch()

[Julien Grall]

Boot CPU and secondary CPUs will use different entry point to C code. At
the moment, the decision on which entry to use is taken within launch().

In order to avoid a branch for the decision and make the code clearer,
launch() is reworked to take in parameters the entry point and its
arguments.

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 | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4f7fa6769f..130ab66d8e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -312,6 +312,11 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Setup the arguments for start_xen and jump to C world */
+        mov   x0, x20                /* x0 := phys_offset */
+        mov   x1, x21                /* x1 := paddr(FDT) */
+        ldr   x2, =start_xen
         b     launch
 ENDPROC(real_start)
 
@@ -374,6 +379,9 @@ secondary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        PRINT("- Ready -\r\n")
+        /* Jump to C world */
+        ldr   x2, =start_secondary
         b     launch
 ENDPROC(init_secondary)
 
@@ -734,23 +742,24 @@ setup_fixmap:
         ret
 ENDPROC(setup_fixmap)
 
+/*
+ * Setup the initial stack and jump to the C world
+ *
+ * Inputs:
+ *   x0 : Argument 0 of the C function to call
+ *   x1 : Argument 1 of the C function to call
+ *   x2 : C entry point
+ */
 launch:
-        PRINT("- Ready -\r\n")
-
-        ldr   x0, =init_data
-        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
-        ldr   x0, [x0]
-        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
-        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
-        mov   sp, x0
-
-        cbnz  x22, 1f
-
-        mov   x0, x20                /* Marshal args: - phys_offset */
-        mov   x1, x21                /*               - FDT */
-        b     start_xen              /* and disappear into the land of C */
-1:
-        b     start_secondary        /* (to the appropriate entry point) */
+        ldr   x4, =init_data
+        add   x4, x4, #INITINFO_stack /* Find the boot-time stack */
+        ldr   x4, [x4]
+        add   x4, x4, #STACK_SIZE    /* (which grows down from the top). */
+        sub   x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */
+        mov   sp, x4
+
+        /* Jump to C world */
+        br    x2
 ENDPROC(launch)
 
 /* Fail-stop */
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 16/17] xen/arm64: head: Rework and document launch()

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> Boot CPU and secondary CPUs will use different entry point to C code. At
> the moment, the decision on which entry to use is taken within launch().
> 
> In order to avoid a branch for the decision and make the code clearer,
> launch() is reworked to take in parameters the entry point and its
> arguments.
> 
> 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 | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 4f7fa6769f..130ab66d8e 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -312,6 +312,11 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Setup the arguments for start_xen and jump to C world */
> +        mov   x0, x20                /* x0 := phys_offset */
> +        mov   x1, x21                /* x1 := paddr(FDT) */
> +        ldr   x2, =start_xen
>          b     launch
>  ENDPROC(real_start)
>  
> @@ -374,6 +379,9 @@ secondary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        PRINT("- Ready -\r\n")
> +        /* Jump to C world */
> +        ldr   x2, =start_secondary
>          b     launch
>  ENDPROC(init_secondary)
>  
> @@ -734,23 +742,24 @@ setup_fixmap:
>          ret
>  ENDPROC(setup_fixmap)
>  
> +/*
> + * Setup the initial stack and jump to the C world
> + *
> + * Inputs:
> + *   x0 : Argument 0 of the C function to call
> + *   x1 : Argument 1 of the C function to call
> + *   x2 : C entry point

I know it is the last one before C-land, but we might as well add a
"Clobbers" section too, just in case. Here it clobbers x4 (or x3, see
below).


> + */
>  launch:
> -        PRINT("- Ready -\r\n")
> -
> -        ldr   x0, =init_data
> -        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
> -        ldr   x0, [x0]
> -        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
> -        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
> -        mov   sp, x0
> -
> -        cbnz  x22, 1f
> -
> -        mov   x0, x20                /* Marshal args: - phys_offset */
> -        mov   x1, x21                /*               - FDT */
> -        b     start_xen              /* and disappear into the land of C */
> -1:
> -        b     start_secondary        /* (to the appropriate entry point) */
> +        ldr   x4, =init_data

why not use x3 instead of x4?


> +        add   x4, x4, #INITINFO_stack /* Find the boot-time stack */
> +        ldr   x4, [x4]
> +        add   x4, x4, #STACK_SIZE    /* (which grows down from the top). */

If you are going to respin it, could you please align the comments a bit
better (one space to the right)?


> +        sub   x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */
> +        mov   sp, x4
> +
> +        /* Jump to C world */
> +        br    x2
>  ENDPROC(launch)
>  
>  /* Fail-stop */

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

Re: [Xen-devel] [PATCH 16/17] xen/arm64: head: Rework and document launch()

[Julien Grall]

Hi Stefano,

On 6/26/19 8:12 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> Boot CPU and secondary CPUs will use different entry point to C code. At
>> the moment, the decision on which entry to use is taken within launch().
>>
>> In order to avoid a branch for the decision and make the code clearer,
>> launch() is reworked to take in parameters the entry point and its
>> arguments.
>>
>> 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 | 41 +++++++++++++++++++++++++----------------
>>   1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 4f7fa6769f..130ab66d8e 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -312,6 +312,11 @@ primary_switched:
>>           /* Use a virtual address to access the UART. */
>>           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Setup the arguments for start_xen and jump to C world */
>> +        mov   x0, x20                /* x0 := phys_offset */
>> +        mov   x1, x21                /* x1 := paddr(FDT) */
>> +        ldr   x2, =start_xen
>>           b     launch
>>   ENDPROC(real_start)
>>   
>> @@ -374,6 +379,9 @@ secondary_switched:
>>           /* Use a virtual address to access the UART. */
>>           ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>>   #endif
>> +        PRINT("- Ready -\r\n")
>> +        /* Jump to C world */
>> +        ldr   x2, =start_secondary
>>           b     launch
>>   ENDPROC(init_secondary)
>>   
>> @@ -734,23 +742,24 @@ setup_fixmap:
>>           ret
>>   ENDPROC(setup_fixmap)
>>   
>> +/*
>> + * Setup the initial stack and jump to the C world
>> + *
>> + * Inputs:
>> + *   x0 : Argument 0 of the C function to call
>> + *   x1 : Argument 1 of the C function to call
>> + *   x2 : C entry point
> 
> I know it is the last one before C-land, but we might as well add a
> "Clobbers" section too, just in case. Here it clobbers x4 (or x3, see
> below).

Sure.

> 
> 
>> + */
>>   launch:
>> -        PRINT("- Ready -\r\n")
>> -
>> -        ldr   x0, =init_data
>> -        add   x0, x0, #INITINFO_stack /* Find the boot-time stack */
>> -        ldr   x0, [x0]
>> -        add   x0, x0, #STACK_SIZE    /* (which grows down from the top). */
>> -        sub   x0, x0, #CPUINFO_sizeof /* Make room for CPU save record */
>> -        mov   sp, x0
>> -
>> -        cbnz  x22, 1f
>> -
>> -        mov   x0, x20                /* Marshal args: - phys_offset */
>> -        mov   x1, x21                /*               - FDT */
>> -        b     start_xen              /* and disappear into the land of C */
>> -1:
>> -        b     start_secondary        /* (to the appropriate entry point) */
>> +        ldr   x4, =init_data
> 
> why not use x3 instead of x4?

I think a remnant of early rework when start_secondary was taking 3 
parameters. I will update it.

> 
> 
>> +        add   x4, x4, #INITINFO_stack /* Find the boot-time stack */
>> +        ldr   x4, [x4]
>> +        add   x4, x4, #STACK_SIZE    /* (which grows down from the top). */
> 
> If you are going to respin it, could you please align the comments a bit
> better (one space to the right)?

Sure.

> 
> 
>> +        sub   x4, x4, #CPUINFO_sizeof /* Make room for CPU save record */
>> +        mov   sp, x4
>> +
>> +        /* Jump to C world */
>> +        br    x2
>>   ENDPROC(launch)
>>   
>>   /* Fail-stop */

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH 17/17] xen/arm64: Zero BSS after the MMU and D-cache is turned on

[Julien Grall]

At the moment BSS is zeroed before the MMU and D-Cache is turned on.
In other words, the cache will be bypassed when zeroing the BSS section.

Per the Image protocol [1], the state of the cache for BSS region is not
known because it is not part of the "loaded kernel image".

This means that the cache will need to be invalidated twice for the BSS
region:
    1) Before zeroing to remove any dirty cache line. Otherwise they may
    get evicted while zeroing and therefore overriding the value.
    2) After zeroing to remove any cache line that may have been
    speculated. Otherwise when turning on MMU and D-Cache, the CPU may
    see old values.

However, the only reason to have the BSS zeroed early is because the
boot page tables are part of BSS. To avoid the two cache invalidations,
it is possible to move the page tables in the section .data.page_aligned.

A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
page-tables used before BSS is zeroed. This includes all boot_* but also
xen_fixmap as zero_bss() will print a message when earlyprintk is
enabled.

[1] linux/Documentation/arm64/booting.txt

---

    Note that the arm32 support is not there yet. This will need to be
    addressed here or separately depending on when the Arm32 boot rework
    is sent.
---
 xen/arch/arm/arm64/head.S |  6 +++---
 xen/arch/arm/mm.c         | 23 +++++++++++++++++------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 130ab66d8e..6c3edbbc81 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -291,7 +291,6 @@ real_start_efi:
         mov   x22, #0                /* x22 := is_secondary_cpu */
 
         bl    check_cpu_mode
-        bl    zero_bss
         bl    cpu_init
         bl    create_page_tables
         bl    enable_mmu
@@ -312,6 +311,7 @@ primary_switched:
         /* Use a virtual address to access the UART. */
         ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
 #endif
+        bl    zero_bss
         PRINT("- Ready -\r\n")
         /* Setup the arguments for start_xen and jump to C world */
         mov   x0, x20                /* x0 := phys_offset */
@@ -423,8 +423,8 @@ zero_bss:
         cbnz  x26, skip_bss
 
         PRINT("- Zero BSS -\r\n")
-        load_paddr x0, __bss_start    /* Load paddr of start & end of bss */
-        load_paddr x1, __bss_end
+        ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
+        ldr   x1, =__bss_end         /* x1 := vaddr(__bss_start) */
 
 1:      str   xzr, [x0], #8
         cmp   x0, x1
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6a549e9283..0b2d07a258 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -48,6 +48,17 @@
 #undef mfn_to_virt
 #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
 
+/*
+ * Macros to define page-tables:
+ *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
+ *  in assembly code before BSS is zeroed.
+ *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
+ *  page-tables to be used after BSS is zeroed (typically they are only used
+ *  in C).
+ */
+#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
+lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned") name[LPAE_ENTRIES]
+
 #define DEFINE_PAGE_TABLES(name, nr)                    \
 lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
 
@@ -76,13 +87,13 @@ lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
  * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
  * by the CPU once it has moved off the 1:1 mapping.
  */
-DEFINE_PAGE_TABLE(boot_pgtable);
+DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
 #ifdef CONFIG_ARM_64
-DEFINE_PAGE_TABLE(boot_first);
-DEFINE_PAGE_TABLE(boot_first_id);
+DEFINE_BOOT_PAGE_TABLE(boot_first);
+DEFINE_BOOT_PAGE_TABLE(boot_first_id);
 #endif
-DEFINE_PAGE_TABLE(boot_second);
-DEFINE_PAGE_TABLE(boot_third);
+DEFINE_BOOT_PAGE_TABLE(boot_second);
+DEFINE_BOOT_PAGE_TABLE(boot_third);
 
 /* Main runtime page tables */
 
@@ -135,7 +146,7 @@ static __initdata int xenheap_first_first_slot = -1;
  */
 static DEFINE_PAGE_TABLES(xen_second, 2);
 /* First level page table used for fixmap */
-DEFINE_PAGE_TABLE(xen_fixmap);
+DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
 /* First level page table used to map Xen itself with the XN bit set
  * as appropriate. */
 static DEFINE_PAGE_TABLE(xen_xenmap);
-- 
2.11.0


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

Re: [Xen-devel] [PATCH 17/17] xen/arm64: Zero BSS after the MMU and D-cache is turned on

[Stefano Stabellini]

On Mon, 10 Jun 2019, Julien Grall wrote:
> At the moment BSS is zeroed before the MMU and D-Cache is turned on.
> In other words, the cache will be bypassed when zeroing the BSS section.
> 
> Per the Image protocol [1], the state of the cache for BSS region is not
> known because it is not part of the "loaded kernel image".
> 
> This means that the cache will need to be invalidated twice for the BSS
> region:
>     1) Before zeroing to remove any dirty cache line. Otherwise they may
>     get evicted while zeroing and therefore overriding the value.
>     2) After zeroing to remove any cache line that may have been
>     speculated. Otherwise when turning on MMU and D-Cache, the CPU may
>     see old values.
> 
> However, the only reason to have the BSS zeroed early is because the
> boot page tables are part of BSS. To avoid the two cache invalidations,
> it is possible to move the page tables in the section .data.page_aligned.

I am not following the last part. How is moving the boot page tables to
.data.page_aligned solving the problem? Do we need to zero
.data.page_aligned early or can we skip it because it is guaranteed to
already be zero?


> A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
> page-tables used before BSS is zeroed. This includes all boot_* but also
> xen_fixmap as zero_bss() will print a message when earlyprintk is
> enabled.

On a similar note, and continuing from what I wrote above, do we need to
make sure to zero the xen_fixmap before hooking it up setup_fixmap?


> [1] linux/Documentation/arm64/booting.txt
> 
> ---
> 
>     Note that the arm32 support is not there yet. This will need to be
>     addressed here or separately depending on when the Arm32 boot rework
>     is sent.
> ---
>  xen/arch/arm/arm64/head.S |  6 +++---
>  xen/arch/arm/mm.c         | 23 +++++++++++++++++------
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 130ab66d8e..6c3edbbc81 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -291,7 +291,6 @@ real_start_efi:
>          mov   x22, #0                /* x22 := is_secondary_cpu */
>  
>          bl    check_cpu_mode
> -        bl    zero_bss
>          bl    cpu_init
>          bl    create_page_tables
>          bl    enable_mmu
> @@ -312,6 +311,7 @@ primary_switched:
>          /* Use a virtual address to access the UART. */
>          ldr   x23, =EARLY_UART_VIRTUAL_ADDRESS
>  #endif
> +        bl    zero_bss
>          PRINT("- Ready -\r\n")
>          /* Setup the arguments for start_xen and jump to C world */
>          mov   x0, x20                /* x0 := phys_offset */
> @@ -423,8 +423,8 @@ zero_bss:
>          cbnz  x26, skip_bss
>  
>          PRINT("- Zero BSS -\r\n")
> -        load_paddr x0, __bss_start    /* Load paddr of start & end of bss */
> -        load_paddr x1, __bss_end
> +        ldr   x0, =__bss_start       /* x0 := vaddr(__bss_start) */
> +        ldr   x1, =__bss_end         /* x1 := vaddr(__bss_start) */
>  
>  1:      str   xzr, [x0], #8
>          cmp   x0, x1
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 6a549e9283..0b2d07a258 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -48,6 +48,17 @@
>  #undef mfn_to_virt
>  #define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>  
> +/*
> + * Macros to define page-tables:
> + *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
> + *  in assembly code before BSS is zeroed.
> + *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
> + *  page-tables to be used after BSS is zeroed (typically they are only used
> + *  in C).
> + */
> +#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
> +lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned") name[LPAE_ENTRIES]
> +
>  #define DEFINE_PAGE_TABLES(name, nr)                    \
>  lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>  
> @@ -76,13 +87,13 @@ lpae_t __aligned(PAGE_SIZE) name[LPAE_ENTRIES * (nr)]
>   * Finally, if EARLY_PRINTK is enabled then xen_fixmap will be mapped
>   * by the CPU once it has moved off the 1:1 mapping.
>   */
> -DEFINE_PAGE_TABLE(boot_pgtable);
> +DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
>  #ifdef CONFIG_ARM_64
> -DEFINE_PAGE_TABLE(boot_first);
> -DEFINE_PAGE_TABLE(boot_first_id);
> +DEFINE_BOOT_PAGE_TABLE(boot_first);
> +DEFINE_BOOT_PAGE_TABLE(boot_first_id);
>  #endif
> -DEFINE_PAGE_TABLE(boot_second);
> -DEFINE_PAGE_TABLE(boot_third);
> +DEFINE_BOOT_PAGE_TABLE(boot_second);
> +DEFINE_BOOT_PAGE_TABLE(boot_third);
>  
>  /* Main runtime page tables */
>  
> @@ -135,7 +146,7 @@ static __initdata int xenheap_first_first_slot = -1;
>   */
>  static DEFINE_PAGE_TABLES(xen_second, 2);
>  /* First level page table used for fixmap */
> -DEFINE_PAGE_TABLE(xen_fixmap);
> +DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
>  /* First level page table used to map Xen itself with the XN bit set
>   * as appropriate. */
>  static DEFINE_PAGE_TABLE(xen_xenmap);
> -- 
> 2.11.0
> 

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

Re: [Xen-devel] [PATCH 17/17] xen/arm64: Zero BSS after the MMU and D-cache is turned on

[Julien Grall]

Hi Stefano,

On 6/26/19 8:29 PM, Stefano Stabellini wrote:
> On Mon, 10 Jun 2019, Julien Grall wrote:
>> At the moment BSS is zeroed before the MMU and D-Cache is turned on.
>> In other words, the cache will be bypassed when zeroing the BSS section.
>>
>> Per the Image protocol [1], the state of the cache for BSS region is not
>> known because it is not part of the "loaded kernel image".
>>
>> This means that the cache will need to be invalidated twice for the BSS
>> region:
>>      1) Before zeroing to remove any dirty cache line. Otherwise they may
>>      get evicted while zeroing and therefore overriding the value.
>>      2) After zeroing to remove any cache line that may have been
>>      speculated. Otherwise when turning on MMU and D-Cache, the CPU may
>>      see old values.
>>
>> However, the only reason to have the BSS zeroed early is because the
>> boot page tables are part of BSS. To avoid the two cache invalidations,
>> it is possible to move the page tables in the section .data.page_aligned.
> 
> I am not following the last part. How is moving the boot page tables to
> .data.page_aligned solving the problem? Do we need to zero
> .data.page_aligned early or can we skip it because it is guaranteed to
> already be zero?

Global variables are initialized to zero by default regardless the 
section they reside. Usually they are stored in BSS because it saves 
space in the binary.

With the Image protocol, BSS is not initialized by the bootloader so we 
have to do ourself.

The section .data.page_aligned is always part of the binary. So the 
compiler will write zero in the binary for any global variables part of 
that section and therefore the corresponding memory will be zeroed when 
loading the binary.

If it makes sense, I can try to incorporate that in the commit message.

> 
> 
>> A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
>> page-tables used before BSS is zeroed. This includes all boot_* but also
>> xen_fixmap as zero_bss() will print a message when earlyprintk is
>> enabled.
> 
> On a similar note, and continuing from what I wrote above, do we need to
> make sure to zero the xen_fixmap before hooking it up setup_fixmap?

See above.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 17/17] xen/arm64: Zero BSS after the MMU and D-cache is turned on

[Stefano Stabellini]

On Wed, 26 Jun 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/26/19 8:29 PM, Stefano Stabellini wrote:
> > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > At the moment BSS is zeroed before the MMU and D-Cache is turned on.
> > > In other words, the cache will be bypassed when zeroing the BSS section.
> > > 
> > > Per the Image protocol [1], the state of the cache for BSS region is not
> > > known because it is not part of the "loaded kernel image".
> > > 
> > > This means that the cache will need to be invalidated twice for the BSS
> > > region:
> > >      1) Before zeroing to remove any dirty cache line. Otherwise they may
> > >      get evicted while zeroing and therefore overriding the value.
> > >      2) After zeroing to remove any cache line that may have been
> > >      speculated. Otherwise when turning on MMU and D-Cache, the CPU may
> > >      see old values.
> > > 
> > > However, the only reason to have the BSS zeroed early is because the
> > > boot page tables are part of BSS. To avoid the two cache invalidations,
> > > it is possible to move the page tables in the section .data.page_aligned.
> > 
> > I am not following the last part. How is moving the boot page tables to
> > .data.page_aligned solving the problem? Do we need to zero
> > .data.page_aligned early or can we skip it because it is guaranteed to
> > already be zero?
> 
> Global variables are initialized to zero by default regardless the section
> they reside. Usually they are stored in BSS because it saves space in the
> binary.
> 
> With the Image protocol, BSS is not initialized by the bootloader so we have
> to do ourself.
> 
> The section .data.page_aligned is always part of the binary. So the compiler
> will write zero in the binary for any global variables part of that section
> and therefore the corresponding memory will be zeroed when loading the binary.
> 
> If it makes sense, I can try to incorporate that in the commit message.

So basically it is really only BSS the problem. All right, looks good to
me.

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


> > > A new macro DEFINE_BOOT_PAGE_TABLE is introduced to create and mark
> > > page-tables used before BSS is zeroed. This includes all boot_* but also
> > > xen_fixmap as zero_bss() will print a message when earlyprintk is
> > > enabled.
> > 
> > On a similar note, and continuing from what I wrote above, do we need to
> > make sure to zero the xen_fixmap before hooking it up setup_fixmap?
> 
> See above.
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [Xen-devel] [PATCH 17/17] xen/arm64: Zero BSS after the MMU and D-cache is turned on

[Julien Grall]

Hi Stefano,

On 26/06/2019 22:08, Stefano Stabellini wrote:
> On Wed, 26 Jun 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 6/26/19 8:29 PM, Stefano Stabellini wrote:
>>> On Mon, 10 Jun 2019, Julien Grall wrote:
>>>> At the moment BSS is zeroed before the MMU and D-Cache is turned on.
>>>> In other words, the cache will be bypassed when zeroing the BSS section.
>>>>
>>>> Per the Image protocol [1], the state of the cache for BSS region is not
>>>> known because it is not part of the "loaded kernel image".
>>>>
>>>> This means that the cache will need to be invalidated twice for the BSS
>>>> region:
>>>>       1) Before zeroing to remove any dirty cache line. Otherwise they may
>>>>       get evicted while zeroing and therefore overriding the value.
>>>>       2) After zeroing to remove any cache line that may have been
>>>>       speculated. Otherwise when turning on MMU and D-Cache, the CPU may
>>>>       see old values.
>>>>
>>>> However, the only reason to have the BSS zeroed early is because the
>>>> boot page tables are part of BSS. To avoid the two cache invalidations,
>>>> it is possible to move the page tables in the section .data.page_aligned.
>>>
>>> I am not following the last part. How is moving the boot page tables to
>>> .data.page_aligned solving the problem? Do we need to zero
>>> .data.page_aligned early or can we skip it because it is guaranteed to
>>> already be zero?
>>
>> Global variables are initialized to zero by default regardless the section
>> they reside. Usually they are stored in BSS because it saves space in the
>> binary.
>>
>> With the Image protocol, BSS is not initialized by the bootloader so we have
>> to do ourself.
>>
>> The section .data.page_aligned is always part of the binary. So the compiler
>> will write zero in the binary for any global variables part of that section
>> and therefore the corresponding memory will be zeroed when loading the binary.
>>
>> If it makes sense, I can try to incorporate that in the commit message.
> 
> So basically it is really only BSS the problem. All right, looks good to
> me.

Yes that's correct.

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

Thank you!

Cheers,

-- 
Julien Grall

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