| Message ID | 1461160464-2194-2-git-send-email-van.freenix@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */ >
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
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 >
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
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 */
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(-)