[PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Oliver O'Halloran]

When booting under OF the zImage expects the initrd address and size to be
passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
currently doesn't do this so the zImage is not aware of the initrd
location.  This can result in initrd corruption either though the zImage
extracting the vmlinux over the initrd, or by the vmlinux overwriting the
initrd when relocating itself.

QEMU does put the linux,initrd-start and linux,initrd-end properties into
the devicetree to vmlinux to find the initrd. We can work around the SLOF
bug by also looking those properties in the zImage.

Cc: stable@vger.kernel.org
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
First noticed here: https://unix.stackexchange.com/questions/547023/linux-kernel-on-ppc64le-vmlinux-equivalent-in-arch-powerpc-boot
---
 arch/powerpc/boot/devtree.c | 21 +++++++++++++++++++++
 arch/powerpc/boot/main.c    |  7 +++++++
 arch/powerpc/boot/of.h      | 16 ----------------
 arch/powerpc/boot/ops.h     |  1 +
 arch/powerpc/boot/swab.h    | 17 +++++++++++++++++
 5 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
index 5d91036..ac5c26b 100644
--- a/arch/powerpc/boot/devtree.c
+++ b/arch/powerpc/boot/devtree.c
@@ -13,6 +13,7 @@
 #include "string.h"
 #include "stdio.h"
 #include "ops.h"
+#include "swab.h"
 
 void dt_fixup_memory(u64 start, u64 size)
 {
@@ -318,6 +319,26 @@ int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size)
 	return dt_xlate(node, res, reglen, addr, size);
 }
 
+int dt_read_addr(void *node, const char *prop, unsigned long *out_addr)
+{
+	int reglen;
+
+	*out_addr = 0;
+
+	reglen = getprop(node, prop, prop_buf, sizeof(prop_buf)) / 4;
+	if (reglen == 2) {
+		u64 v0 = be32_to_cpu(prop_buf[0]);
+		u64 v1 = be32_to_cpu(prop_buf[1]);
+		*out_addr = (v0 << 32) | v1;
+	} else if (reglen == 1) {
+		*out_addr = be32_to_cpu(prop_buf[0]);
+	} else {
+		return 0;
+	}
+
+	return 1;
+}
+
 int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr)
 {
 
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index a9d2091..518af24 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -112,6 +112,13 @@ static struct addr_range prep_initrd(struct addr_range vmlinux, void *chosen,
 	} else if (initrd_size > 0) {
 		printf("Using loader supplied ramdisk at 0x%lx-0x%lx\n\r",
 		       initrd_addr, initrd_addr + initrd_size);
+	} else if (chosen) {
+		unsigned long initrd_end;
+
+		dt_read_addr(chosen, "linux,initrd-start", &initrd_addr);
+		dt_read_addr(chosen, "linux,initrd-end", &initrd_end);
+
+		initrd_size = initrd_end - initrd_addr;
 	}
 
 	/* If there's no initrd at all, we're done */
diff --git a/arch/powerpc/boot/of.h b/arch/powerpc/boot/of.h
index 31b2f5d..dc24770 100644
--- a/arch/powerpc/boot/of.h
+++ b/arch/powerpc/boot/of.h
@@ -26,22 +26,6 @@ typedef u16			__be16;
 typedef u32			__be32;
 typedef u64			__be64;
 
-#ifdef __LITTLE_ENDIAN__
-#define cpu_to_be16(x) swab16(x)
-#define be16_to_cpu(x) swab16(x)
-#define cpu_to_be32(x) swab32(x)
-#define be32_to_cpu(x) swab32(x)
-#define cpu_to_be64(x) swab64(x)
-#define be64_to_cpu(x) swab64(x)
-#else
-#define cpu_to_be16(x) (x)
-#define be16_to_cpu(x) (x)
-#define cpu_to_be32(x) (x)
-#define be32_to_cpu(x) (x)
-#define cpu_to_be64(x) (x)
-#define be64_to_cpu(x) (x)
-#endif
-
 #define PROM_ERROR (-1u)
 
 #endif /* _PPC_BOOT_OF_H_ */
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index e060676..5100dd7 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -95,6 +95,7 @@ void *simple_alloc_init(char *base, unsigned long heap_size,
 extern void flush_cache(void *, unsigned long);
 int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr);
+int dt_read_addr(void *node, const char *prop, unsigned long *out);
 int dt_is_compatible(void *node, const char *compat);
 void dt_get_reg_format(void *node, u32 *naddr, u32 *nsize);
 int dt_get_virtual_reg(void *node, void **addr, int nres);
diff --git a/arch/powerpc/boot/swab.h b/arch/powerpc/boot/swab.h
index 11d2069..82db2c1 100644
--- a/arch/powerpc/boot/swab.h
+++ b/arch/powerpc/boot/swab.h
@@ -27,4 +27,21 @@ static inline u64 swab64(u64 x)
 		(u64)((x & (u64)0x00ff000000000000ULL) >> 40) |
 		(u64)((x & (u64)0xff00000000000000ULL) >> 56);
 }
+
+#ifdef __LITTLE_ENDIAN__
+#define cpu_to_be16(x) swab16(x)
+#define be16_to_cpu(x) swab16(x)
+#define cpu_to_be32(x) swab32(x)
+#define be32_to_cpu(x) swab32(x)
+#define cpu_to_be64(x) swab64(x)
+#define be64_to_cpu(x) swab64(x)
+#else
+#define cpu_to_be16(x) (x)
+#define be16_to_cpu(x) (x)
+#define cpu_to_be32(x) (x)
+#define be32_to_cpu(x) (x)
+#define cpu_to_be64(x) (x)
+#define be64_to_cpu(x) (x)
+#endif
+
 #endif /* _PPC_BOOT_SWAB_H_ */
-- 
2.9.5

[PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Oliver O'Halloran]

When booting under OF the zImage expects the initrd address and size to be
passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
currently doesn't do this so the zImage is not aware of the initrd
location.  This can result in initrd corruption either though the zImage
extracting the vmlinux over the initrd, or by the vmlinux overwriting the
initrd when relocating itself.

QEMU does put the linux,initrd-start and linux,initrd-end properties into
the devicetree to vmlinux to find the initrd. We can work around the SLOF
bug by also looking those properties in the zImage.

Cc: stable@vger.kernel.org
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
First noticed here: https://unix.stackexchange.com/questions/547023/linux-kernel-on-ppc64le-vmlinux-equivalent-in-arch-powerpc-boot
---
 arch/powerpc/boot/devtree.c | 21 +++++++++++++++++++++
 arch/powerpc/boot/main.c    |  7 +++++++
 arch/powerpc/boot/of.h      | 16 ----------------
 arch/powerpc/boot/ops.h     |  1 +
 arch/powerpc/boot/swab.h    | 17 +++++++++++++++++
 5 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
index 5d91036..ac5c26b 100644
--- a/arch/powerpc/boot/devtree.c
+++ b/arch/powerpc/boot/devtree.c
@@ -13,6 +13,7 @@
 #include "string.h"
 #include "stdio.h"
 #include "ops.h"
+#include "swab.h"
 
 void dt_fixup_memory(u64 start, u64 size)
 {
@@ -318,6 +319,26 @@ int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size)
 	return dt_xlate(node, res, reglen, addr, size);
 }
 
+int dt_read_addr(void *node, const char *prop, unsigned long *out_addr)
+{
+	int reglen;
+
+	*out_addr = 0;
+
+	reglen = getprop(node, prop, prop_buf, sizeof(prop_buf)) / 4;
+	if (reglen == 2) {
+		u64 v0 = be32_to_cpu(prop_buf[0]);
+		u64 v1 = be32_to_cpu(prop_buf[1]);
+		*out_addr = (v0 << 32) | v1;
+	} else if (reglen == 1) {
+		*out_addr = be32_to_cpu(prop_buf[0]);
+	} else {
+		return 0;
+	}
+
+	return 1;
+}
+
 int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr)
 {
 
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index a9d2091..518af24 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -112,6 +112,13 @@ static struct addr_range prep_initrd(struct addr_range vmlinux, void *chosen,
 	} else if (initrd_size > 0) {
 		printf("Using loader supplied ramdisk at 0x%lx-0x%lx\n\r",
 		       initrd_addr, initrd_addr + initrd_size);
+	} else if (chosen) {
+		unsigned long initrd_end;
+
+		dt_read_addr(chosen, "linux,initrd-start", &initrd_addr);
+		dt_read_addr(chosen, "linux,initrd-end", &initrd_end);
+
+		initrd_size = initrd_end - initrd_addr;
 	}
 
 	/* If there's no initrd at all, we're done */
diff --git a/arch/powerpc/boot/of.h b/arch/powerpc/boot/of.h
index 31b2f5d..dc24770 100644
--- a/arch/powerpc/boot/of.h
+++ b/arch/powerpc/boot/of.h
@@ -26,22 +26,6 @@ typedef u16			__be16;
 typedef u32			__be32;
 typedef u64			__be64;
 
-#ifdef __LITTLE_ENDIAN__
-#define cpu_to_be16(x) swab16(x)
-#define be16_to_cpu(x) swab16(x)
-#define cpu_to_be32(x) swab32(x)
-#define be32_to_cpu(x) swab32(x)
-#define cpu_to_be64(x) swab64(x)
-#define be64_to_cpu(x) swab64(x)
-#else
-#define cpu_to_be16(x) (x)
-#define be16_to_cpu(x) (x)
-#define cpu_to_be32(x) (x)
-#define be32_to_cpu(x) (x)
-#define cpu_to_be64(x) (x)
-#define be64_to_cpu(x) (x)
-#endif
-
 #define PROM_ERROR (-1u)
 
 #endif /* _PPC_BOOT_OF_H_ */
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index e060676..5100dd7 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -95,6 +95,7 @@ void *simple_alloc_init(char *base, unsigned long heap_size,
 extern void flush_cache(void *, unsigned long);
 int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr);
+int dt_read_addr(void *node, const char *prop, unsigned long *out);
 int dt_is_compatible(void *node, const char *compat);
 void dt_get_reg_format(void *node, u32 *naddr, u32 *nsize);
 int dt_get_virtual_reg(void *node, void **addr, int nres);
diff --git a/arch/powerpc/boot/swab.h b/arch/powerpc/boot/swab.h
index 11d2069..82db2c1 100644
--- a/arch/powerpc/boot/swab.h
+++ b/arch/powerpc/boot/swab.h
@@ -27,4 +27,21 @@ static inline u64 swab64(u64 x)
 		(u64)((x & (u64)0x00ff000000000000ULL) >> 40) |
 		(u64)((x & (u64)0xff00000000000000ULL) >> 56);
 }
+
+#ifdef __LITTLE_ENDIAN__
+#define cpu_to_be16(x) swab16(x)
+#define be16_to_cpu(x) swab16(x)
+#define cpu_to_be32(x) swab32(x)
+#define be32_to_cpu(x) swab32(x)
+#define cpu_to_be64(x) swab64(x)
+#define be64_to_cpu(x) swab64(x)
+#else
+#define cpu_to_be16(x) (x)
+#define be16_to_cpu(x) (x)
+#define cpu_to_be32(x) (x)
+#define be32_to_cpu(x) (x)
+#define cpu_to_be64(x) (x)
+#define be64_to_cpu(x) (x)
+#endif
+
 #endif /* _PPC_BOOT_SWAB_H_ */
-- 
2.9.5

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Alexey Kardashevskiy]

On 23/10/2019 12:36, Oliver O'Halloran wrote:
> When booting under OF the zImage expects the initrd address and size to be
> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> currently doesn't do this so the zImage is not aware of the initrd
> location.  This can result in initrd corruption either though the zImage
> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> initrd when relocating itself.
> 
> QEMU does put the linux,initrd-start and linux,initrd-end properties into
> the devicetree to vmlinux to find the initrd. We can work around the SLOF
> bug by also looking those properties in the zImage.

This does not boot zImage for me anyway:

Trying to unpack rootfs image as initramfs...
rootfs image is not initramfs (invalid magic at start of compressed archive); looks like an initrd



> 
> Cc: stable@vger.kernel.org
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> First noticed here: https://unix.stackexchange.com/questions/547023/linux-kernel-on-ppc64le-vmlinux-equivalent-in-arch-powerpc-boot
> ---
>  arch/powerpc/boot/devtree.c | 21 +++++++++++++++++++++
>  arch/powerpc/boot/main.c    |  7 +++++++
>  arch/powerpc/boot/of.h      | 16 ----------------
>  arch/powerpc/boot/ops.h     |  1 +
>  arch/powerpc/boot/swab.h    | 17 +++++++++++++++++
>  5 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
> index 5d91036..ac5c26b 100644
> --- a/arch/powerpc/boot/devtree.c
> +++ b/arch/powerpc/boot/devtree.c
> @@ -13,6 +13,7 @@
>  #include "string.h"
>  #include "stdio.h"
>  #include "ops.h"
> +#include "swab.h"
>  
>  void dt_fixup_memory(u64 start, u64 size)
>  {
> @@ -318,6 +319,26 @@ int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size)
>  	return dt_xlate(node, res, reglen, addr, size);
>  }
>  
> +int dt_read_addr(void *node, const char *prop, unsigned long *out_addr)
> +{
> +	int reglen;
> +
> +	*out_addr = 0;
> +
> +	reglen = getprop(node, prop, prop_buf, sizeof(prop_buf)) / 4;
> +	if (reglen == 2) {
> +		u64 v0 = be32_to_cpu(prop_buf[0]);
> +		u64 v1 = be32_to_cpu(prop_buf[1]);
> +		*out_addr = (v0 << 32) | v1;
> +	} else if (reglen == 1) {
> +		*out_addr = be32_to_cpu(prop_buf[0]);
> +	} else {
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr)
>  {
>  
> diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
> index a9d2091..518af24 100644
> --- a/arch/powerpc/boot/main.c
> +++ b/arch/powerpc/boot/main.c
> @@ -112,6 +112,13 @@ static struct addr_range prep_initrd(struct addr_range vmlinux, void *chosen,
>  	} else if (initrd_size > 0) {
>  		printf("Using loader supplied ramdisk at 0x%lx-0x%lx\n\r",
>  		       initrd_addr, initrd_addr + initrd_size);
> +	} else if (chosen) {
> +		unsigned long initrd_end;
> +
> +		dt_read_addr(chosen, "linux,initrd-start", &initrd_addr);
> +		dt_read_addr(chosen, "linux,initrd-end", &initrd_end);
> +
> +		initrd_size = initrd_end - initrd_addr;
>  	}
>  
>  	/* If there's no initrd at all, we're done */
> diff --git a/arch/powerpc/boot/of.h b/arch/powerpc/boot/of.h
> index 31b2f5d..dc24770 100644
> --- a/arch/powerpc/boot/of.h
> +++ b/arch/powerpc/boot/of.h
> @@ -26,22 +26,6 @@ typedef u16			__be16;
>  typedef u32			__be32;
>  typedef u64			__be64;
>  
> -#ifdef __LITTLE_ENDIAN__
> -#define cpu_to_be16(x) swab16(x)
> -#define be16_to_cpu(x) swab16(x)
> -#define cpu_to_be32(x) swab32(x)
> -#define be32_to_cpu(x) swab32(x)
> -#define cpu_to_be64(x) swab64(x)
> -#define be64_to_cpu(x) swab64(x)
> -#else
> -#define cpu_to_be16(x) (x)
> -#define be16_to_cpu(x) (x)
> -#define cpu_to_be32(x) (x)
> -#define be32_to_cpu(x) (x)
> -#define cpu_to_be64(x) (x)
> -#define be64_to_cpu(x) (x)
> -#endif
> -
>  #define PROM_ERROR (-1u)
>  
>  #endif /* _PPC_BOOT_OF_H_ */
> diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
> index e060676..5100dd7 100644
> --- a/arch/powerpc/boot/ops.h
> +++ b/arch/powerpc/boot/ops.h
> @@ -95,6 +95,7 @@ void *simple_alloc_init(char *base, unsigned long heap_size,
>  extern void flush_cache(void *, unsigned long);
>  int dt_xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
>  int dt_xlate_addr(void *node, u32 *buf, int buflen, unsigned long *xlated_addr);
> +int dt_read_addr(void *node, const char *prop, unsigned long *out);
>  int dt_is_compatible(void *node, const char *compat);
>  void dt_get_reg_format(void *node, u32 *naddr, u32 *nsize);
>  int dt_get_virtual_reg(void *node, void **addr, int nres);
> diff --git a/arch/powerpc/boot/swab.h b/arch/powerpc/boot/swab.h
> index 11d2069..82db2c1 100644
> --- a/arch/powerpc/boot/swab.h
> +++ b/arch/powerpc/boot/swab.h
> @@ -27,4 +27,21 @@ static inline u64 swab64(u64 x)
>  		(u64)((x & (u64)0x00ff000000000000ULL) >> 40) |
>  		(u64)((x & (u64)0xff00000000000000ULL) >> 56);
>  }
> +
> +#ifdef __LITTLE_ENDIAN__
> +#define cpu_to_be16(x) swab16(x)
> +#define be16_to_cpu(x) swab16(x)
> +#define cpu_to_be32(x) swab32(x)
> +#define be32_to_cpu(x) swab32(x)
> +#define cpu_to_be64(x) swab64(x)
> +#define be64_to_cpu(x) swab64(x)
> +#else
> +#define cpu_to_be16(x) (x)
> +#define be16_to_cpu(x) (x)
> +#define cpu_to_be32(x) (x)
> +#define be32_to_cpu(x) (x)
> +#define cpu_to_be64(x) (x)
> +#define be64_to_cpu(x) (x)
> +#endif
> +
>  #endif /* _PPC_BOOT_SWAB_H_ */
> 

-- 
Alexey

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Segher Boessenkool]

On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> When booting under OF the zImage expects the initrd address and size to be
> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> currently doesn't do this so the zImage is not aware of the initrd
> location.  This can result in initrd corruption either though the zImage
> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> initrd when relocating itself.
> 
> QEMU does put the linux,initrd-start and linux,initrd-end properties into
> the devicetree to vmlinux to find the initrd. We can work around the SLOF
> bug by also looking those properties in the zImage.

This is not a bug.  What boot protocol requires passing the initrd start
and size in GPR3, GPR4?

The CHRP binding (what SLOF implements) requires passing two zeroes here.
And ePAPR requires passing the address of a device tree and a zero, plus
something in GPR6 to allow distinguishing what it does.

As Alexey says, initramfs works just fine, so please use that?  initrd was
deprecated when this code was written already.


Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Segher Boessenkool]

On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> When booting under OF the zImage expects the initrd address and size to be
> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> currently doesn't do this so the zImage is not aware of the initrd
> location.  This can result in initrd corruption either though the zImage
> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> initrd when relocating itself.
> 
> QEMU does put the linux,initrd-start and linux,initrd-end properties into
> the devicetree to vmlinux to find the initrd. We can work around the SLOF
> bug by also looking those properties in the zImage.

This is not a bug.  What boot protocol requires passing the initrd start
and size in GPR3, GPR4?

The CHRP binding (what SLOF implements) requires passing two zeroes here.
And ePAPR requires passing the address of a device tree and a zero, plus
something in GPR6 to allow distinguishing what it does.

As Alexey says, initramfs works just fine, so please use that?  initrd was
deprecated when this code was written already.


Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Oliver O'Halloran]

On Wed, Oct 23, 2019 at 10:21 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> > When booting under OF the zImage expects the initrd address and size to be
> > passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> > currently doesn't do this so the zImage is not aware of the initrd
> > location.  This can result in initrd corruption either though the zImage
> > extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> > initrd when relocating itself.
> >
> > QEMU does put the linux,initrd-start and linux,initrd-end properties into
> > the devicetree to vmlinux to find the initrd. We can work around the SLOF
> > bug by also looking those properties in the zImage.
>
> This is not a bug.  What boot protocol requires passing the initrd start
> and size in GPR3, GPR4?
>
> The CHRP binding (what SLOF implements) requires passing two zeroes here.
> And ePAPR requires passing the address of a device tree and a zero, plus
> something in GPR6 to allow distinguishing what it does.

This is what is assumed by the zImage.pseries. I have no idea where
that assumption comes from,A B

> As Alexey says, initramfs works just fine, so please use that?  initrd was
> deprecated when this code was written already.

That's not what Alexey said and the distinction between an initrd and
an initramfs is completely arbitrary.

>
>
> Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Oliver O'Halloran]

On Wed, Oct 23, 2019 at 10:21 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> > When booting under OF the zImage expects the initrd address and size to be
> > passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> > currently doesn't do this so the zImage is not aware of the initrd
> > location.  This can result in initrd corruption either though the zImage
> > extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> > initrd when relocating itself.
> >
> > QEMU does put the linux,initrd-start and linux,initrd-end properties into
> > the devicetree to vmlinux to find the initrd. We can work around the SLOF
> > bug by also looking those properties in the zImage.
>
> This is not a bug.  What boot protocol requires passing the initrd start
> and size in GPR3, GPR4?
>
> The CHRP binding (what SLOF implements) requires passing two zeroes here.
> And ePAPR requires passing the address of a device tree and a zero, plus
> something in GPR6 to allow distinguishing what it does.

This is what is assumed by the zImage.pseries. I have no idea where
that assumption comes from,A B

> As Alexey says, initramfs works just fine, so please use that?  initrd was
> deprecated when this code was written already.

That's not what Alexey said and the distinction between an initrd and
an initramfs is completely arbitrary.

>
>
> Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Alexey Kardashevskiy]

On 23/10/2019 22:21, Segher Boessenkool wrote:
> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
>> When booting under OF the zImage expects the initrd address and size to be
>> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
>> currently doesn't do this so the zImage is not aware of the initrd
>> location.  This can result in initrd corruption either though the zImage
>> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
>> initrd when relocating itself.
>>
>> QEMU does put the linux,initrd-start and linux,initrd-end properties into
>> the devicetree to vmlinux to find the initrd. We can work around the SLOF
>> bug by also looking those properties in the zImage.
> 
> This is not a bug.  What boot protocol requires passing the initrd start
> and size in GPR3, GPR4?

So far I was unable to identify it...

> The CHRP binding (what SLOF implements) requires passing two zeroes here.
> And ePAPR requires passing the address of a device tree and a zero, plus
> something in GPR6 to allow distinguishing what it does.
> 
> As Alexey says, initramfs works just fine, so please use that?  initrd was
> deprecated when this code was written already.

I did not say about anything working fine :)

In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
also breaks "vmlinux".


diff --git a/slof/fs/client.fs b/slof/fs/client.fs
index 8a7f6ac4326d..138177e4c2a3 100644
--- a/slof/fs/client.fs
+++ b/slof/fs/client.fs
@@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
   >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
   \ Initialise client-stack-pointer
   cistack ciregs >r1 !
+
+  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
+  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
+  2dup - dup IF
+    ciregs >r4 !
+    ciregs >r3 !
+    drop
+  ELSE
+    3drop
+  THEN
+


-- 
Alexey

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Alexey Kardashevskiy]

On 24/10/2019 12:31, Alexey Kardashevskiy wrote:
> 
> 
> On 23/10/2019 22:21, Segher Boessenkool wrote:
>> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
>>> When booting under OF the zImage expects the initrd address and size to be
>>> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
>>> currently doesn't do this so the zImage is not aware of the initrd
>>> location.  This can result in initrd corruption either though the zImage
>>> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
>>> initrd when relocating itself.
>>>
>>> QEMU does put the linux,initrd-start and linux,initrd-end properties into
>>> the devicetree to vmlinux to find the initrd. We can work around the SLOF
>>> bug by also looking those properties in the zImage.
>>
>> This is not a bug.  What boot protocol requires passing the initrd start
>> and size in GPR3, GPR4?
> 
> So far I was unable to identify it...
> 
>> The CHRP binding (what SLOF implements) requires passing two zeroes here.
>> And ePAPR requires passing the address of a device tree and a zero, plus
>> something in GPR6 to allow distinguishing what it does.
>>
>> As Alexey says, initramfs works just fine, so please use that?  initrd was
>> deprecated when this code was written already.
> 
> I did not say about anything working fine :)
> 
> In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
> linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
> fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
> also breaks "vmlinux".


ah no, that works for vmlinux as well. Hm.


> 
> 
> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
> index 8a7f6ac4326d..138177e4c2a3 100644
> --- a/slof/fs/client.fs
> +++ b/slof/fs/client.fs
> @@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
>    >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
>    \ Initialise client-stack-pointer
>    cistack ciregs >r1 !
> +
> +  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  2dup - dup IF
> +    ciregs >r4 !
> +    ciregs >r3 !
> +    drop
> +  ELSE
> +    3drop
> +  THEN
> +
> 
> 

-- 
Alexey

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Segher Boessenkool]

On Thu, Oct 24, 2019 at 12:31:24PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 23/10/2019 22:21, Segher Boessenkool wrote:
> > On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> >> When booting under OF the zImage expects the initrd address and size to be
> >> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> >> currently doesn't do this so the zImage is not aware of the initrd
> >> location.  This can result in initrd corruption either though the zImage
> >> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> >> initrd when relocating itself.
> >>
> >> QEMU does put the linux,initrd-start and linux,initrd-end properties into
> >> the devicetree to vmlinux to find the initrd. We can work around the SLOF
> >> bug by also looking those properties in the zImage.
> > 
> > This is not a bug.  What boot protocol requires passing the initrd start
> > and size in GPR3, GPR4?
> 
> So far I was unable to identify it...

Maybe this comes from yaboot?
https://git.ozlabs.org/?p=yaboot.git;a=blob;f=second/yaboot.c;h=9b66ab44e1be0ee82b88e386a5d0358428766e73;hb=HEAD#l1186

> > The CHRP binding (what SLOF implements) requires passing two zeroes here.
> > And ePAPR requires passing the address of a device tree and a zero, plus
> > something in GPR6 to allow distinguishing what it does.
> > 
> > As Alexey says, initramfs works just fine, so please use that?  initrd was
> > deprecated when this code was written already.
> 
> I did not say about anything working fine :)

Yeah, I read that from your words, wrong it seems.  Sorry.  I often used
INITRAMFS_SOURCE for kernels for use with SLOF, it's just so convenient.

> In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
> linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
> fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
> also breaks "vmlinux".
> 
> 
> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
> index 8a7f6ac4326d..138177e4c2a3 100644
> --- a/slof/fs/client.fs
> +++ b/slof/fs/client.fs
> @@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
>    >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
>    \ Initialise client-stack-pointer
>    cistack ciregs >r1 !
> +
> +  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  2dup - dup IF
> +    ciregs >r4 !
> +    ciregs >r3 !
> +    drop
> +  ELSE
> +    3drop
> +  THEN

Something like that should work fine.  Do it in go-32 and go-64 though?
Or is that the wrong spot?


Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Segher Boessenkool]

On Thu, Oct 24, 2019 at 12:31:24PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 23/10/2019 22:21, Segher Boessenkool wrote:
> > On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
> >> When booting under OF the zImage expects the initrd address and size to be
> >> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
> >> currently doesn't do this so the zImage is not aware of the initrd
> >> location.  This can result in initrd corruption either though the zImage
> >> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
> >> initrd when relocating itself.
> >>
> >> QEMU does put the linux,initrd-start and linux,initrd-end properties into
> >> the devicetree to vmlinux to find the initrd. We can work around the SLOF
> >> bug by also looking those properties in the zImage.
> > 
> > This is not a bug.  What boot protocol requires passing the initrd start
> > and size in GPR3, GPR4?
> 
> So far I was unable to identify it...

Maybe this comes from yaboot?
https://git.ozlabs.org/?p=yaboot.git;a=blob;f=second/yaboot.c;h=9b66ab44e1be0ee82b88e386a5d0358428766e73;hb=HEAD#l1186

> > The CHRP binding (what SLOF implements) requires passing two zeroes here.
> > And ePAPR requires passing the address of a device tree and a zero, plus
> > something in GPR6 to allow distinguishing what it does.
> > 
> > As Alexey says, initramfs works just fine, so please use that?  initrd was
> > deprecated when this code was written already.
> 
> I did not say about anything working fine :)

Yeah, I read that from your words, wrong it seems.  Sorry.  I often used
INITRAMFS_SOURCE for kernels for use with SLOF, it's just so convenient.

> In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
> linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
> fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
> also breaks "vmlinux".
> 
> 
> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
> index 8a7f6ac4326d..138177e4c2a3 100644
> --- a/slof/fs/client.fs
> +++ b/slof/fs/client.fs
> @@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
>    >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
>    \ Initialise client-stack-pointer
>    cistack ciregs >r1 !
> +
> +  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
> +  2dup - dup IF
> +    ciregs >r4 !
> +    ciregs >r3 !
> +    drop
> +  ELSE
> +    3drop
> +  THEN

Something like that should work fine.  Do it in go-32 and go-64 though?
Or is that the wrong spot?


Segher

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Alexey Kardashevskiy]

On 25/10/2019 04:45, Segher Boessenkool wrote:
> On Thu, Oct 24, 2019 at 12:31:24PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 23/10/2019 22:21, Segher Boessenkool wrote:
>>> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
>>>> When booting under OF the zImage expects the initrd address and size to be
>>>> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
>>>> currently doesn't do this so the zImage is not aware of the initrd
>>>> location.  This can result in initrd corruption either though the zImage
>>>> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
>>>> initrd when relocating itself.
>>>>
>>>> QEMU does put the linux,initrd-start and linux,initrd-end properties into
>>>> the devicetree to vmlinux to find the initrd. We can work around the SLOF
>>>> bug by also looking those properties in the zImage.
>>>
>>> This is not a bug.  What boot protocol requires passing the initrd start
>>> and size in GPR3, GPR4?
>>
>> So far I was unable to identify it...
> 
> Maybe this comes from yaboot?
> https://git.ozlabs.org/?p=yaboot.git;a=blob;f=second/yaboot.c;h=9b66ab44e1be0ee82b88e386a5d0358428766e73;hb=HEAD#l1186

I asked around, a "common practice" was the response :) It's been like this for ages and it did not come from any OF/PPC
binding. It was also noted that we do not use zImage right - the whole idea was that it is a single binary blob with
vmlinux _and_ initramdisk to point OF at as at the time it could only deal with single blobs. So having separate zImage
and initrd is out of zImage design scope (some disagreed here).


>>> The CHRP binding (what SLOF implements) requires passing two zeroes here.
>>> And ePAPR requires passing the address of a device tree and a zero, plus
>>> something in GPR6 to allow distinguishing what it does.
>>>
>>> As Alexey says, initramfs works just fine, so please use that?  initrd was
>>> deprecated when this code was written already.
>>
>> I did not say about anything working fine :)
> 
> Yeah, I read that from your words, wrong it seems.  Sorry.  I often used
> INITRAMFS_SOURCE for kernels for use with SLOF, it's just so convenient.
> 
>> In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
>> linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
>> fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
>> also breaks "vmlinux".
>>
>>
>> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
>> index 8a7f6ac4326d..138177e4c2a3 100644
>> --- a/slof/fs/client.fs
>> +++ b/slof/fs/client.fs
>> @@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
>>    >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
>>    \ Initialise client-stack-pointer
>>    cistack ciregs >r1 !
>> +
>> +  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
>> +  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
>> +  2dup - dup IF
>> +    ciregs >r4 !
>> +    ciregs >r3 !
>> +    drop
>> +  ELSE
>> +    3drop
>> +  THEN
> 
> Something like that should work fine.  Do it in go-32 and go-64 though?
> Or is that the wrong spot?


Nah, I was trying a different initramdisk which complained about my test kernel being too old, after fixing that, it
works. I'll post a patch. Thanks,



-- 
Alexey

Re: [PATCH] powerpc/boot: Fix the initrd being overwritten under qemu

[Alexey Kardashevskiy]

On 25/10/2019 04:45, Segher Boessenkool wrote:
> On Thu, Oct 24, 2019 at 12:31:24PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> On 23/10/2019 22:21, Segher Boessenkool wrote:
>>> On Wed, Oct 23, 2019 at 12:36:35PM +1100, Oliver O'Halloran wrote:
>>>> When booting under OF the zImage expects the initrd address and size to be
>>>> passed to it using registers r3 and r4. SLOF (guest firmware used by QEMU)
>>>> currently doesn't do this so the zImage is not aware of the initrd
>>>> location.  This can result in initrd corruption either though the zImage
>>>> extracting the vmlinux over the initrd, or by the vmlinux overwriting the
>>>> initrd when relocating itself.
>>>>
>>>> QEMU does put the linux,initrd-start and linux,initrd-end properties into
>>>> the devicetree to vmlinux to find the initrd. We can work around the SLOF
>>>> bug by also looking those properties in the zImage.
>>>
>>> This is not a bug.  What boot protocol requires passing the initrd start
>>> and size in GPR3, GPR4?
>>
>> So far I was unable to identify it...
> 
> Maybe this comes from yaboot?
> https://git.ozlabs.org/?p=yaboot.git;a=blob;f=second/yaboot.c;h=9b66ab44e1be0ee82b88e386a5d0358428766e73;hb=HEAD#l1186

I asked around, a "common practice" was the response :) It's been like this for ages and it did not come from any OF/PPC
binding. It was also noted that we do not use zImage right - the whole idea was that it is a single binary blob with
vmlinux _and_ initramdisk to point OF at as at the time it could only deal with single blobs. So having separate zImage
and initrd is out of zImage design scope (some disagreed here).


>>> The CHRP binding (what SLOF implements) requires passing two zeroes here.
>>> And ePAPR requires passing the address of a device tree and a zero, plus
>>> something in GPR6 to allow distinguishing what it does.
>>>
>>> As Alexey says, initramfs works just fine, so please use that?  initrd was
>>> deprecated when this code was written already.
>>
>> I did not say about anything working fine :)
> 
> Yeah, I read that from your words, wrong it seems.  Sorry.  I often used
> INITRAMFS_SOURCE for kernels for use with SLOF, it's just so convenient.
> 
>> In my case I was using a new QEMU which does full FDT on client-arch-support and that thing would put the original
>> linux,initrd-start/end to the FDT even though the initrd was unpacked and the properties were changes in SLOF. With that
>> fixed, this is an alternative fix for SLOF but I am not pushing it out as I have no idea about the bindings and this
>> also breaks "vmlinux".
>>
>>
>> diff --git a/slof/fs/client.fs b/slof/fs/client.fs
>> index 8a7f6ac4326d..138177e4c2a3 100644
>> --- a/slof/fs/client.fs
>> +++ b/slof/fs/client.fs
>> @@ -45,6 +45,17 @@ VARIABLE  client-callback \ Address of client's callback function
>>    >r  ciregs >r7 !  ciregs >r6 !  client-entry-point @ ciregs >r5 !
>>    \ Initialise client-stack-pointer
>>    cistack ciregs >r1 !
>> +
>> +  s" linux,initrd-end" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
>> +  s" linux,initrd-start" get-chosen IF decode-int -rot 2drop ELSE 0 THEN
>> +  2dup - dup IF
>> +    ciregs >r4 !
>> +    ciregs >r3 !
>> +    drop
>> +  ELSE
>> +    3drop
>> +  THEN
> 
> Something like that should work fine.  Do it in go-32 and go-64 though?
> Or is that the wrong spot?


Nah, I was trying a different initramdisk which complained about my test kernel being too old, after fixing that, it
works. I'll post a patch. Thanks,



-- 
Alexey