[PATCH v2] xen:arm: Populate arm64 image header

[PATCH v2] xen:arm: Populate arm64 image header

[Amit Singh Tomar]

This patch adds image size and flags to XEN image header. It uses
those fields according to the updated Linux kernel image definition.

With this patch bootloader can now place XEN image anywhere in system
RAM at 2MB aligned address without to worry about relocation.
For instance, it fixes the XEN boot on Amlogic SoC where bootloader(U-BOOT)
always relocates the XEN image to an address range reserved for firmware data.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
Changes since v1:
        * Updated commit message
        * Removed endianess code
---
 xen/arch/arm/arm64/head.S          | 5 +++--
 xen/arch/arm/arm64/lib/assembler.h | 9 +++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index d63734f..8e35968 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -25,6 +25,7 @@
 #include <asm/early_printk.h>
 #include <efi/efierr.h>
 #include <asm/arm64/efibind.h>
+#include "lib/assembler.h"
 
 #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
@@ -120,8 +121,8 @@ efi_head:
         add     x13, x18, #0x16
         b       real_start           /* branch to kernel start */
         .quad   0                    /* Image load offset from start of RAM */
-        .quad   0                    /* reserved */
-        .quad   0                    /* reserved */
+        .quad   __KERNEL_SIZE        /* Effective size of kernel image, little-endian */
+        .quad   __HEAD_FLAGS         /* Informative flags, little-endian */
         .quad   0                    /* reserved */
         .quad   0                    /* reserved */
         .quad   0                    /* reserved */
diff --git a/xen/arch/arm/arm64/lib/assembler.h b/xen/arch/arm/arm64/lib/assembler.h
index 3f9c0dc..7239c19 100644
--- a/xen/arch/arm/arm64/lib/assembler.h
+++ b/xen/arch/arm/arm64/lib/assembler.h
@@ -9,4 +9,13 @@
 #define CPU_BE(x...)
 #define CPU_LE(x...) x
 
+#define __HEAD_FLAG_PAGE_SIZE   1 /* 4K hard-coded */
+
+#define __HEAD_FLAG_PHYS_BASE   1
+
+#define __HEAD_FLAGS            ((__HEAD_FLAG_PAGE_SIZE << 1) |  \
+                                (__HEAD_FLAG_PHYS_BASE << 3))
+
+#define __KERNEL_SIZE           (_end - start)
+
 #endif /* __ASM_ASSEMBLER_H__ */
-- 
1.9.1


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

Re: [PATCH v2] xen:arm: Populate arm64 image header

[Andre Przywara]

Hi,

On 04/09/18 18:25, Amit Singh Tomar wrote:
> This patch adds image size and flags to XEN image header. It uses
> those fields according to the updated Linux kernel image definition.
> 
> With this patch bootloader can now place XEN image anywhere in system
> RAM at 2MB aligned address without to worry about relocation.
> For instance, it fixes the XEN boot on Amlogic SoC where bootloader(U-BOOT)
> always relocates the XEN image to an address range reserved for firmware data.

This message looks good to me now.

> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
> Changes since v1:
>         * Updated commit message
>         * Removed endianess code
> ---
>  xen/arch/arm/arm64/head.S          | 5 +++--
>  xen/arch/arm/arm64/lib/assembler.h | 9 +++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d63734f..8e35968 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -25,6 +25,7 @@
>  #include <asm/early_printk.h>
>  #include <efi/efierr.h>
>  #include <asm/arm64/efibind.h>
> +#include "lib/assembler.h"
>  
>  #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
>  #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
> @@ -120,8 +121,8 @@ efi_head:
>          add     x13, x18, #0x16
>          b       real_start           /* branch to kernel start */
>          .quad   0                    /* Image load offset from start of RAM */
> -        .quad   0                    /* reserved */
> -        .quad   0                    /* reserved */
> +        .quad   __KERNEL_SIZE        /* Effective size of kernel image, little-endian */

I don't think it's helpful to hide that KERNEL_SIZE definition in
another file. Please just put "_end - start" here.

> +        .quad   __HEAD_FLAGS         /* Informative flags, little-endian */

and I don't see much sense either in having those defined in
assembler.h, since it is only useful in this very file.
So just have the definitions somewhere at the top of head.S.

>          .quad   0                    /* reserved */
>          .quad   0                    /* reserved */
>          .quad   0                    /* reserved */
> diff --git a/xen/arch/arm/arm64/lib/assembler.h b/xen/arch/arm/arm64/lib/assembler.h
> index 3f9c0dc..7239c19 100644
> --- a/xen/arch/arm/arm64/lib/assembler.h
> +++ b/xen/arch/arm/arm64/lib/assembler.h
> @@ -9,4 +9,13 @@
>  #define CPU_BE(x...)
>  #define CPU_LE(x...) x
>  
> +#define __HEAD_FLAG_PAGE_SIZE   1 /* 4K hard-coded */

Either you call this __HEAD_FLAG_PAGE_SIZE_4K, or, even better, copy the
Linux definition, which would make it obvious where this comes from and
would alert any developer of the PAGE_SIZE dependency.

Plus move those into head.S, as mentioned above.

Cheers,
Andre.

> +
> +#define __HEAD_FLAG_PHYS_BASE   1
> +
> +#define __HEAD_FLAGS            ((__HEAD_FLAG_PAGE_SIZE << 1) |  \
> +                                (__HEAD_FLAG_PHYS_BASE << 3))
> +
> +#define __KERNEL_SIZE           (_end - start)
> +
>  #endif /* __ASM_ASSEMBLER_H__ */
> 

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

Re: [PATCH v2] xen:arm: Populate arm64 image header

[Amit Tomer]

Hello,

Thanks for having a look.

On Wed, Sep 5, 2018 at 6:07 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi,
>
> I don't think it's helpful to hide that KERNEL_SIZE definition in
> another file. Please just put "_end - start" here.

Yeah,  I thought about it and felt that it would be cleaner and more
readable if we use macros here.

Also, wanted to have minimal changes in head.S and that is the reason
for using assemble.h.

> Either you call this __HEAD_FLAG_PAGE_SIZE_4K, or, even better, copy the
> Linux definition, which would make it obvious where this comes from and
> would alert any developer of the PAGE_SIZE dependency.
>
> Plus move those into head.S, as mentioned above.

Ok.
> > +
> > +#define __KERNEL_SIZE           (_end - start)
> > +
> >  #endif /* __ASM_ASSEMBLER_H__ */
> >

Thanks
-Amit

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

Re: [PATCH v2] xen:arm: Populate arm64 image header

[Andre Przywara]

Hi,

On 05/09/18 13:52, Amit Tomer wrote:
> Hello,
> 
> Thanks for having a look.
> 
> On Wed, Sep 5, 2018 at 6:07 PM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Hi,
>>
>> I don't think it's helpful to hide that KERNEL_SIZE definition in
>> another file. Please just put "_end - start" here.
> 
> Yeah,  I thought about it and felt that it would be cleaner and more
> readable if we use macros here.

I don't see how this improves readability.
The point is that this line as is:

 .quad __KERNEL_SIZE /* Effective size of kernel image, little-endian */

conveys only little information, basically you say that "kernel size is
kernel size". I would rather see the actual definition of kernel size
here, since the comment explains the meaning already.

> Also, wanted to have minimal changes in head.S and that is the reason
> for using assemble.h.

assembler.h wouldn't be the right place anyway. AFAICT it's only there
to make those files in lib/ happy, which are mostly copied from Linux.

Cheers,
Andre.

>> Either you call this __HEAD_FLAG_PAGE_SIZE_4K, or, even better, copy the
>> Linux definition, which would make it obvious where this comes from and
>> would alert any developer of the PAGE_SIZE dependency.
>>
>> Plus move those into head.S, as mentioned above.
> 
> Ok.
>>> +
>>> +#define __KERNEL_SIZE           (_end - start)
>>> +
>>>  #endif /* __ASM_ASSEMBLER_H__ */
>>>
> 
> Thanks
> -Amit
> 

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