* [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:36 ` Wei Chen
2022-08-15 13:43 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it Julien Grall
` (7 subsequent siblings)
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Since commit 7e14a47e7c73 ("xen/arm64: head Rework and document
launch()"), the boot code is setting x22 but not read it.
So remove the two instructions setting x22 and update the documentation
to show x22 has no specific purpose.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm64/head.S | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 1babcc65d7c9..26cc7705f556 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -71,7 +71,7 @@
* x19 - paddr(start)
* x20 - phys offset
* x21 - DTB address (boot cpu only)
- * x22 - is_secondary_cpu
+ * x22 -
* x23 - UART address
* x24 -
* x25 -
@@ -305,8 +305,6 @@ real_start_efi:
#endif
PRINT("- Boot CPU booting -\r\n")
- mov x22, #0 /* x22 := is_secondary_cpu */
-
bl check_cpu_mode
bl cpu_init
bl create_page_tables
@@ -345,8 +343,6 @@ GLOBAL(init_secondary)
adr x19, start /* x19 := paddr (start) */
sub x20, x19, x0 /* x20 := phys-offset */
- mov x22, #1 /* x22 := is_secondary_cpu */
-
mrs x0, mpidr_el1
ldr x13, =(~MPIDR_HWID_MASK)
bic x24, x0, x13 /* Mask out flags to get CPU ID */
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* RE: [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation
2022-08-12 19:24 ` [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation Julien Grall
@ 2022-08-15 1:36 ` Wei Chen
2022-08-15 13:43 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Wei Chen @ 2022-08-15 1:36 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: 2022年8月13日 3:25
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [PATCH 1/7] xen/arm64: head: Don't set x22 and update the
> documentation
>
> From: Julien Grall <jgrall@amazon.com>
>
> Since commit 7e14a47e7c73 ("xen/arm64: head Rework and document
> launch()"), the boot code is setting x22 but not read it.
>
> So remove the two instructions setting x22 and update the documentation
> to show x22 has no specific purpose.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm64/head.S | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 1babcc65d7c9..26cc7705f556 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -71,7 +71,7 @@
> * x19 - paddr(start)
> * x20 - phys offset
> * x21 - DTB address (boot cpu only)
> - * x22 - is_secondary_cpu
> + * x22 -
> * x23 - UART address
> * x24 -
> * x25 -
> @@ -305,8 +305,6 @@ real_start_efi:
> #endif
> PRINT("- Boot CPU booting -\r\n")
>
> - mov x22, #0 /* x22 := is_secondary_cpu */
> -
> bl check_cpu_mode
> bl cpu_init
> bl create_page_tables
> @@ -345,8 +343,6 @@ GLOBAL(init_secondary)
> adr x19, start /* x19 := paddr (start) */
> sub x20, x19, x0 /* x20 := phys-offset */
>
> - mov x22, #1 /* x22 := is_secondary_cpu */
> -
> mrs x0, mpidr_el1
> ldr x13, =(~MPIDR_HWID_MASK)
> bic x24, x0, x13 /* Mask out flags to get CPU ID */
> --
> 2.37.1
>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation
2022-08-12 19:24 ` [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation Julien Grall
2022-08-15 1:36 ` Wei Chen
@ 2022-08-15 13:43 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 13:43 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> Since commit 7e14a47e7c73 ("xen/arm64: head Rework and document
> launch()"), the boot code is setting x22 but not read it.
>
> So remove the two instructions setting x22 and update the documentation
> to show x22 has no specific purpose.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/arm64/head.S | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 1babcc65d7c9..26cc7705f556 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -71,7 +71,7 @@
> * x19 - paddr(start)
> * x20 - phys offset
> * x21 - DTB address (boot cpu only)
> - * x22 - is_secondary_cpu
> + * x22 -
> * x23 - UART address
> * x24 -
> * x25 -
> @@ -305,8 +305,6 @@ real_start_efi:
> #endif
> PRINT("- Boot CPU booting -\r\n")
>
> - mov x22, #0 /* x22 := is_secondary_cpu */
> -
> bl check_cpu_mode
> bl cpu_init
> bl create_page_tables
> @@ -345,8 +343,6 @@ GLOBAL(init_secondary)
> adr x19, start /* x19 := paddr (start) */
> sub x20, x19, x0 /* x20 := phys-offset */
>
> - mov x22, #1 /* x22 := is_secondary_cpu */
> -
> mrs x0, mpidr_el1
> ldr x13, =(~MPIDR_HWID_MASK)
> bic x24, x0, x13 /* Mask out flags to get CPU ID */
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
2022-08-12 19:24 ` [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:45 ` Wei Chen
2022-08-15 14:45 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 3/7] xen/arm32: " Julien Grall
` (6 subsequent siblings)
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
There are a few places in the code that need to find the slot
at a given page-table level.
So create a new macro get_table_slot() for that. This will reduce
the effort to figure out whether the code is doing the right thing.
Take the opportunity to use 'ubfx'. The only benefits is reducing
the number of instructions from 2 to 1.
The new macro is used everywhere we need to compute the slot. This
requires to tweak the parameter of create_table_entry() to pass
a level rather than shift.
Note, for slot 0 the code is currently skipping the masking part. While
this is fine, it is safer to mask it as technically slot 0 only covers
bit 48 - 39 bit (assuming 4KB page granularity).
Take the opportunity to correct the comment when finding the second
slot for the identity mapping (we are computing the second slot
rather than first).
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
This patch also has the benefits to reduce the number
of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
patch for arm32 will reduce further.
---
xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 25 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 26cc7705f556..ad014716db6f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -493,13 +493,24 @@ cpu_init:
ret
ENDPROC(cpu_init)
+/*
+ * Macro to find the slot number at a given page-table level
+ *
+ * slot: slot computed
+ * virt: virtual address
+ * lvl: page-table level
+ */
+.macro get_table_slot, slot, virt, lvl
+ ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
+.endm
+
/*
* 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
+ * lvl: page-table level
* tmp1: scratch register
* tmp2: scratch register
* tmp3: scratch register
@@ -511,9 +522,8 @@ ENDPROC(cpu_init)
*
* 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, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+.macro create_table_entry, ptbl, tbl, virt, lvl, tmp1, tmp2, tmp3
+ get_table_slot \tmp1, \virt, \lvl /* \tmp1 := slot in \tlb */
load_paddr \tmp2, \tbl
mov \tmp3, #PT_PT /* \tmp3 := right for linear PT */
@@ -544,8 +554,7 @@ ENDPROC(cpu_init)
.macro create_mapping_entry, ptbl, virt, phys, tmp1, tmp2, tmp3, type=PT_MEM_L3
and \tmp3, \phys, #THIRD_MASK /* \tmp3 := PAGE_ALIGNED(phys) */
- lsr \tmp1, \virt, #THIRD_SHIFT
- and \tmp1, \tmp1, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
+ get_table_slot \tmp1, \virt, 3 /* \tmp1 := slot in \tlb */
mov \tmp2, #\type /* \tmp2 := right for section PT */
orr \tmp2, \tmp2, \tmp3 /* + PAGE_ALIGNED(phys) */
@@ -573,9 +582,9 @@ ENDPROC(cpu_init)
create_page_tables:
/* 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
+ create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
+ create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
+ create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
/* Map Xen */
adr_l x4, boot_third
@@ -612,10 +621,10 @@ create_page_tables:
* XEN_ZEROETH_SLOT, then the 1:1 mapping will use its own set of
* page-tables from the first level.
*/
- lsr x0, x19, #ZEROETH_SHIFT /* x0 := zeroeth slot */
+ get_table_slot x0, x19, 0 /* x0 := zeroeth slot */
cmp x0, #XEN_ZEROETH_SLOT
beq 1f
- create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2
+ create_table_entry boot_pgtable, boot_first_id, x19, 0, x0, x1, x2
b link_from_first_id
1:
@@ -624,11 +633,10 @@ create_page_tables:
* then the 1:1 mapping will use its own set of page-tables from
* the second level.
*/
- lsr x0, x19, #FIRST_SHIFT
- and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
+ get_table_slot x0, x19, 1 /* x0 := first slot */
cmp x0, #XEN_FIRST_SLOT
beq 1f
- create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
+ create_table_entry boot_first, boot_second_id, x19, 1, x0, x1, x2
b link_from_second_id
1:
@@ -638,17 +646,16 @@ create_page_tables:
* third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
* it.
*/
- lsr x0, x19, #SECOND_SHIFT
- and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
+ get_table_slot x0, x19, 2 /* x0 := second slot */
cmp x0, #XEN_SECOND_SLOT
beq virtphys_clash
- create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
+ create_table_entry boot_second, boot_third_id, x19, 2, x0, x1, x2
b link_from_third_id
link_from_first_id:
- create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
+ create_table_entry boot_first_id, boot_second_id, x19, 1, x0, x1, x2
link_from_second_id:
- create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
+ create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2
link_from_third_id:
create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
ret
@@ -705,7 +712,7 @@ remove_identity_mapping:
* Find the zeroeth slot used. Remove the entry from zeroeth
* table if the slot is not XEN_ZEROETH_SLOT.
*/
- lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */
+ get_table_slot x1, x19, 0 /* x1 := zeroeth slot */
cmp x1, #XEN_ZEROETH_SLOT
beq 1f
/* It is not in slot XEN_ZEROETH_SLOT, remove the entry. */
@@ -718,8 +725,7 @@ remove_identity_mapping:
* Find the first slot used. Remove the entry for the first
* table if the slot is not XEN_FIRST_SLOT.
*/
- lsr x1, x19, #FIRST_SHIFT
- and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
+ get_table_slot x1, x19, 1 /* x1 := first slot */
cmp x1, #XEN_FIRST_SLOT
beq 1f
/* It is not in slot XEN_FIRST_SLOT, remove the entry. */
@@ -732,8 +738,7 @@ remove_identity_mapping:
* Find the second slot used. Remove the entry for the first
* table if the slot is not XEN_SECOND_SLOT.
*/
- lsr x1, x19, #SECOND_SHIFT
- and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
+ get_table_slot x1, x19, 2 /* x1 := second slot */
cmp x1, #XEN_SECOND_SLOT
beq identity_mapping_removed
/* It is not in slot 1, remove the entry */
@@ -771,7 +776,7 @@ setup_fixmap:
#endif
/* Map fixmap into boot_second */
ldr x0, =FIXMAP_ADDR(0)
- create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3
+ create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
/* Ensure any page table updates made above have occurred. */
dsb nshst
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
2022-08-12 19:24 ` [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it Julien Grall
@ 2022-08-15 1:45 ` Wei Chen
2022-08-15 14:45 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Wei Chen @ 2022-08-15 1:45 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 2022/8/13 3:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> There are a few places in the code that need to find the slot
> at a given page-table level.
>
> So create a new macro get_table_slot() for that. This will reduce
> the effort to figure out whether the code is doing the right thing.
>
> Take the opportunity to use 'ubfx'. The only benefits is reducing
> the number of instructions from 2 to 1.
>
> The new macro is used everywhere we need to compute the slot. This
> requires to tweak the parameter of create_table_entry() to pass
> a level rather than shift.
>
> Note, for slot 0 the code is currently skipping the masking part. While
> this is fine, it is safer to mask it as technically slot 0 only covers
> bit 48 - 39 bit (assuming 4KB page granularity).
>
> Take the opportunity to correct the comment when finding the second
> slot for the identity mapping (we are computing the second slot
> rather than first).
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> ----
>
> This patch also has the benefits to reduce the number
> of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
> patch for arm32 will reduce further.
> ---
> xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 26cc7705f556..ad014716db6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -493,13 +493,24 @@ cpu_init:
> ret
> ENDPROC(cpu_init)
>
> +/*
> + * Macro to find the slot number at a given page-table level
> + *
> + * slot: slot computed
> + * virt: virtual address
> + * lvl: page-table level
> + */
> +.macro get_table_slot, slot, virt, lvl
> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> +.endm
> +
> /*
> * 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
> + * lvl: page-table level
> * tmp1: scratch register
> * tmp2: scratch register
> * tmp3: scratch register
> @@ -511,9 +522,8 @@ ENDPROC(cpu_init)
> *
> * 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, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +.macro create_table_entry, ptbl, tbl, virt, lvl, tmp1, tmp2, tmp3
> + get_table_slot \tmp1, \virt, \lvl /* \tmp1 := slot in \tlb */
>
> load_paddr \tmp2, \tbl
> mov \tmp3, #PT_PT /* \tmp3 := right for linear PT */
> @@ -544,8 +554,7 @@ ENDPROC(cpu_init)
> .macro create_mapping_entry, ptbl, virt, phys, tmp1, tmp2, tmp3, type=PT_MEM_L3
> and \tmp3, \phys, #THIRD_MASK /* \tmp3 := PAGE_ALIGNED(phys) */
>
> - lsr \tmp1, \virt, #THIRD_SHIFT
> - and \tmp1, \tmp1, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> + get_table_slot \tmp1, \virt, 3 /* \tmp1 := slot in \tlb */
>
> mov \tmp2, #\type /* \tmp2 := right for section PT */
> orr \tmp2, \tmp2, \tmp3 /* + PAGE_ALIGNED(phys) */
> @@ -573,9 +582,9 @@ ENDPROC(cpu_init)
> create_page_tables:
> /* 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
> + create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
> + create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
> + create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
>
> /* Map Xen */
> adr_l x4, boot_third
> @@ -612,10 +621,10 @@ create_page_tables:
> * XEN_ZEROETH_SLOT, then the 1:1 mapping will use its own set of
> * page-tables from the first level.
> */
> - lsr x0, x19, #ZEROETH_SHIFT /* x0 := zeroeth slot */
> + get_table_slot x0, x19, 0 /* x0 := zeroeth slot */
> cmp x0, #XEN_ZEROETH_SLOT
> beq 1f
> - create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2
> + create_table_entry boot_pgtable, boot_first_id, x19, 0, x0, x1, x2
> b link_from_first_id
>
> 1:
> @@ -624,11 +633,10 @@ create_page_tables:
> * then the 1:1 mapping will use its own set of page-tables from
> * the second level.
> */
> - lsr x0, x19, #FIRST_SHIFT
> - and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
> + get_table_slot x0, x19, 1 /* x0 := first slot */
> cmp x0, #XEN_FIRST_SLOT
> beq 1f
> - create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> + create_table_entry boot_first, boot_second_id, x19, 1, x0, x1, x2
> b link_from_second_id
>
> 1:
> @@ -638,17 +646,16 @@ create_page_tables:
> * third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
> * it.
> */
> - lsr x0, x19, #SECOND_SHIFT
> - and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
> + get_table_slot x0, x19, 2 /* x0 := second slot */
> cmp x0, #XEN_SECOND_SLOT
> beq virtphys_clash
> - create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> + create_table_entry boot_second, boot_third_id, x19, 2, x0, x1, x2
> b link_from_third_id
>
> link_from_first_id:
> - create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> + create_table_entry boot_first_id, boot_second_id, x19, 1, x0, x1, x2
> link_from_second_id:
> - create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> + create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2
> link_from_third_id:
> create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
> ret
> @@ -705,7 +712,7 @@ remove_identity_mapping:
> * Find the zeroeth slot used. Remove the entry from zeroeth
> * table if the slot is not XEN_ZEROETH_SLOT.
> */
> - lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */
> + get_table_slot x1, x19, 0 /* x1 := zeroeth slot */
> cmp x1, #XEN_ZEROETH_SLOT
> beq 1f
> /* It is not in slot XEN_ZEROETH_SLOT, remove the entry. */
> @@ -718,8 +725,7 @@ remove_identity_mapping:
> * Find the first slot used. Remove the entry for the first
> * table if the slot is not XEN_FIRST_SLOT.
> */
> - lsr x1, x19, #FIRST_SHIFT
> - and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
> + get_table_slot x1, x19, 1 /* x1 := first slot */
> cmp x1, #XEN_FIRST_SLOT
> beq 1f
> /* It is not in slot XEN_FIRST_SLOT, remove the entry. */
> @@ -732,8 +738,7 @@ remove_identity_mapping:
> * Find the second slot used. Remove the entry for the first
> * table if the slot is not XEN_SECOND_SLOT.
> */
> - lsr x1, x19, #SECOND_SHIFT
> - and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
> + get_table_slot x1, x19, 2 /* x1 := second slot */
> cmp x1, #XEN_SECOND_SLOT
> beq identity_mapping_removed
> /* It is not in slot 1, remove the entry */
> @@ -771,7 +776,7 @@ setup_fixmap:
> #endif
> /* Map fixmap into boot_second */
> ldr x0, =FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3
> + create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
2022-08-12 19:24 ` [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it Julien Grall
2022-08-15 1:45 ` Wei Chen
@ 2022-08-15 14:45 ` Bertrand Marquis
2022-08-15 16:44 ` Julien Grall
1 sibling, 1 reply; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 14:45 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> There are a few places in the code that need to find the slot
> at a given page-table level.
>
> So create a new macro get_table_slot() for that. This will reduce
> the effort to figure out whether the code is doing the right thing.
>
> Take the opportunity to use 'ubfx'. The only benefits is reducing
> the number of instructions from 2 to 1.
>
> The new macro is used everywhere we need to compute the slot. This
> requires to tweak the parameter of create_table_entry() to pass
> a level rather than shift.
>
> Note, for slot 0 the code is currently skipping the masking part. While
> this is fine, it is safer to mask it as technically slot 0 only covers
> bit 48 - 39 bit (assuming 4KB page granularity).
>
> Take the opportunity to correct the comment when finding the second
> slot for the identity mapping (we are computing the second slot
> rather than first).
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> ----
>
> This patch also has the benefits to reduce the number
> of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
> patch for arm32 will reduce further.
> ---
> xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
> 1 file changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 26cc7705f556..ad014716db6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -493,13 +493,24 @@ cpu_init:
> ret
> ENDPROC(cpu_init)
>
> +/*
> + * Macro to find the slot number at a given page-table level
> + *
> + * slot: slot computed
> + * virt: virtual address
> + * lvl: page-table level
> + */
> +.macro get_table_slot, slot, virt, lvl
> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> +.endm
> +
Crawling through the macros to verify the code was not that easy.
This is not related to this patch but XEN_PT_* macros could really do with more comments.
Cheers
Bertrand
> /*
> * 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
> + * lvl: page-table level
> * tmp1: scratch register
> * tmp2: scratch register
> * tmp3: scratch register
> @@ -511,9 +522,8 @@ ENDPROC(cpu_init)
> *
> * 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, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> +.macro create_table_entry, ptbl, tbl, virt, lvl, tmp1, tmp2, tmp3
> + get_table_slot \tmp1, \virt, \lvl /* \tmp1 := slot in \tlb */
>
> load_paddr \tmp2, \tbl
> mov \tmp3, #PT_PT /* \tmp3 := right for linear PT */
> @@ -544,8 +554,7 @@ ENDPROC(cpu_init)
> .macro create_mapping_entry, ptbl, virt, phys, tmp1, tmp2, tmp3, type=PT_MEM_L3
> and \tmp3, \phys, #THIRD_MASK /* \tmp3 := PAGE_ALIGNED(phys) */
>
> - lsr \tmp1, \virt, #THIRD_SHIFT
> - and \tmp1, \tmp1, #XEN_PT_LPAE_ENTRY_MASK/* \tmp1 := slot in \tlb */
> + get_table_slot \tmp1, \virt, 3 /* \tmp1 := slot in \tlb */
>
> mov \tmp2, #\type /* \tmp2 := right for section PT */
> orr \tmp2, \tmp2, \tmp3 /* + PAGE_ALIGNED(phys) */
> @@ -573,9 +582,9 @@ ENDPROC(cpu_init)
> create_page_tables:
> /* 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
> + create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
> + create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
> + create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
>
> /* Map Xen */
> adr_l x4, boot_third
> @@ -612,10 +621,10 @@ create_page_tables:
> * XEN_ZEROETH_SLOT, then the 1:1 mapping will use its own set of
> * page-tables from the first level.
> */
> - lsr x0, x19, #ZEROETH_SHIFT /* x0 := zeroeth slot */
> + get_table_slot x0, x19, 0 /* x0 := zeroeth slot */
> cmp x0, #XEN_ZEROETH_SLOT
> beq 1f
> - create_table_entry boot_pgtable, boot_first_id, x19, ZEROETH_SHIFT, x0, x1, x2
> + create_table_entry boot_pgtable, boot_first_id, x19, 0, x0, x1, x2
> b link_from_first_id
>
> 1:
> @@ -624,11 +633,10 @@ create_page_tables:
> * then the 1:1 mapping will use its own set of page-tables from
> * the second level.
> */
> - lsr x0, x19, #FIRST_SHIFT
> - and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
> + get_table_slot x0, x19, 1 /* x0 := first slot */
> cmp x0, #XEN_FIRST_SLOT
> beq 1f
> - create_table_entry boot_first, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> + create_table_entry boot_first, boot_second_id, x19, 1, x0, x1, x2
> b link_from_second_id
>
> 1:
> @@ -638,17 +646,16 @@ create_page_tables:
> * third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
> * it.
> */
> - lsr x0, x19, #SECOND_SHIFT
> - and x0, x0, #XEN_PT_LPAE_ENTRY_MASK /* x0 := first slot */
> + get_table_slot x0, x19, 2 /* x0 := second slot */
> cmp x0, #XEN_SECOND_SLOT
> beq virtphys_clash
> - create_table_entry boot_second, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> + create_table_entry boot_second, boot_third_id, x19, 2, x0, x1, x2
> b link_from_third_id
>
> link_from_first_id:
> - create_table_entry boot_first_id, boot_second_id, x19, FIRST_SHIFT, x0, x1, x2
> + create_table_entry boot_first_id, boot_second_id, x19, 1, x0, x1, x2
> link_from_second_id:
> - create_table_entry boot_second_id, boot_third_id, x19, SECOND_SHIFT, x0, x1, x2
> + create_table_entry boot_second_id, boot_third_id, x19, 2, x0, x1, x2
> link_from_third_id:
> create_mapping_entry boot_third_id, x19, x19, x0, x1, x2
> ret
> @@ -705,7 +712,7 @@ remove_identity_mapping:
> * Find the zeroeth slot used. Remove the entry from zeroeth
> * table if the slot is not XEN_ZEROETH_SLOT.
> */
> - lsr x1, x19, #ZEROETH_SHIFT /* x1 := zeroeth slot */
> + get_table_slot x1, x19, 0 /* x1 := zeroeth slot */
> cmp x1, #XEN_ZEROETH_SLOT
> beq 1f
> /* It is not in slot XEN_ZEROETH_SLOT, remove the entry. */
> @@ -718,8 +725,7 @@ remove_identity_mapping:
> * Find the first slot used. Remove the entry for the first
> * table if the slot is not XEN_FIRST_SLOT.
> */
> - lsr x1, x19, #FIRST_SHIFT
> - and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
> + get_table_slot x1, x19, 1 /* x1 := first slot */
> cmp x1, #XEN_FIRST_SLOT
> beq 1f
> /* It is not in slot XEN_FIRST_SLOT, remove the entry. */
> @@ -732,8 +738,7 @@ remove_identity_mapping:
> * Find the second slot used. Remove the entry for the first
> * table if the slot is not XEN_SECOND_SLOT.
> */
> - lsr x1, x19, #SECOND_SHIFT
> - and x1, x1, #XEN_PT_LPAE_ENTRY_MASK /* x1 := first slot */
> + get_table_slot x1, x19, 2 /* x1 := second slot */
> cmp x1, #XEN_SECOND_SLOT
> beq identity_mapping_removed
> /* It is not in slot 1, remove the entry */
> @@ -771,7 +776,7 @@ setup_fixmap:
> #endif
> /* Map fixmap into boot_second */
> ldr x0, =FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, x0, SECOND_SHIFT, x1, x2, x3
> + create_table_entry boot_second, xen_fixmap, x0, 2, x1, x2, x3
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
2022-08-15 14:45 ` Bertrand Marquis
@ 2022-08-15 16:44 ` Julien Grall
2022-08-16 7:36 ` Bertrand Marquis
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-08-15 16:44 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
On 15/08/2022 15:45, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,
>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> There are a few places in the code that need to find the slot
>> at a given page-table level.
>>
>> So create a new macro get_table_slot() for that. This will reduce
>> the effort to figure out whether the code is doing the right thing.
>>
>> Take the opportunity to use 'ubfx'. The only benefits is reducing
>> the number of instructions from 2 to 1.
>>
>> The new macro is used everywhere we need to compute the slot. This
>> requires to tweak the parameter of create_table_entry() to pass
>> a level rather than shift.
>>
>> Note, for slot 0 the code is currently skipping the masking part. While
>> this is fine, it is safer to mask it as technically slot 0 only covers
>> bit 48 - 39 bit (assuming 4KB page granularity).
>>
>> Take the opportunity to correct the comment when finding the second
>> slot for the identity mapping (we are computing the second slot
>> rather than first).
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Thanks!
>>
>> ----
>>
>> This patch also has the benefits to reduce the number
>> of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
>> patch for arm32 will reduce further.
>> ---
>> xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index 26cc7705f556..ad014716db6f 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -493,13 +493,24 @@ cpu_init:
>> ret
>> ENDPROC(cpu_init)
>>
>> +/*
>> + * Macro to find the slot number at a given page-table level
>> + *
>> + * slot: slot computed
>> + * virt: virtual address
>> + * lvl: page-table level
>> + */
>> +.macro get_table_slot, slot, virt, lvl
>> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
>> +.endm
>> +
>
> Crawling through the macros to verify the code was not that easy.
> This is not related to this patch but XEN_PT_* macros could really do with more comments.
To me, the name of the macros are self-explaining. So I am not entirely
what to write in the comments. Please suggest.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it
2022-08-15 16:44 ` Julien Grall
@ 2022-08-16 7:36 ` Bertrand Marquis
0 siblings, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-16 7:36 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi,
> On 15 Aug 2022, at 17:44, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 15/08/2022 15:45, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi Bertrand,
>
>>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> There are a few places in the code that need to find the slot
>>> at a given page-table level.
>>>
>>> So create a new macro get_table_slot() for that. This will reduce
>>> the effort to figure out whether the code is doing the right thing.
>>>
>>> Take the opportunity to use 'ubfx'. The only benefits is reducing
>>> the number of instructions from 2 to 1.
>>>
>>> The new macro is used everywhere we need to compute the slot. This
>>> requires to tweak the parameter of create_table_entry() to pass
>>> a level rather than shift.
>>>
>>> Note, for slot 0 the code is currently skipping the masking part. While
>>> this is fine, it is safer to mask it as technically slot 0 only covers
>>> bit 48 - 39 bit (assuming 4KB page granularity).
>>>
>>> Take the opportunity to correct the comment when finding the second
>>> slot for the identity mapping (we are computing the second slot
>>> rather than first).
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Thanks!
>
>>>
>>> ----
>>>
>>> This patch also has the benefits to reduce the number
>>> of use of {ZEROETH, FIRST, SECOND, THIRD}_SHIFT. The next
>>> patch for arm32 will reduce further.
>>> ---
>>> xen/arch/arm/arm64/head.S | 55 +++++++++++++++++++++------------------
>>> 1 file changed, 30 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 26cc7705f556..ad014716db6f 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -493,13 +493,24 @@ cpu_init:
>>> ret
>>> ENDPROC(cpu_init)
>>>
>>> +/*
>>> + * Macro to find the slot number at a given page-table level
>>> + *
>>> + * slot: slot computed
>>> + * virt: virtual address
>>> + * lvl: page-table level
>>> + */
>>> +.macro get_table_slot, slot, virt, lvl
>>> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
>>> +.endm
>>> +
>> Crawling through the macros to verify the code was not that easy.
>> This is not related to this patch but XEN_PT_* macros could really do with more comments.
>
> To me, the name of the macros are self-explaining. So I am not entirely what to write in the comments. Please suggest.
I will add that in my todo list and try to suggest something in the future (this is really not critical).
I was just pointing out because doing a deep review for someone who did not write this is complex due to the macros of macros of macros of macros ...
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/7] xen/arm32: head: Introduce get_table_slot() and use it
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
2022-08-12 19:24 ` [PATCH 1/7] xen/arm64: head: Don't set x22 and update the documentation Julien Grall
2022-08-12 19:24 ` [PATCH 2/7] xen/arm64: head: Introduce get_table_slot() and use it Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:48 ` Wei Chen
2022-08-15 14:56 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded Julien Grall
` (5 subsequent siblings)
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
There are a few places in the code that need to find the slot at a
given page-table level.
So create a new macro get_table_slot() for that. This will reduce
the effort to figure out whether the code is doing the right thing.
The new macro is using 'ubfx' (or 'lsr' for the first level) rather
than the existing sequence (mov_w, lsr, and) because it doesn't require
a scratch register and reduce the number of instructions (4 -> 1).
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm32/head.S | 56 ++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 24 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 46d93bebbabe..50f6fa4eb38d 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -358,13 +358,31 @@ cpu_init_done:
mov pc, r5 /* Return address is in r5 */
ENDPROC(cpu_init)
+/*
+ * Macro to find the slot number at a given page-table level
+ *
+ * slot: slot computed
+ * virt: virtual address
+ * lvl: page-table level
+ *
+ * Note that ubxf is unpredictable when the end bit is above 32-bit. So we
+ * can't use it for first level offset.
+ */
+.macro get_table_slot, slot, virt, lvl
+ .if \lvl == 1
+ lsr \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl)
+ .else
+ ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
+ .endif
+.endm
+
/*
* 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
+ * lvl: page-table level
* mmu: Is the MMU turned on/off. If not specified it will be off
*
* Preserves \virt
@@ -374,11 +392,9 @@ ENDPROC(cpu_init)
*
* Note that \virt should be in a register other than r1 - r4
*/
-.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
- lsr r1, \virt, #\shift
- mov_w r2, XEN_PT_LPAE_ENTRY_MASK
- and r1, r1, r2 /* r1 := slot in \tlb */
- lsl r1, r1, #3 /* r1 := slot offset in \tlb */
+.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
+ get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
+ lsl r1, r1, #3 /* r1 := slot offset in \tlb */
load_paddr r4, \tbl
@@ -448,8 +464,8 @@ ENDPROC(cpu_init)
create_page_tables:
/* Prepare the page-tables for mapping Xen */
ldr r0, =XEN_VIRT_START
- create_table_entry boot_pgtable, boot_second, r0, FIRST_SHIFT
- create_table_entry boot_second, boot_third, r0, SECOND_SHIFT
+ create_table_entry boot_pgtable, boot_second, r0, 1
+ create_table_entry boot_second, boot_third, r0, 2
/* Setup boot_third: */
adr_l r4, boot_third, mmu=0
@@ -486,12 +502,10 @@ create_page_tables:
* then the 1:1 mapping will use its own set of page-tables from
* the second level.
*/
- lsr r1, r9, #FIRST_SHIFT
- mov_w r0, XEN_PT_LPAE_ENTRY_MASK
- and r1, r1, r0 /* r1 := first slot */
+ get_table_slot r1, r9, 1 /* r1 := first slot */
cmp r1, #XEN_FIRST_SLOT
beq 1f
- create_table_entry boot_pgtable, boot_second_id, r9, FIRST_SHIFT
+ create_table_entry boot_pgtable, boot_second_id, r9, 1
b link_from_second_id
1:
@@ -501,16 +515,14 @@ create_page_tables:
* third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
* it.
*/
- lsr r1, r9, #SECOND_SHIFT
- mov_w r0, XEN_PT_LPAE_ENTRY_MASK
- and r1, r1, r0 /* r1 := second slot */
+ get_table_slot r1, r9, 2 /* r1 := second slot */
cmp r1, #XEN_SECOND_SLOT
beq virtphys_clash
- create_table_entry boot_second, boot_third_id, r9, SECOND_SHIFT
+ create_table_entry boot_second, boot_third_id, r9, 2
b link_from_third_id
link_from_second_id:
- create_table_entry boot_second_id, boot_third_id, r9, SECOND_SHIFT
+ create_table_entry boot_second_id, boot_third_id, r9, 2
link_from_third_id:
create_mapping_entry boot_third_id, r9, r9
mov pc, lr
@@ -571,9 +583,7 @@ remove_identity_mapping:
* Find the first slot used. Remove the entry for the first
* table if the slot is not XEN_FIRST_SLOT.
*/
- lsr r1, r9, #FIRST_SHIFT
- mov_w r0, XEN_PT_LPAE_ENTRY_MASK
- and r1, r1, r0 /* r1 := first slot */
+ get_table_slot r1, r9, 1 /* r1 := first slot */
cmp r1, #XEN_FIRST_SLOT
beq 1f
/* It is not in slot 0, remove the entry */
@@ -587,9 +597,7 @@ remove_identity_mapping:
* Find the second slot used. Remove the entry for the first
* table if the slot is not XEN_SECOND_SLOT.
*/
- lsr r1, r9, #SECOND_SHIFT
- mov_w r0, XEN_PT_LPAE_ENTRY_MASK
- and r1, r1, r0 /* r1 := second slot */
+ get_table_slot r1, r9, 2 /* r1 := second slot */
cmp r1, #XEN_SECOND_SLOT
beq identity_mapping_removed
/* It is not in slot 1, remove the entry */
@@ -628,7 +636,7 @@ setup_fixmap:
#endif
/* Map fixmap into boot_second */
mov_w r0, FIXMAP_ADDR(0)
- create_table_entry boot_second, xen_fixmap, r0, SECOND_SHIFT, mmu=1
+ create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
/* Ensure any page table updates made above have occurred. */
dsb nshst
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 3/7] xen/arm32: head: Introduce get_table_slot() and use it
2022-08-12 19:24 ` [PATCH 3/7] xen/arm32: " Julien Grall
@ 2022-08-15 1:48 ` Wei Chen
2022-08-15 14:56 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Wei Chen @ 2022-08-15 1:48 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 2022/8/13 3:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> There are a few places in the code that need to find the slot at a
> given page-table level.
>
> So create a new macro get_table_slot() for that. This will reduce
> the effort to figure out whether the code is doing the right thing.
>
> The new macro is using 'ubfx' (or 'lsr' for the first level) rather
> than the existing sequence (mov_w, lsr, and) because it doesn't require
> a scratch register and reduce the number of instructions (4 -> 1).
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/head.S | 56 ++++++++++++++++++++++-----------------
> 1 file changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 46d93bebbabe..50f6fa4eb38d 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -358,13 +358,31 @@ cpu_init_done:
> mov pc, r5 /* Return address is in r5 */
> ENDPROC(cpu_init)
>
> +/*
> + * Macro to find the slot number at a given page-table level
> + *
> + * slot: slot computed
> + * virt: virtual address
> + * lvl: page-table level
> + *
> + * Note that ubxf is unpredictable when the end bit is above 32-bit. So we
> + * can't use it for first level offset.
> + */
> +.macro get_table_slot, slot, virt, lvl
> + .if \lvl == 1
> + lsr \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl)
> + .else
> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> + .endif
> +.endm
> +
> /*
> * 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
> + * lvl: page-table level
> * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt
> @@ -374,11 +392,9 @@ ENDPROC(cpu_init)
> *
> * Note that \virt should be in a register other than r1 - r4
> */
> -.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
> - lsr r1, \virt, #\shift
> - mov_w r2, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r2 /* r1 := slot in \tlb */
> - lsl r1, r1, #3 /* r1 := slot offset in \tlb */
> +.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
> + get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
> + lsl r1, r1, #3 /* r1 := slot offset in \tlb */
>
> load_paddr r4, \tbl
>
> @@ -448,8 +464,8 @@ ENDPROC(cpu_init)
> create_page_tables:
> /* Prepare the page-tables for mapping Xen */
> ldr r0, =XEN_VIRT_START
> - create_table_entry boot_pgtable, boot_second, r0, FIRST_SHIFT
> - create_table_entry boot_second, boot_third, r0, SECOND_SHIFT
> + create_table_entry boot_pgtable, boot_second, r0, 1
> + create_table_entry boot_second, boot_third, r0, 2
>
> /* Setup boot_third: */
> adr_l r4, boot_third, mmu=0
> @@ -486,12 +502,10 @@ create_page_tables:
> * then the 1:1 mapping will use its own set of page-tables from
> * the second level.
> */
> - lsr r1, r9, #FIRST_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := first slot */
> + get_table_slot r1, r9, 1 /* r1 := first slot */
> cmp r1, #XEN_FIRST_SLOT
> beq 1f
> - create_table_entry boot_pgtable, boot_second_id, r9, FIRST_SHIFT
> + create_table_entry boot_pgtable, boot_second_id, r9, 1
> b link_from_second_id
>
> 1:
> @@ -501,16 +515,14 @@ create_page_tables:
> * third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
> * it.
> */
> - lsr r1, r9, #SECOND_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := second slot */
> + get_table_slot r1, r9, 2 /* r1 := second slot */
> cmp r1, #XEN_SECOND_SLOT
> beq virtphys_clash
> - create_table_entry boot_second, boot_third_id, r9, SECOND_SHIFT
> + create_table_entry boot_second, boot_third_id, r9, 2
> b link_from_third_id
>
> link_from_second_id:
> - create_table_entry boot_second_id, boot_third_id, r9, SECOND_SHIFT
> + create_table_entry boot_second_id, boot_third_id, r9, 2
> link_from_third_id:
> create_mapping_entry boot_third_id, r9, r9
> mov pc, lr
> @@ -571,9 +583,7 @@ remove_identity_mapping:
> * Find the first slot used. Remove the entry for the first
> * table if the slot is not XEN_FIRST_SLOT.
> */
> - lsr r1, r9, #FIRST_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := first slot */
> + get_table_slot r1, r9, 1 /* r1 := first slot */
> cmp r1, #XEN_FIRST_SLOT
> beq 1f
> /* It is not in slot 0, remove the entry */
> @@ -587,9 +597,7 @@ remove_identity_mapping:
> * Find the second slot used. Remove the entry for the first
> * table if the slot is not XEN_SECOND_SLOT.
> */
> - lsr r1, r9, #SECOND_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := second slot */
> + get_table_slot r1, r9, 2 /* r1 := second slot */
> cmp r1, #XEN_SECOND_SLOT
> beq identity_mapping_removed
> /* It is not in slot 1, remove the entry */
> @@ -628,7 +636,7 @@ setup_fixmap:
> #endif
> /* Map fixmap into boot_second */
> mov_w r0, FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, r0, SECOND_SHIFT, mmu=1
> + create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/7] xen/arm32: head: Introduce get_table_slot() and use it
2022-08-12 19:24 ` [PATCH 3/7] xen/arm32: " Julien Grall
2022-08-15 1:48 ` Wei Chen
@ 2022-08-15 14:56 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 14:56 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> There are a few places in the code that need to find the slot at a
> given page-table level.
>
> So create a new macro get_table_slot() for that. This will reduce
> the effort to figure out whether the code is doing the right thing.
>
> The new macro is using 'ubfx' (or 'lsr' for the first level) rather
> than the existing sequence (mov_w, lsr, and) because it doesn't require
> a scratch register and reduce the number of instructions (4 -> 1).
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Also I passed our test suite on arm32 qemu so:
Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/arm32/head.S | 56 ++++++++++++++++++++++-----------------
> 1 file changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 46d93bebbabe..50f6fa4eb38d 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -358,13 +358,31 @@ cpu_init_done:
> mov pc, r5 /* Return address is in r5 */
> ENDPROC(cpu_init)
>
> +/*
> + * Macro to find the slot number at a given page-table level
> + *
> + * slot: slot computed
> + * virt: virtual address
> + * lvl: page-table level
> + *
> + * Note that ubxf is unpredictable when the end bit is above 32-bit. So we
> + * can't use it for first level offset.
> + */
> +.macro get_table_slot, slot, virt, lvl
> + .if \lvl == 1
> + lsr \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl)
> + .else
> + ubfx \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> + .endif
> +.endm
> +
> /*
> * 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
> + * lvl: page-table level
> * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt
> @@ -374,11 +392,9 @@ ENDPROC(cpu_init)
> *
> * Note that \virt should be in a register other than r1 - r4
> */
> -.macro create_table_entry, ptbl, tbl, virt, shift, mmu=0
> - lsr r1, \virt, #\shift
> - mov_w r2, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r2 /* r1 := slot in \tlb */
> - lsl r1, r1, #3 /* r1 := slot offset in \tlb */
> +.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
> + get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
> + lsl r1, r1, #3 /* r1 := slot offset in \tlb */
>
> load_paddr r4, \tbl
>
> @@ -448,8 +464,8 @@ ENDPROC(cpu_init)
> create_page_tables:
> /* Prepare the page-tables for mapping Xen */
> ldr r0, =XEN_VIRT_START
> - create_table_entry boot_pgtable, boot_second, r0, FIRST_SHIFT
> - create_table_entry boot_second, boot_third, r0, SECOND_SHIFT
> + create_table_entry boot_pgtable, boot_second, r0, 1
> + create_table_entry boot_second, boot_third, r0, 2
>
> /* Setup boot_third: */
> adr_l r4, boot_third, mmu=0
> @@ -486,12 +502,10 @@ create_page_tables:
> * then the 1:1 mapping will use its own set of page-tables from
> * the second level.
> */
> - lsr r1, r9, #FIRST_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := first slot */
> + get_table_slot r1, r9, 1 /* r1 := first slot */
> cmp r1, #XEN_FIRST_SLOT
> beq 1f
> - create_table_entry boot_pgtable, boot_second_id, r9, FIRST_SHIFT
> + create_table_entry boot_pgtable, boot_second_id, r9, 1
> b link_from_second_id
>
> 1:
> @@ -501,16 +515,14 @@ create_page_tables:
> * third level. For slot XEN_SECOND_SLOT, Xen is not yet able to handle
> * it.
> */
> - lsr r1, r9, #SECOND_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := second slot */
> + get_table_slot r1, r9, 2 /* r1 := second slot */
> cmp r1, #XEN_SECOND_SLOT
> beq virtphys_clash
> - create_table_entry boot_second, boot_third_id, r9, SECOND_SHIFT
> + create_table_entry boot_second, boot_third_id, r9, 2
> b link_from_third_id
>
> link_from_second_id:
> - create_table_entry boot_second_id, boot_third_id, r9, SECOND_SHIFT
> + create_table_entry boot_second_id, boot_third_id, r9, 2
> link_from_third_id:
> create_mapping_entry boot_third_id, r9, r9
> mov pc, lr
> @@ -571,9 +583,7 @@ remove_identity_mapping:
> * Find the first slot used. Remove the entry for the first
> * table if the slot is not XEN_FIRST_SLOT.
> */
> - lsr r1, r9, #FIRST_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := first slot */
> + get_table_slot r1, r9, 1 /* r1 := first slot */
> cmp r1, #XEN_FIRST_SLOT
> beq 1f
> /* It is not in slot 0, remove the entry */
> @@ -587,9 +597,7 @@ remove_identity_mapping:
> * Find the second slot used. Remove the entry for the first
> * table if the slot is not XEN_SECOND_SLOT.
> */
> - lsr r1, r9, #SECOND_SHIFT
> - mov_w r0, XEN_PT_LPAE_ENTRY_MASK
> - and r1, r1, r0 /* r1 := second slot */
> + get_table_slot r1, r9, 2 /* r1 := second slot */
> cmp r1, #XEN_SECOND_SLOT
> beq identity_mapping_removed
> /* It is not in slot 1, remove the entry */
> @@ -628,7 +636,7 @@ setup_fixmap:
> #endif
> /* Map fixmap into boot_second */
> mov_w r0, FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, r0, SECOND_SHIFT, mmu=1
> + create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (2 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 3/7] xen/arm32: " Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:56 ` Wei Chen
2022-08-15 15:28 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
` (4 subsequent siblings)
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
At the moment, the macro addr_l needs to know whether the caller
is running with the MMU on. This is fine today because there are
only two possible cases:
1) MMU off
2) MMU on and linked to the virtual address
This is still cumbersome to use for the developer as they need
to know if the MMU is on.
Thankfully, Linux developpers came up with a great way to allow
adr_l to work within the range +/- 4GB of PC by emitting a PC-relative
reference [1].
Re-use the same approach on Arm and drop the parameter 'mmu'.
[1] 0b1674638a5c ("ARM: assembler: introduce adr_l, ldr_l and str_l macros")
Signed-off-by: Julien Grall <jgrall@amazon.com>
----
I haven't added an Origin tag because this is quite different
from the Linux commit. I am happy to add one if this is desired..
---
xen/arch/arm/arm32/head.S | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 50f6fa4eb38d..27d02ac8d68f 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -49,20 +49,16 @@
.endm
/*
- * There are no easy way to have a PC relative address within the range
- * +/- 4GB of the PC.
+ * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
+ * within the range +/- 4GB of the PC.
*
- * This macro workaround it by asking the user to tell whether the MMU
- * has been turned on or not.
- *
- * When the MMU is turned off, we need to apply the physical offset
- * (r10) in order to find the associated physical address.
+ * @dst: destination register
+ * @sym: name of the symbol
*/
-.macro adr_l, dst, sym, mmu
- ldr \dst, =\sym
- .if \mmu == 0
- add \dst, \dst, r10
- .endif
+.macro adr_l, dst, sym
+ mov_w \dst, \sym - .Lpc\@
+ .set .Lpc\@, .+ 8 /* PC bias */
+ add \dst, \dst, pc
.endm
.macro load_paddr rb, sym
@@ -383,7 +379,6 @@ ENDPROC(cpu_init)
* tbl: table symbol to point to
* virt: virtual address
* lvl: page-table level
- * mmu: Is the MMU turned on/off. If not specified it will be off
*
* Preserves \virt
* Clobbers r1 - r4
@@ -392,7 +387,7 @@ ENDPROC(cpu_init)
*
* Note that \virt should be in a register other than r1 - r4
*/
-.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
+.macro create_table_entry, ptbl, tbl, virt, lvl
get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
lsl r1, r1, #3 /* r1 := slot offset in \tlb */
@@ -402,7 +397,7 @@ ENDPROC(cpu_init)
orr r2, r2, r4 /* + \tlb paddr */
mov r3, #0
- adr_l r4, \ptbl, \mmu
+ adr_l r4, \ptbl
strd r2, r3, [r4, r1]
.endm
@@ -415,17 +410,14 @@ ENDPROC(cpu_init)
* virt: virtual address
* phys: physical address
* type: mapping type. If not specified it will be normal memory (PT_MEM_L3)
- * mmu: Is the MMU turned on/off. If not specified it will be off
*
* Preserves \virt, \phys
* Clobbers r1 - r4
*
- * * Also use r10 for the phys offset.
- *
* Note that \virt and \paddr should be in other registers than r1 - r4
* and be distinct.
*/
-.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3, mmu=0
+.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3
mov_w r2, XEN_PT_LPAE_ENTRY_MASK
lsr r1, \virt, #THIRD_SHIFT
and r1, r1, r2 /* r1 := slot in \tlb */
@@ -438,7 +430,7 @@ ENDPROC(cpu_init)
orr r2, r2, r4 /* + PAGE_ALIGNED(phys) */
mov r3, #0
- adr_l r4, \ptbl, \mmu
+ adr_l r4, \ptbl
strd r2, r3, [r4, r1]
.endm
@@ -468,7 +460,7 @@ create_page_tables:
create_table_entry boot_second, boot_third, r0, 2
/* Setup boot_third: */
- adr_l r4, boot_third, mmu=0
+ adr_l r4, boot_third
lsr r2, r9, #THIRD_SHIFT /* Base address for 4K mapping */
lsl r2, r2, #THIRD_SHIFT
@@ -632,11 +624,11 @@ setup_fixmap:
#if defined(CONFIG_EARLY_PRINTK)
/* Add UART to the fixmap table */
ldr r0, =EARLY_UART_VIRTUAL_ADDRESS
- create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3, mmu=1
+ create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
#endif
/* Map fixmap into boot_second */
mov_w r0, FIXMAP_ADDR(0)
- create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
+ create_table_entry boot_second, xen_fixmap, r0, 2
/* Ensure any page table updates made above have occurred. */
dsb nshst
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded
2022-08-12 19:24 ` [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded Julien Grall
@ 2022-08-15 1:56 ` Wei Chen
2022-08-15 15:28 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Wei Chen @ 2022-08-15 1:56 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 2022/8/13 3:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the macro addr_l needs to know whether the caller
> is running with the MMU on. This is fine today because there are
> only two possible cases:
> 1) MMU off
> 2) MMU on and linked to the virtual address
>
> This is still cumbersome to use for the developer as they need
> to know if the MMU is on.
>
> Thankfully, Linux developpers came up with a great way to allow
> adr_l to work within the range +/- 4GB of PC by emitting a PC-relative
> reference [1].
>
> Re-use the same approach on Arm and drop the parameter 'mmu'.
>
> [1] 0b1674638a5c ("ARM: assembler: introduce adr_l, ldr_l and str_l macros")
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> ----
> I haven't added an Origin tag because this is quite different
> from the Linux commit. I am happy to add one if this is desired..
> ---
> xen/arch/arm/arm32/head.S | 38 +++++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 50f6fa4eb38d..27d02ac8d68f 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -49,20 +49,16 @@
> .endm
>
> /*
> - * There are no easy way to have a PC relative address within the range
> - * +/- 4GB of the PC.
> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
> + * within the range +/- 4GB of the PC.
> *
> - * This macro workaround it by asking the user to tell whether the MMU
> - * has been turned on or not.
> - *
> - * When the MMU is turned off, we need to apply the physical offset
> - * (r10) in order to find the associated physical address.
> + * @dst: destination register
> + * @sym: name of the symbol
> */
> -.macro adr_l, dst, sym, mmu
> - ldr \dst, =\sym
> - .if \mmu == 0
> - add \dst, \dst, r10
> - .endif
> +.macro adr_l, dst, sym
> + mov_w \dst, \sym - .Lpc\@
> + .set .Lpc\@, .+ 8 /* PC bias */
> + add \dst, \dst, pc
> .endm
>
> .macro load_paddr rb, sym
> @@ -383,7 +379,6 @@ ENDPROC(cpu_init)
> * tbl: table symbol to point to
> * virt: virtual address
> * lvl: page-table level
> - * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt
> * Clobbers r1 - r4
> @@ -392,7 +387,7 @@ ENDPROC(cpu_init)
> *
> * Note that \virt should be in a register other than r1 - r4
> */
> -.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
> +.macro create_table_entry, ptbl, tbl, virt, lvl
> get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
> lsl r1, r1, #3 /* r1 := slot offset in \tlb */
>
> @@ -402,7 +397,7 @@ ENDPROC(cpu_init)
> orr r2, r2, r4 /* + \tlb paddr */
> mov r3, #0
>
> - adr_l r4, \ptbl, \mmu
> + adr_l r4, \ptbl
>
> strd r2, r3, [r4, r1]
> .endm
> @@ -415,17 +410,14 @@ ENDPROC(cpu_init)
> * virt: virtual address
> * phys: physical address
> * type: mapping type. If not specified it will be normal memory (PT_MEM_L3)
> - * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt, \phys
> * Clobbers r1 - r4
> *
> - * * Also use r10 for the phys offset.
> - *
> * Note that \virt and \paddr should be in other registers than r1 - r4
> * and be distinct.
> */
> -.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3, mmu=0
> +.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3
> mov_w r2, XEN_PT_LPAE_ENTRY_MASK
> lsr r1, \virt, #THIRD_SHIFT
> and r1, r1, r2 /* r1 := slot in \tlb */
> @@ -438,7 +430,7 @@ ENDPROC(cpu_init)
> orr r2, r2, r4 /* + PAGE_ALIGNED(phys) */
> mov r3, #0
>
> - adr_l r4, \ptbl, \mmu
> + adr_l r4, \ptbl
>
> strd r2, r3, [r4, r1]
> .endm
> @@ -468,7 +460,7 @@ create_page_tables:
> create_table_entry boot_second, boot_third, r0, 2
>
> /* Setup boot_third: */
> - adr_l r4, boot_third, mmu=0
> + adr_l r4, boot_third
>
> lsr r2, r9, #THIRD_SHIFT /* Base address for 4K mapping */
> lsl r2, r2, #THIRD_SHIFT
> @@ -632,11 +624,11 @@ setup_fixmap:
> #if defined(CONFIG_EARLY_PRINTK)
> /* Add UART to the fixmap table */
> ldr r0, =EARLY_UART_VIRTUAL_ADDRESS
> - create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3, mmu=1
> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
> #endif
> /* Map fixmap into boot_second */
> mov_w r0, FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
> + create_table_entry boot_second, xen_fixmap, r0, 2
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
LGTM.
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded
2022-08-12 19:24 ` [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded Julien Grall
2022-08-15 1:56 ` Wei Chen
@ 2022-08-15 15:28 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 15:28 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the macro addr_l needs to know whether the caller
> is running with the MMU on. This is fine today because there are
> only two possible cases:
> 1) MMU off
> 2) MMU on and linked to the virtual address
>
> This is still cumbersome to use for the developer as they need
> to know if the MMU is on.
>
> Thankfully, Linux developpers came up with a great way to allow
> adr_l to work within the range +/- 4GB of PC by emitting a PC-relative
> reference [1].
This is indeed a great solution :-)
>
> Re-use the same approach on Arm and drop the parameter 'mmu'.
>
> [1] 0b1674638a5c ("ARM: assembler: introduce adr_l, ldr_l and str_l macros")
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> ----
> I haven't added an Origin tag because this is quite different
> from the Linux commit. I am happy to add one if this is desired..
I think the reference in the commit message is enough as you reuse the
idea but not the code per say.
Cheers
Bertrand
> ---
> xen/arch/arm/arm32/head.S | 38 +++++++++++++++-----------------------
> 1 file changed, 15 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 50f6fa4eb38d..27d02ac8d68f 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -49,20 +49,16 @@
> .endm
>
> /*
> - * There are no easy way to have a PC relative address within the range
> - * +/- 4GB of the PC.
> + * Pseudo-op for PC relative adr <reg>, <symbol> where <symbol> is
> + * within the range +/- 4GB of the PC.
> *
> - * This macro workaround it by asking the user to tell whether the MMU
> - * has been turned on or not.
> - *
> - * When the MMU is turned off, we need to apply the physical offset
> - * (r10) in order to find the associated physical address.
> + * @dst: destination register
> + * @sym: name of the symbol
> */
> -.macro adr_l, dst, sym, mmu
> - ldr \dst, =\sym
> - .if \mmu == 0
> - add \dst, \dst, r10
> - .endif
> +.macro adr_l, dst, sym
> + mov_w \dst, \sym - .Lpc\@
> + .set .Lpc\@, .+ 8 /* PC bias */
> + add \dst, \dst, pc
> .endm
>
> .macro load_paddr rb, sym
> @@ -383,7 +379,6 @@ ENDPROC(cpu_init)
> * tbl: table symbol to point to
> * virt: virtual address
> * lvl: page-table level
> - * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt
> * Clobbers r1 - r4
> @@ -392,7 +387,7 @@ ENDPROC(cpu_init)
> *
> * Note that \virt should be in a register other than r1 - r4
> */
> -.macro create_table_entry, ptbl, tbl, virt, lvl, mmu=0
> +.macro create_table_entry, ptbl, tbl, virt, lvl
> get_table_slot r1, \virt, \lvl /* r1 := slot in \tlb */
> lsl r1, r1, #3 /* r1 := slot offset in \tlb */
>
> @@ -402,7 +397,7 @@ ENDPROC(cpu_init)
> orr r2, r2, r4 /* + \tlb paddr */
> mov r3, #0
>
> - adr_l r4, \ptbl, \mmu
> + adr_l r4, \ptbl
>
> strd r2, r3, [r4, r1]
> .endm
> @@ -415,17 +410,14 @@ ENDPROC(cpu_init)
> * virt: virtual address
> * phys: physical address
> * type: mapping type. If not specified it will be normal memory (PT_MEM_L3)
> - * mmu: Is the MMU turned on/off. If not specified it will be off
> *
> * Preserves \virt, \phys
> * Clobbers r1 - r4
> *
> - * * Also use r10 for the phys offset.
> - *
> * Note that \virt and \paddr should be in other registers than r1 - r4
> * and be distinct.
> */
> -.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3, mmu=0
> +.macro create_mapping_entry, ptbl, virt, phys, type=PT_MEM_L3
> mov_w r2, XEN_PT_LPAE_ENTRY_MASK
> lsr r1, \virt, #THIRD_SHIFT
> and r1, r1, r2 /* r1 := slot in \tlb */
> @@ -438,7 +430,7 @@ ENDPROC(cpu_init)
> orr r2, r2, r4 /* + PAGE_ALIGNED(phys) */
> mov r3, #0
>
> - adr_l r4, \ptbl, \mmu
> + adr_l r4, \ptbl
>
> strd r2, r3, [r4, r1]
> .endm
> @@ -468,7 +460,7 @@ create_page_tables:
> create_table_entry boot_second, boot_third, r0, 2
>
> /* Setup boot_third: */
> - adr_l r4, boot_third, mmu=0
> + adr_l r4, boot_third
>
> lsr r2, r9, #THIRD_SHIFT /* Base address for 4K mapping */
> lsl r2, r2, #THIRD_SHIFT
> @@ -632,11 +624,11 @@ setup_fixmap:
> #if defined(CONFIG_EARLY_PRINTK)
> /* Add UART to the fixmap table */
> ldr r0, =EARLY_UART_VIRTUAL_ADDRESS
> - create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3, mmu=1
> + create_mapping_entry xen_fixmap, r0, r11, type=PT_DEV_L3
> #endif
> /* Map fixmap into boot_second */
> mov_w r0, FIXMAP_ADDR(0)
> - create_table_entry boot_second, xen_fixmap, r0, 2, mmu=1
> + create_table_entry boot_second, xen_fixmap, r0, 2
> /* Ensure any page table updates made above have occurred. */
> dsb nshst
>
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (3 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 4/7] xen/arm32: heap: Rework adr_l so it doesn't rely on where Xen is loaded Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:57 ` Jiamei Xie
` (3 more replies)
2022-08-12 19:24 ` [PATCH 6/7] xen/arm: Tweak the dump page-table walk output Julien Grall
` (3 subsequent siblings)
8 siblings, 4 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
At the moment, the strings are in text right after each use because
the instruction 'adr' has specific requirement on the location
and the compiler will forbid cross section label.
The macro 'adr_l' was recently reworked so the caller doesn't need
to know whether the MMU is on. This makes it easier to use where
instructions can be run in both context.
This also means that the strings don't need to be part of .text
anymore. So move them to .rodata.str.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm32/head.S | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 27d02ac8d68f..a558c2a6876e 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -93,13 +93,10 @@
*/
#define PRINT(_s) \
mov r3, lr ;\
- adr r0, 98f ;\
+ adr_l r0, 98f ;\
bl puts ;\
mov lr, r3 ;\
- b 99f ;\
-98: .asciz _s ;\
- .align 2 ;\
-99:
+ RODATA_STR(98, _s)
/*
* Macro to print the value of register \rb
@@ -736,7 +733,7 @@ ENDPROC(puts)
* Clobbers r0-r3
*/
putn:
- adr r1, hex
+ adr_l r1, hex
mov r3, #8
1:
early_uart_ready r11, r2
@@ -749,8 +746,7 @@ putn:
mov pc, lr
ENDPROC(putn)
-hex: .ascii "0123456789abcdef"
- .align 2
+RODATA_STR(hex, "0123456789abcdef")
#else /* CONFIG_EARLY_PRINTK */
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* RE: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
@ 2022-08-15 1:57 ` Jiamei Xie
2022-08-15 2:05 ` Wei Chen
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Jiamei Xie @ 2022-08-15 1:57 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> Julien Grall
> Sent: Saturday, August 13, 2022 3:25 AM
> To: xen-devel@lists.xenproject.org
> Cc: julien@xen.org; Julien Grall <jgrall@amazon.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages
> to .rodata.str
>
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
>
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
>
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/head.S | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
> */
> #define PRINT(_s) \
> mov r3, lr ;\
> - adr r0, 98f ;\
> + adr_l r0, 98f ;\
> bl puts ;\
> mov lr, r3 ;\
> - b 99f ;\
> -98: .asciz _s ;\
> - .align 2 ;\
> -99:
> + RODATA_STR(98, _s)
>
> /*
> * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
> * Clobbers r0-r3
> */
> putn:
> - adr r1, hex
> + adr_l r1, hex
> mov r3, #8
> 1:
> early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
> mov pc, lr
> ENDPROC(putn)
>
> -hex: .ascii "0123456789abcdef"
> - .align 2
> +RODATA_STR(hex, "0123456789abcdef")
>
> #else /* CONFIG_EARLY_PRINTK */
>
> --
> 2.37.1
>
That looks good to me.
Reviewed-by: Jiamei Xie <jiamei.xie@arm.com>
Best wishes
Jiamei Xie
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
2022-08-15 1:57 ` Jiamei Xie
@ 2022-08-15 2:05 ` Wei Chen
2022-08-15 6:43 ` Jan Beulich
2022-08-15 16:26 ` Bertrand Marquis
3 siblings, 0 replies; 38+ messages in thread
From: Wei Chen @ 2022-08-15 2:05 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 2022/8/13 3:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
>
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
>
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
>
This sounds very good!
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/head.S | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
> */
> #define PRINT(_s) \
> mov r3, lr ;\
> - adr r0, 98f ;\
> + adr_l r0, 98f ;\
> bl puts ;\
> mov lr, r3 ;\
> - b 99f ;\
> -98: .asciz _s ;\
> - .align 2 ;\
> -99:
> + RODATA_STR(98, _s)
>
> /*
> * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
> * Clobbers r0-r3
> */
> putn:
> - adr r1, hex
> + adr_l r1, hex
> mov r3, #8
> 1:
> early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
> mov pc, lr
> ENDPROC(putn)
>
> -hex: .ascii "0123456789abcdef"
> - .align 2
> +RODATA_STR(hex, "0123456789abcdef")
>
> #else /* CONFIG_EARLY_PRINTK */
>
Reviewed-by: Wei Chen <Wei.Chen@arm.com>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
2022-08-15 1:57 ` Jiamei Xie
2022-08-15 2:05 ` Wei Chen
@ 2022-08-15 6:43 ` Jan Beulich
2022-08-15 8:17 ` Julien Grall
2022-08-15 16:26 ` Bertrand Marquis
3 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-08-15 6:43 UTC (permalink / raw)
To: Julien Grall
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, xen-devel
On 12.08.2022 21:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
>
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
>
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
Wouldn't they better live somewhere in .init* ?
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
> */
> #define PRINT(_s) \
> mov r3, lr ;\
> - adr r0, 98f ;\
> + adr_l r0, 98f ;\
> bl puts ;\
> mov lr, r3 ;\
> - b 99f ;\
> -98: .asciz _s ;\
> - .align 2 ;\
> -99:
> + RODATA_STR(98, _s)
Nit: How about using uniform indentation here and ...
> @@ -736,7 +733,7 @@ ENDPROC(puts)
> * Clobbers r0-r3
> */
> putn:
> - adr r1, hex
> + adr_l r1, hex
> mov r3, #8
> 1:
> early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
> mov pc, lr
> ENDPROC(putn)
>
> -hex: .ascii "0123456789abcdef"
> - .align 2
> +RODATA_STR(hex, "0123456789abcdef")
... here?
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-15 6:43 ` Jan Beulich
@ 2022-08-15 8:17 ` Julien Grall
2022-08-15 8:21 ` Jan Beulich
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-08-15 8:17 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, xen-devel
Hi Jan,
On 15/08/2022 07:43, Jan Beulich wrote:
> On 12.08.2022 21:24, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, the strings are in text right after each use because
>> the instruction 'adr' has specific requirement on the location
>> and the compiler will forbid cross section label.
>>
>> The macro 'adr_l' was recently reworked so the caller doesn't need
>> to know whether the MMU is on. This makes it easier to use where
>> instructions can be run in both context.
>>
>> This also means that the strings don't need to be part of .text
>> anymore. So move them to .rodata.str.
>
> Wouldn't they better live somewhere in .init* ?
PRINT() is also used in path for secondary bring-up. So this could be
used after .init (even though today CPU hotplug doesn't work on Arm).
Furthermore, PRINT() is only used when earlyprintk is enabled. This
should only be used in a development environment (gated by
CONFIG_DEBUG). So I think it is better to keep all the strings in
.rodata.str rather than trying to distinguish whether the caller will
happen only during init on boot.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-15 8:17 ` Julien Grall
@ 2022-08-15 8:21 ` Jan Beulich
2022-08-15 16:46 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Jan Beulich @ 2022-08-15 8:21 UTC (permalink / raw)
To: Julien Grall
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, xen-devel
On 15.08.2022 10:17, Julien Grall wrote:
> On 15/08/2022 07:43, Jan Beulich wrote:
>> On 12.08.2022 21:24, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> At the moment, the strings are in text right after each use because
>>> the instruction 'adr' has specific requirement on the location
>>> and the compiler will forbid cross section label.
>>>
>>> The macro 'adr_l' was recently reworked so the caller doesn't need
>>> to know whether the MMU is on. This makes it easier to use where
>>> instructions can be run in both context.
>>>
>>> This also means that the strings don't need to be part of .text
>>> anymore. So move them to .rodata.str.
>>
>> Wouldn't they better live somewhere in .init* ?
>
> PRINT() is also used in path for secondary bring-up. So this could be
> used after .init (even though today CPU hotplug doesn't work on Arm).
Then the term "earlyprintk" looks to be misleading?
> Furthermore, PRINT() is only used when earlyprintk is enabled. This
> should only be used in a development environment (gated by
> CONFIG_DEBUG). So I think it is better to keep all the strings in
> .rodata.str rather than trying to distinguish whether the caller will
> happen only during init on boot.
Fair enough.
Jan
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-15 8:21 ` Jan Beulich
@ 2022-08-15 16:46 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-15 16:46 UTC (permalink / raw)
To: Jan Beulich
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, xen-devel
Hi Jan,
On 15/08/2022 09:21, Jan Beulich wrote:
> On 15.08.2022 10:17, Julien Grall wrote:
>> On 15/08/2022 07:43, Jan Beulich wrote:
>>> On 12.08.2022 21:24, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> At the moment, the strings are in text right after each use because
>>>> the instruction 'adr' has specific requirement on the location
>>>> and the compiler will forbid cross section label.
>>>>
>>>> The macro 'adr_l' was recently reworked so the caller doesn't need
>>>> to know whether the MMU is on. This makes it easier to use where
>>>> instructions can be run in both context.
>>>>
>>>> This also means that the strings don't need to be part of .text
>>>> anymore. So move them to .rodata.str.
>>>
>>> Wouldn't they better live somewhere in .init* ?
>>
>> PRINT() is also used in path for secondary bring-up. So this could be
>> used after .init (even though today CPU hotplug doesn't work on Arm).
>
> Then the term "earlyprintk" looks to be misleading?
I can't think of a better name. "early" could also be interpreted as
"early CPU bring-up".
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
` (2 preceding siblings ...)
2022-08-15 6:43 ` Jan Beulich
@ 2022-08-15 16:26 ` Bertrand Marquis
3 siblings, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 16:26 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> At the moment, the strings are in text right after each use because
> the instruction 'adr' has specific requirement on the location
> and the compiler will forbid cross section label.
>
> The macro 'adr_l' was recently reworked so the caller doesn't need
> to know whether the MMU is on. This makes it easier to use where
> instructions can be run in both context.
>
> This also means that the strings don't need to be part of .text
> anymore. So move them to .rodata.str.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Tested by enabling early printk on qemu arm32 and it works so:
Tested-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/arm32/head.S | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 27d02ac8d68f..a558c2a6876e 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -93,13 +93,10 @@
> */
> #define PRINT(_s) \
> mov r3, lr ;\
> - adr r0, 98f ;\
> + adr_l r0, 98f ;\
> bl puts ;\
> mov lr, r3 ;\
> - b 99f ;\
> -98: .asciz _s ;\
> - .align 2 ;\
> -99:
> + RODATA_STR(98, _s)
>
> /*
> * Macro to print the value of register \rb
> @@ -736,7 +733,7 @@ ENDPROC(puts)
> * Clobbers r0-r3
> */
> putn:
> - adr r1, hex
> + adr_l r1, hex
> mov r3, #8
> 1:
> early_uart_ready r11, r2
> @@ -749,8 +746,7 @@ putn:
> mov pc, lr
> ENDPROC(putn)
>
> -hex: .ascii "0123456789abcdef"
> - .align 2
> +RODATA_STR(hex, "0123456789abcdef")
>
> #else /* CONFIG_EARLY_PRINTK */
>
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 6/7] xen/arm: Tweak the dump page-table walk output
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (4 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 5/7] xen/arm32: head: Move earlyprintk messages to .rodata.str Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:32 ` Henry Wang
2022-08-15 16:12 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 7/8] patch arm32-tweak-enable-mmu.patch Julien Grall
` (2 subsequent siblings)
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Currently the output is looking like:
(XEN) 1ST[0x1] = 0x000000004015ff7f
(XEN) 2ND[0x1f] = 0x00500000bfe00f7d
The content of the entries are not aligned making a bit trickier to
read (I appreciate this is a matter of taste).
Align the values by forcing the index to be always printed using
3 characters (enough to cover 512 in hexadecimal).
New output:
(XEN) 1ST[0x001] = 0x000000004015ff7f
(XEN) 2ND[0x01f] = 0x00500000bfe00f7d
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/mm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b42cddb1b446..c81c706c8b23 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -234,7 +234,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
pte = mapping[offsets[level]];
- printk("%s[0x%x] = 0x%"PRIpaddr"\n",
+ printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
level_strs[level], offsets[level], pte.bits);
if ( level == 3 || !pte.walk.valid || !pte.walk.table )
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* RE: [PATCH 6/7] xen/arm: Tweak the dump page-table walk output
2022-08-12 19:24 ` [PATCH 6/7] xen/arm: Tweak the dump page-table walk output Julien Grall
@ 2022-08-15 1:32 ` Henry Wang
2022-08-15 16:12 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Henry Wang @ 2022-08-15 1:32 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> -----Original Message-----
> Subject: [PATCH 6/7] xen/arm: Tweak the dump page-table walk output
>
> From: Julien Grall <jgrall@amazon.com>
>
> Currently the output is looking like:
>
> (XEN) 1ST[0x1] = 0x000000004015ff7f
> (XEN) 2ND[0x1f] = 0x00500000bfe00f7d
>
> The content of the entries are not aligned making a bit trickier to
> read (I appreciate this is a matter of taste).
>
> Align the values by forcing the index to be always printed using
> 3 characters (enough to cover 512 in hexadecimal).
>
> New output:
>
> (XEN) 1ST[0x001] = 0x000000004015ff7f
> (XEN) 2ND[0x01f] = 0x00500000bfe00f7d
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
Kind regards,
Henry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 6/7] xen/arm: Tweak the dump page-table walk output
2022-08-12 19:24 ` [PATCH 6/7] xen/arm: Tweak the dump page-table walk output Julien Grall
2022-08-15 1:32 ` Henry Wang
@ 2022-08-15 16:12 ` Bertrand Marquis
1 sibling, 0 replies; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 16:12 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> Currently the output is looking like:
>
> (XEN) 1ST[0x1] = 0x000000004015ff7f
> (XEN) 2ND[0x1f] = 0x00500000bfe00f7d
>
> The content of the entries are not aligned making a bit trickier to
> read (I appreciate this is a matter of taste).
>
> Align the values by forcing the index to be always printed using
> 3 characters (enough to cover 512 in hexadecimal).
>
> New output:
>
> (XEN) 1ST[0x001] = 0x000000004015ff7f
> (XEN) 2ND[0x01f] = 0x00500000bfe00f7d
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> xen/arch/arm/mm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b42cddb1b446..c81c706c8b23 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -234,7 +234,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>
> pte = mapping[offsets[level]];
>
> - printk("%s[0x%x] = 0x%"PRIpaddr"\n",
> + printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
> level_strs[level], offsets[level], pte.bits);
>
> if ( level == 3 || !pte.walk.valid || !pte.walk.table )
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 7/8] patch arm32-tweak-enable-mmu.patch
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (5 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 6/7] xen/arm: Tweak the dump page-table walk output Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-12 19:24 ` [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort Julien Grall
2022-08-12 19:24 ` [PATCH 8/8] " Julien Grall
8 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm32/head.S | 50 +++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index a558c2a6876e..a914ffab98a5 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -167,19 +167,12 @@ past_zImage:
bl check_cpu_mode
bl cpu_init
bl create_page_tables
- bl enable_mmu
- /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
- ldr r0, =primary_switched
- mov pc, r0
+ /* Address in the runtime mapping to jump to after the MMU is enabled */
+ mov_w lr, primary_switched
+ b enable_mmu
+
primary_switched:
- /*
- * The 1:1 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_identity_mapping
bl setup_fixmap
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
@@ -223,12 +216,10 @@ GLOBAL(init_secondary)
bl check_cpu_mode
bl cpu_init
bl create_page_tables
- bl enable_mmu
-
- /* We are still in the 1:1 mapping. Jump to the runtime Virtual Address. */
- ldr r0, =secondary_switched
- mov pc, r0
+ /* Address in the runtime mapping to jump to after the MMU is enabled */
+ mov_w lr, secondary_switched
+ b enable_mmu
secondary_switched:
/*
* Non-boot CPUs need to move on to the proper pagetables, which were
@@ -523,9 +514,11 @@ virtphys_clash:
ENDPROC(create_page_tables)
/*
- * Turn on the Data Cache and the MMU. The function will return on the 1:1
- * mapping. In other word, the caller is responsible to switch to the runtime
- * mapping.
+ * Turn on the Data Cache and the MMU. The function will return
+ * to the virtual address provided in LR (e.g. the runtime mapping).
+ *
+ * Inputs:
+ * lr : Virtual address to return to
*
* Clobbers r0 - r3
*/
@@ -551,7 +544,24 @@ enable_mmu:
dsb /* Flush PTE writes and finish reads */
mcr CP32(r0, HSCTLR) /* now paging is enabled */
isb /* Now, flush the icache */
- mov pc, lr
+
+ /*
+ * The MMU is turned on and we are in the 1:1 mapping. Switch
+ * to the runtime mapping.
+ */
+ mov_w r0, 1f
+ mov pc, r0
+1:
+ /*
+ * The 1:1 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.
+ *
+ * On return this will jump to the virtual address requested by
+ * the caller.
+ */
+ b remove_identity_mapping
ENDPROC(enable_mmu)
/*
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (6 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 7/8] patch arm32-tweak-enable-mmu.patch Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 1:40 ` Henry Wang
2022-08-15 16:39 ` Bertrand Marquis
2022-08-12 19:24 ` [PATCH 8/8] " Julien Grall
8 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Unlike arm64, on arm32 there are no extra information dumped (e.g.
page table walk) for hypervisor data abort.
For data abort, the HSR will be set properly and so replace the call
to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm32/traps.c | 2 +-
xen/arch/arm/include/asm/traps.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index a4ce2b92d904..a2fc1c22cbc9 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
if ( VABORT_GEN_BY_GUEST(regs) )
do_trap_guest_serror(regs);
else
- do_unexpected_trap("Data Abort", regs);
+ do_trap_hyp_sync(regs);
}
void finalize_instr_emulation(const struct instr_details *instr)
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 08bc0b484c75..883dae368eac 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
void noreturn do_unexpected_trap(const char *msg,
const struct cpu_user_regs *regs);
+void do_trap_hyp_sync(struct cpu_user_regs *regs);
/* Functions for pending virtual abort checking window. */
void abort_guest_exit_start(void);
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* RE: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-12 19:24 ` [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort Julien Grall
@ 2022-08-15 1:40 ` Henry Wang
2022-08-15 16:47 ` Julien Grall
2022-08-15 16:39 ` Bertrand Marquis
1 sibling, 1 reply; 38+ messages in thread
From: Henry Wang @ 2022-08-15 1:40 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> -----Original Message-----
> Subject: [PATCH 7/7] xen/arm32: traps: Dump more information for
> hypervisor data abort
>
> From: Julien Grall <jgrall@amazon.com>
>
> Unlike arm64, on arm32 there are no extra information dumped (e.g.
> page table walk) for hypervisor data abort.
>
> For data abort, the HSR will be set properly and so replace the call
> to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
I think this patch looks good to me so:
Reviewed-by: Henry Wang <Henry.Wang@arm.com>
But it seems that there is a duplicated patch at:
https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-10-julien@xen.org/
Kind regards,
Henry
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-15 1:40 ` Henry Wang
@ 2022-08-15 16:47 ` Julien Grall
2022-08-16 3:29 ` Henry Wang
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-08-15 16:47 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 15/08/2022 02:40, Henry Wang wrote:
> Hi Julien,
Hi Henry,
>> -----Original Message-----
>> Subject: [PATCH 7/7] xen/arm32: traps: Dump more information for
>> hypervisor data abort
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Unlike arm64, on arm32 there are no extra information dumped (e.g.
>> page table walk) for hypervisor data abort.
>>
>> For data abort, the HSR will be set properly and so replace the call
>> to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> I think this patch looks good to me so:
>
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
>
> But it seems that there is a duplicated patch at:
> https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-10-julien@xen.org/
Hmmm... I dropped a patch from the series and it looks like I didn't
properly clean the directory before doing that. Please ignore patch #8.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-15 16:47 ` Julien Grall
@ 2022-08-16 3:29 ` Henry Wang
2022-08-31 19:12 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Henry Wang @ 2022-08-16 3:29 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 7/7] xen/arm32: traps: Dump more information for
> hypervisor data abort
> Hmmm... I dropped a patch from the series and it looks like I didn't
> properly clean the directory before doing that. Please ignore patch #8.
Sure, I guess the patch that you dropped is this one?
https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-8-julien@xen.org/
Kind regards,
Henry
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-16 3:29 ` Henry Wang
@ 2022-08-31 19:12 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-31 19:12 UTC (permalink / raw)
To: Henry Wang, xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 16/08/2022 04:29, Henry Wang wrote:
> Hi Julien,
Hi Henry,
Sorry for the late reply.
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH 7/7] xen/arm32: traps: Dump more information for
>> hypervisor data abort
>> Hmmm... I dropped a patch from the series and it looks like I didn't
>> properly clean the directory before doing that. Please ignore patch #8.
>
> Sure, I guess the patch that you dropped is this one?
> https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-8-julien@xen.org/
That's correct.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-12 19:24 ` [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort Julien Grall
2022-08-15 1:40 ` Henry Wang
@ 2022-08-15 16:39 ` Bertrand Marquis
2022-08-15 17:04 ` Julien Grall
1 sibling, 1 reply; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-15 16:39 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>
> From: Julien Grall <jgrall@amazon.com>
>
> Unlike arm64, on arm32 there are no extra information dumped (e.g.
> page table walk) for hypervisor data abort.
The code in arch/arm/traps.c has nothing arm32 specific like that so
could you explain this statement ?
Here the arm32 code will call the generic function which has only
something specific for BRK handling but the rest is generic.
>
> For data abort, the HSR will be set properly and so replace the call
> to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
I agree with that, I just do not understand your previous statement here.
Cheers
Bertrand
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/traps.c | 2 +-
> xen/arch/arm/include/asm/traps.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index a4ce2b92d904..a2fc1c22cbc9 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
> if ( VABORT_GEN_BY_GUEST(regs) )
> do_trap_guest_serror(regs);
> else
> - do_unexpected_trap("Data Abort", regs);
> + do_trap_hyp_sync(regs);
> }
>
> void finalize_instr_emulation(const struct instr_details *instr)
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 08bc0b484c75..883dae368eac 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
>
> void noreturn do_unexpected_trap(const char *msg,
> const struct cpu_user_regs *regs);
> +void do_trap_hyp_sync(struct cpu_user_regs *regs);
>
> /* Functions for pending virtual abort checking window. */
> void abort_guest_exit_start(void);
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-15 16:39 ` Bertrand Marquis
@ 2022-08-15 17:04 ` Julien Grall
2022-08-16 7:28 ` Bertrand Marquis
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-08-15 17:04 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
On 15/08/2022 17:39, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,
>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Unlike arm64, on arm32 there are no extra information dumped (e.g.
>> page table walk) for hypervisor data abort.
>
> The code in arch/arm/traps.c has nothing arm32 specific like that so
> could you explain this statement ?
>
> Here the arm32 code will call the generic function which has only
> something specific for BRK handling but the rest is generic.
The statement is not related to the code but the console output. On
arm64, a data abort will decode the HSR and provide a dump of the
page-table walk.
This doesn't happen on arm32 because Xen will call do_unexpected_trap().
So the only information we have is the HSR and FAR. This is not very
helpful for debugging page-table walk.
After this patch, the same information will be printed on arm32 and arm64.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-15 17:04 ` Julien Grall
@ 2022-08-16 7:28 ` Bertrand Marquis
2022-08-31 19:17 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Bertrand Marquis @ 2022-08-16 7:28 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hi Julien,
> On 15 Aug 2022, at 18:04, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 15/08/2022 17:39, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi Bertrand,
>
>>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Unlike arm64, on arm32 there are no extra information dumped (e.g.
>>> page table walk) for hypervisor data abort.
>> The code in arch/arm/traps.c has nothing arm32 specific like that so
>> could you explain this statement ?
>> Here the arm32 code will call the generic function which has only
>> something specific for BRK handling but the rest is generic.
>
> The statement is not related to the code but the console output. On arm64, a data abort will decode the HSR and provide a dump of the page-table walk.
>
> This doesn't happen on arm32 because Xen will call do_unexpected_trap(). So the only information we have is the HSR and FAR. This is not very helpful for debugging page-table walk.
>
> After this patch, the same information will be printed on arm32 and arm64.
Ok then this is what I understood. Your commit message is maybe a bit unclear.
I would add a sentence like that: Call do_trap_hyp_sync for hypervisor data aborts on arm32 to have the same information than on arm64.
This can be done on commit so feel free to add my:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-16 7:28 ` Bertrand Marquis
@ 2022-08-31 19:17 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-31 19:17 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
On 16/08/2022 08:28, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,
>> On 15 Aug 2022, at 18:04, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 15/08/2022 17:39, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Unlike arm64, on arm32 there are no extra information dumped (e.g.
>>>> page table walk) for hypervisor data abort.
>>> The code in arch/arm/traps.c has nothing arm32 specific like that so
>>> could you explain this statement ?
>>> Here the arm32 code will call the generic function which has only
>>> something specific for BRK handling but the rest is generic.
>>
>> The statement is not related to the code but the console output. On arm64, a data abort will decode the HSR and provide a dump of the page-table walk.
>>
>> This doesn't happen on arm32 because Xen will call do_unexpected_trap(). So the only information we have is the HSR and FAR. This is not very helpful for debugging page-table walk.
>>
>> After this patch, the same information will be printed on arm32 and arm64.
>
> Ok then this is what I understood. Your commit message is maybe a bit unclear.
>
> I would add a sentence like that: Call do_trap_hyp_sync for hypervisor data aborts on arm32 to have the same information than on arm64.
Below the new commit message:
Unlike arm64, on arm32 there are no extra information dumped (e.g.
page table walk) for hypervisor data abort.
For data abort, the HSR will be set properly and so call
do_trap_hyp_sync() instead of do_unexpected_trap() on arm32 to have
the print the same information as arm64.
>
> This can be done on commit so feel free to add my:
>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Thanks! I have committed the series.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 8/8] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-12 19:24 [PATCH 0/7] xen/arm: More clean-ups and improvement Julien Grall
` (7 preceding siblings ...)
2022-08-12 19:24 ` [PATCH 7/7] xen/arm32: traps: Dump more information for hypervisor data abort Julien Grall
@ 2022-08-12 19:24 ` Julien Grall
2022-08-15 16:48 ` Julien Grall
8 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-08-12 19:24 UTC (permalink / raw)
To: xen-devel
Cc: julien, Julien Grall, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
From: Julien Grall <jgrall@amazon.com>
Unlike arm64, on arm32 there are no extra information dumped (e.g.
page table walk) for hypervisor data abort.
For data abort, the HSR will be set properly and so replace the call
to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
xen/arch/arm/arm32/traps.c | 2 +-
xen/arch/arm/include/asm/traps.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index a4ce2b92d904..a2fc1c22cbc9 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
if ( VABORT_GEN_BY_GUEST(regs) )
do_trap_guest_serror(regs);
else
- do_unexpected_trap("Data Abort", regs);
+ do_trap_hyp_sync(regs);
}
void finalize_instr_emulation(const struct instr_details *instr)
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 08bc0b484c75..883dae368eac 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
void noreturn do_unexpected_trap(const char *msg,
const struct cpu_user_regs *regs);
+void do_trap_hyp_sync(struct cpu_user_regs *regs);
/* Functions for pending virtual abort checking window. */
void abort_guest_exit_start(void);
--
2.37.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 8/8] xen/arm32: traps: Dump more information for hypervisor data abort
2022-08-12 19:24 ` [PATCH 8/8] " Julien Grall
@ 2022-08-15 16:48 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-08-15 16:48 UTC (permalink / raw)
To: xen-devel
Cc: Julien Grall, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi all,
Please ignore this patch. This is a duplication of patch #7.
Cheers,
On 12/08/2022 20:24, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> Unlike arm64, on arm32 there are no extra information dumped (e.g.
> page table walk) for hypervisor data abort.
>
> For data abort, the HSR will be set properly and so replace the call
> to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
> xen/arch/arm/arm32/traps.c | 2 +-
> xen/arch/arm/include/asm/traps.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index a4ce2b92d904..a2fc1c22cbc9 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
> if ( VABORT_GEN_BY_GUEST(regs) )
> do_trap_guest_serror(regs);
> else
> - do_unexpected_trap("Data Abort", regs);
> + do_trap_hyp_sync(regs);
> }
>
> void finalize_instr_emulation(const struct instr_details *instr)
> diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
> index 08bc0b484c75..883dae368eac 100644
> --- a/xen/arch/arm/include/asm/traps.h
> +++ b/xen/arch/arm/include/asm/traps.h
> @@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc);
>
> void noreturn do_unexpected_trap(const char *msg,
> const struct cpu_user_regs *regs);
> +void do_trap_hyp_sync(struct cpu_user_regs *regs);
>
> /* Functions for pending virtual abort checking window. */
> void abort_guest_exit_start(void);
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread