Re: [Xen-devel] [PATCH 2/2] xen/arm64: use shift operator
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 FanCc: Stefano Stabellini Cc: Julien Grall --- 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
Re: [Xen-devel] [PATCH 2/2] xen/arm64: use shift operator
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 FanCc: Stefano Stabellini Cc: Julien Grall --- 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
Re: [Xen-devel] [PATCH 2/2] xen/arm64: use shift operator
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>>Cc: Stefano Stabellini >>Cc: Julien Grall >>--- >> 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
Re: [Xen-devel] [PATCH 2/2] xen/arm64: use shift operator
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 FanCc: Stefano Stabellini Cc: Julien Grall --- 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
[Xen-devel] [PATCH 2/2] xen/arm64: use shift operator
Use shift operator, but not muliplication. No function change. Signed-off-by: Peng FanCc: Stefano Stabellini Cc: Julien Grall --- 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