* [PATCH 1/2] xen/arm64: correct comments
@ 2016-04-20 13:54 Peng Fan
2016-04-20 13:54 ` [PATCH 2/2] xen/arm64: use shift operator Peng Fan
2016-04-22 16:24 ` [PATCH 1/2] xen/arm64: correct comments Julien Grall
0 siblings, 2 replies; 7+ messages in thread
From: Peng Fan @ 2016-04-20 13:54 UTC (permalink / raw)
To: xen-devel; +Cc: van.freenix, Julien Grall, Stefano Stabellini
The 'Base address for 4K mapping' is '(x19 >> THIRD_SHIFT) << THIRD_SHIFT'.
Also we are building 4K page mapping, not section mapping.
Signed-off-by: Peng Fan <van.freenix@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/arm64/head.S | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 946e2c9..05e3db0 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -471,10 +471,10 @@ skip_bss:
ldr x4, =boot_third
add x4, x4, x20 /* x4 := paddr (boot_third) */
- lsr x2, x19, #THIRD_SHIFT /* Base address for 4K mapping */
- lsl x2, x2, #THIRD_SHIFT
- mov x3, #PT_MEM_L3 /* x2 := Section map */
- orr x2, x2, x3
+ lsr x2, x19, #THIRD_SHIFT
+ lsl x2, x2, #THIRD_SHIFT /* Base address for 4K mapping */
+ mov x3, #PT_MEM_L3
+ orr x2, x2, x3 /* x2 := Page map */
/* ... map of vaddr(start) in boot_third */
mov x1, xzr
--
2.6.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] xen/arm64: use shift operator
2016-04-20 13:54 [PATCH 1/2] xen/arm64: correct comments Peng Fan
@ 2016-04-20 13:54 ` Peng Fan
2016-04-20 14:44 ` Julien Grall
2016-04-22 16:24 ` [PATCH 1/2] xen/arm64: correct comments Julien Grall
1 sibling, 1 reply; 7+ messages in thread
From: Peng Fan @ 2016-04-20 13:54 UTC (permalink / raw)
To: xen-devel; +Cc: van.freenix, Julien Grall, Stefano Stabellini
Use shift operator, but not muliplication.
No function change.
Signed-off-by: Peng Fan <van.freenix@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/arm64/head.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 05e3db0..ad6e593 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -524,7 +524,7 @@ paging:
lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */
mov x3, #PT_DEV_L3
orr x2, x2, x3 /* x2 := 4K dev map including UART */
- str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
+ str x2, [x1, #(FIXMAP_CONSOLE << 3)] /* Map it in the first fixmap's slot */
1:
/* Map fixmap into boot_second */
--
2.6.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] xen/arm64: use shift operator
2016-04-20 13:54 ` [PATCH 2/2] xen/arm64: use shift operator Peng Fan
@ 2016-04-20 14:44 ` Julien Grall
2016-04-21 1:06 ` Peng Fan
0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2016-04-20 14:44 UTC (permalink / raw)
To: Peng Fan, xen-devel; +Cc: Stefano Stabellini
Hello Peng,
On 20/04/16 14:54, Peng Fan wrote:
> Use shift operator, but not muliplication.
> No function change.
Why? The compiler will calculate the address at compilation time.
Regards,
> Signed-off-by: Peng Fan <van.freenix@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/arm64/head.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 05e3db0..ad6e593 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -524,7 +524,7 @@ paging:
> lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */
> mov x3, #PT_DEV_L3
> orr x2, x2, x3 /* x2 := 4K dev map including UART */
> - str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
> + str x2, [x1, #(FIXMAP_CONSOLE << 3)] /* Map it in the first fixmap's slot */
> 1:
>
> /* Map fixmap into boot_second */
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] xen/arm64: use shift operator
2016-04-20 14:44 ` Julien Grall
@ 2016-04-21 1:06 ` Peng Fan
2016-04-22 16:08 ` Julien Grall
0 siblings, 1 reply; 7+ messages in thread
From: Peng Fan @ 2016-04-21 1:06 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, xen-devel
Hi Julien,
On Wed, Apr 20, 2016 at 03:44:09PM +0100, Julien Grall wrote:
>Hello Peng,
>
>On 20/04/16 14:54, Peng Fan wrote:
>>Use shift operator, but not muliplication.
>>No function change.
>
>Why? The compiler will calculate the address at compilation time.
Yeah. The compiler will do this. I just think in asm, we rarely use
multiplication. In this file, there is "cmp x1, #(LPAE_ENTRIES<<3)",
here use shift operator but not multiplication.
Thanks,
Peng.
>
>Regards,
>
>>Signed-off-by: Peng Fan <van.freenix@gmail.com>
>>Cc: Stefano Stabellini <sstabellini@kernel.org>
>>Cc: Julien Grall <julien.grall@arm.com>
>>---
>> xen/arch/arm/arm64/head.S | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>index 05e3db0..ad6e593 100644
>>--- a/xen/arch/arm/arm64/head.S
>>+++ b/xen/arch/arm/arm64/head.S
>>@@ -524,7 +524,7 @@ paging:
>> lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */
>> mov x3, #PT_DEV_L3
>> orr x2, x2, x3 /* x2 := 4K dev map including UART */
>>- str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>>+ str x2, [x1, #(FIXMAP_CONSOLE << 3)] /* Map it in the first fixmap's slot */
>> 1:
>>
>> /* Map fixmap into boot_second */
>>
>
>--
>Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] xen/arm64: use shift operator
2016-04-21 1:06 ` Peng Fan
@ 2016-04-22 16:08 ` Julien Grall
2016-04-23 13:28 ` Peng Fan
0 siblings, 1 reply; 7+ messages in thread
From: Julien Grall @ 2016-04-22 16:08 UTC (permalink / raw)
To: Peng Fan; +Cc: Stefano Stabellini, xen-devel
On 21/04/16 02:06, Peng Fan wrote:
> Hi Julien,
Hello Peng,
> On Wed, Apr 20, 2016 at 03:44:09PM +0100, Julien Grall wrote:
>> Hello Peng,
>>
>> On 20/04/16 14:54, Peng Fan wrote:
>>> Use shift operator, but not muliplication.
>>> No function change.
>>
>> Why? The compiler will calculate the address at compilation time.
>
> Yeah. The compiler will do this. I just think in asm, we rarely use
> multiplication. In this file, there is "cmp x1, #(LPAE_ENTRIES<<3)",
> here use shift operator but not multiplication.
It took me a bit of time to understand that 8 is the number of byte of
an LPAE entries. IMHO the "<< 3" would be more confusing.
So I would introduce a macro to get the slot based of an index.
Something like:
LPAE_SLOT(slot) ((slot) * 8)
And add a comment to explain why 8. You could even introduce
LPAE_ENTRY_SIZE.
Regards,
> Thanks,
> Peng.
>
>>
>> Regards,
>>
>>> Signed-off-by: Peng Fan <van.freenix@gmail.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> ---
>>> xen/arch/arm/arm64/head.S | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index 05e3db0..ad6e593 100644
>>> --- a/xen/arch/arm/arm64/head.S
>>> +++ b/xen/arch/arm/arm64/head.S
>>> @@ -524,7 +524,7 @@ paging:
>>> lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */
>>> mov x3, #PT_DEV_L3
>>> orr x2, x2, x3 /* x2 := 4K dev map including UART */
>>> - str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>>> + str x2, [x1, #(FIXMAP_CONSOLE << 3)] /* Map it in the first fixmap's slot */
>>> 1:
>>>
>>> /* Map fixmap into boot_second */
>>>
>>
>> --
>> Julien Grall
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] xen/arm64: correct comments
2016-04-20 13:54 [PATCH 1/2] xen/arm64: correct comments Peng Fan
2016-04-20 13:54 ` [PATCH 2/2] xen/arm64: use shift operator Peng Fan
@ 2016-04-22 16:24 ` Julien Grall
1 sibling, 0 replies; 7+ messages in thread
From: Julien Grall @ 2016-04-22 16:24 UTC (permalink / raw)
To: Peng Fan, xen-devel; +Cc: Stefano Stabellini
Hello Peng,
I would specific the file modified in the title.
On 20/04/16 14:54, Peng Fan wrote:
> The 'Base address for 4K mapping' is '(x19 >> THIRD_SHIFT) << THIRD_SHIFT'.
The computation is a sequence of 2 instructions. I gave a look to the
rest of the file and the comments are usually put on the first instruction.
So I would much prefer to stay consistent with the rest of the code.
> Also we are building 4K page mapping, not section mapping.
To be fair, a page is a specialized section.
> Signed-off-by: Peng Fan <van.freenix@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> xen/arch/arm/arm64/head.S | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 946e2c9..05e3db0 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -471,10 +471,10 @@ skip_bss:
> ldr x4, =boot_third
> add x4, x4, x20 /* x4 := paddr (boot_third) */
>
> - lsr x2, x19, #THIRD_SHIFT /* Base address for 4K mapping */
> - lsl x2, x2, #THIRD_SHIFT
> - mov x3, #PT_MEM_L3 /* x2 := Section map */
> - orr x2, x2, x3
> + lsr x2, x19, #THIRD_SHIFT
> + lsl x2, x2, #THIRD_SHIFT /* Base address for 4K mapping */
> + mov x3, #PT_MEM_L3
> + orr x2, x2, x3 /* x2 := Page map */
The section map is the combination of 2 instructions. With this change
we don't know quickly what the purpose of "mov x3, #PT_MEM_L3".
>
> /* ... map of vaddr(start) in boot_third */
> mov x1, xzr
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] xen/arm64: use shift operator
2016-04-22 16:08 ` Julien Grall
@ 2016-04-23 13:28 ` Peng Fan
0 siblings, 0 replies; 7+ messages in thread
From: Peng Fan @ 2016-04-23 13:28 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, xen-devel
Hi Julien,
On Fri, Apr 22, 2016 at 05:08:26PM +0100, Julien Grall wrote:
>On 21/04/16 02:06, Peng Fan wrote:
>>Hi Julien,
>
>Hello Peng,
>
>>On Wed, Apr 20, 2016 at 03:44:09PM +0100, Julien Grall wrote:
>>>Hello Peng,
>>>
>>>On 20/04/16 14:54, Peng Fan wrote:
>>>>Use shift operator, but not muliplication.
>>>>No function change.
>>>
>>>Why? The compiler will calculate the address at compilation time.
>>
>>Yeah. The compiler will do this. I just think in asm, we rarely use
>>multiplication. In this file, there is "cmp x1, #(LPAE_ENTRIES<<3)",
>>here use shift operator but not multiplication.
>
>It took me a bit of time to understand that 8 is the number of byte of an
>LPAE entries. IMHO the "<< 3" would be more confusing.
>
>So I would introduce a macro to get the slot based of an index. Something
>like:
>
>LPAE_SLOT(slot) ((slot) * 8)
>
>And add a comment to explain why 8. You could even introduce LPAE_ENTRY_SIZE.
Thanks for comments. I'll try to submit a new patch following your suggestion.
Thanks,
Peng.
>
>Regards,
>
>>Thanks,
>>Peng.
>>
>>>
>>>Regards,
>>>
>>>>Signed-off-by: Peng Fan <van.freenix@gmail.com>
>>>>Cc: Stefano Stabellini <sstabellini@kernel.org>
>>>>Cc: Julien Grall <julien.grall@arm.com>
>>>>---
>>>> xen/arch/arm/arm64/head.S | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>>diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>>>index 05e3db0..ad6e593 100644
>>>>--- a/xen/arch/arm/arm64/head.S
>>>>+++ b/xen/arch/arm/arm64/head.S
>>>>@@ -524,7 +524,7 @@ paging:
>>>> lsl x2, x2, #THIRD_SHIFT /* 4K aligned paddr of UART */
>>>> mov x3, #PT_DEV_L3
>>>> orr x2, x2, x3 /* x2 := 4K dev map including UART */
>>>>- str x2, [x1, #(FIXMAP_CONSOLE*8)] /* Map it in the first fixmap's slot */
>>>>+ str x2, [x1, #(FIXMAP_CONSOLE << 3)] /* Map it in the first fixmap's slot */
>>>> 1:
>>>>
>>>> /* Map fixmap into boot_second */
>>>>
>>>
>>>--
>>>Julien Grall
>>
>
>--
>Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-23 13:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-20 13:54 [PATCH 1/2] xen/arm64: correct comments Peng Fan
2016-04-20 13:54 ` [PATCH 2/2] xen/arm64: use shift operator Peng Fan
2016-04-20 14:44 ` Julien Grall
2016-04-21 1:06 ` Peng Fan
2016-04-22 16:08 ` Julien Grall
2016-04-23 13:28 ` Peng Fan
2016-04-22 16:24 ` [PATCH 1/2] xen/arm64: correct comments Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.