[PATCH qemu] spapr: Add /choses to FDT only at reset time to preserve kernel and initramdisk

[PATCH qemu] spapr: Add /choses to FDT only at reset time to preserve kernel and initramdisk

[Alexey Kardashevskiy]

Since "spapr: Render full FDT on ibm,client-architecture-support" we build
the entire flatten device tree (FDT) twice - at the reset time and
when "ibm,client-architecture-support" (CAS) is called. The full FDT from
CAS is then applied on top of the SLOF internal device tree.

This is mostly ok, however there is a case when the QEMU is started with
-initrd and for some reason the guest decided to move/unpack the init RAM
disk image - the guest correctly notifies SLOF about the change but
at CAS it is overridden with the QEMU initial location addresses and
the guest may fail to boot if the original initrd memory was changed.

This fixes the problem by only adding the /chosen node at the reset time
to prevent the original QEMU's linux,initrd-start/linux,initrd-end to
override the updated addresses.

This only treats /chosen differently as we know there is a special case
already and it is unlikely anything else will need to change /chosen at CAS
we are better off not touching /chosen after we handed it over to SLOF.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/ppc/spapr.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d4c07a9b1bab..0580789a1509 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -925,7 +925,7 @@ static bool spapr_hotplugged_dev_before_cas(void)
     return false;
 }
 
-static void *spapr_build_fdt(SpaprMachineState *spapr);
+static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
 
 int spapr_h_cas_compose_response(SpaprMachineState *spapr,
                                  target_ulong addr, target_ulong size,
@@ -947,7 +947,7 @@ int spapr_h_cas_compose_response(SpaprMachineState *spapr,
 
     size -= sizeof(hdr);
 
-    fdt = spapr_build_fdt(spapr);
+    fdt = spapr_build_fdt(spapr, false);
     _FDT((fdt_pack(fdt)));
 
     if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
@@ -1205,7 +1205,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
     }
 }
 
-static void *spapr_build_fdt(SpaprMachineState *spapr)
+static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
 {
     MachineState *machine = MACHINE(spapr);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -1305,7 +1305,9 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
     spapr_dt_rtas(spapr, fdt);
 
     /* /chosen */
-    spapr_dt_chosen(spapr, fdt);
+    if (reset) {
+        spapr_dt_chosen(spapr, fdt);
+    }
 
     /* /hypervisor */
     if (kvm_enabled()) {
@@ -1313,11 +1315,14 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
     }
 
     /* Build memory reserve map */
-    if (spapr->kernel_size) {
-        _FDT((fdt_add_mem_rsv(fdt, KERNEL_LOAD_ADDR, spapr->kernel_size)));
-    }
-    if (spapr->initrd_size) {
-        _FDT((fdt_add_mem_rsv(fdt, spapr->initrd_base, spapr->initrd_size)));
+    if (reset) {
+        if (spapr->kernel_size) {
+            _FDT((fdt_add_mem_rsv(fdt, KERNEL_LOAD_ADDR, spapr->kernel_size)));
+        }
+        if (spapr->initrd_size) {
+            _FDT((fdt_add_mem_rsv(fdt, spapr->initrd_base,
+                                  spapr->initrd_size)));
+        }
     }
 
     /* ibm,client-architecture-support updates */
@@ -1726,7 +1731,7 @@ static void spapr_machine_reset(MachineState *machine)
      */
     fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
 
-    fdt = spapr_build_fdt(spapr);
+    fdt = spapr_build_fdt(spapr, true);
 
     rc = fdt_pack(fdt);
 
-- 
2.17.1

Re: [PATCH qemu] spapr: Add /choses to FDT only at reset time to preserve kernel and initramdisk

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191024041308.5673-1-aik@ozlabs.ru/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    iotest-qcow2: 268
Failures: 192
Failed 1 of 109 iotests
make: *** [check-tests/check-block.sh] Error 1
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=5d5462280d174ce88f8f69048bbd8b1f', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-5s094kqm/src/docker-src.2019-10-24-16.56.10.18617:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=5d5462280d174ce88f8f69048bbd8b1f
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-5s094kqm/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m14.511s
user    0m8.352s


The full log is available at
http://patchew.org/logs/20191024041308.5673-1-aik@ozlabs.ru/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH qemu] spapr: Add /choses to FDT only at reset time to preserve kernel and initramdisk

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4175 bytes --]

On Thu, Oct 24, 2019 at 03:13:08PM +1100, Alexey Kardashevskiy wrote:
65;5603;1c> Since "spapr: Render full FDT on ibm,client-architecture-support" we build
> the entire flatten device tree (FDT) twice - at the reset time and
> when "ibm,client-architecture-support" (CAS) is called. The full FDT from
> CAS is then applied on top of the SLOF internal device tree.
> 
> This is mostly ok, however there is a case when the QEMU is started with
> -initrd and for some reason the guest decided to move/unpack the init RAM
> disk image - the guest correctly notifies SLOF about the change but
> at CAS it is overridden with the QEMU initial location addresses and
> the guest may fail to boot if the original initrd memory was changed.
> 
> This fixes the problem by only adding the /chosen node at the reset time
> to prevent the original QEMU's linux,initrd-start/linux,initrd-end to
> override the updated addresses.
> 
> This only treats /chosen differently as we know there is a special case
> already and it is unlikely anything else will need to change /chosen at CAS
> we are better off not touching /chosen after we handed it over to SLOF.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Applied to ppc-for-4.2 (with the typo in the subject line corrected
/choses -> /chosen).

> ---
>  hw/ppc/spapr.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d4c07a9b1bab..0580789a1509 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -925,7 +925,7 @@ static bool spapr_hotplugged_dev_before_cas(void)
>      return false;
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr);
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset);
>  
>  int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>                                   target_ulong addr, target_ulong size,
> @@ -947,7 +947,7 @@ int spapr_h_cas_compose_response(SpaprMachineState *spapr,
>  
>      size -= sizeof(hdr);
>  
> -    fdt = spapr_build_fdt(spapr);
> +    fdt = spapr_build_fdt(spapr, false);
>      _FDT((fdt_pack(fdt)));
>  
>      if (fdt_totalsize(fdt) + sizeof(hdr) > size) {
> @@ -1205,7 +1205,7 @@ static void spapr_dt_hypervisor(SpaprMachineState *spapr, void *fdt)
>      }
>  }
>  
> -static void *spapr_build_fdt(SpaprMachineState *spapr)
> +static void *spapr_build_fdt(SpaprMachineState *spapr, bool reset)
>  {
>      MachineState *machine = MACHINE(spapr);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -1305,7 +1305,9 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>      spapr_dt_rtas(spapr, fdt);
>  
>      /* /chosen */
> -    spapr_dt_chosen(spapr, fdt);
> +    if (reset) {
> +        spapr_dt_chosen(spapr, fdt);
> +    }
>  
>      /* /hypervisor */
>      if (kvm_enabled()) {
> @@ -1313,11 +1315,14 @@ static void *spapr_build_fdt(SpaprMachineState *spapr)
>      }
>  
>      /* Build memory reserve map */
> -    if (spapr->kernel_size) {
> -        _FDT((fdt_add_mem_rsv(fdt, KERNEL_LOAD_ADDR, spapr->kernel_size)));
> -    }
> -    if (spapr->initrd_size) {
> -        _FDT((fdt_add_mem_rsv(fdt, spapr->initrd_base, spapr->initrd_size)));
> +    if (reset) {
> +        if (spapr->kernel_size) {
> +            _FDT((fdt_add_mem_rsv(fdt, KERNEL_LOAD_ADDR, spapr->kernel_size)));
> +        }
> +        if (spapr->initrd_size) {
> +            _FDT((fdt_add_mem_rsv(fdt, spapr->initrd_base,
> +                                  spapr->initrd_size)));
> +        }
>      }
>  
>      /* ibm,client-architecture-support updates */
> @@ -1726,7 +1731,7 @@ static void spapr_machine_reset(MachineState *machine)
>       */
>      fdt_addr = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FDT_MAX_SIZE;
>  
> -    fdt = spapr_build_fdt(spapr);
> +    fdt = spapr_build_fdt(spapr, true);
>  
>      rc = fdt_pack(fdt);
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]