[Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios option

[Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios option

[Igor Mammedov]

When QEMU is started with following CLI
 -machine virt,gic-version=3,accel=kvm -cpu host -bios AAVMF_CODE.fd
it crashes with abort at
 accel/kvm/kvm-all.c:2164:
 KVM_SET_DEVICE_ATTR failed: Group 6 attr 0x000000000000c665: Invalid argument

Which is caused by implicit dependecy of kvm_arm_gicv3_reset() on
arm_gicv3_icc_reset() where the later is called by CPU reset
reset callback.

However commit:
 3b77f6c arm/boot: split load_dtb() from arm_load_kernel()
broke CPU reset callback registration in case

  arm_load_kernel()
      ...
      if (!info->kernel_filename || info->firmware_loaded)

branch is taken, i.e. it's sufficient to provide a firmware
or do not provide kernel on CLI to skip cpu reset callback
registration, where before offending commit the callback
has been registered unconditionally.

Fix it by registering the callback right at the begging of
arm_load_kernel() unconditionally instead of doing it at the end.

NOTE:
 we probably should eleminate that dependency anyways as well as
 separate arch CPU reset parts from arm_load_kernel() into CPU
 itself, but that refactoring that I probably would have to do
 anyways later for CPU hotplug to work.

Reported-by: Auger Eric <eric.auger@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 Thanks Andrew Jones <drjones@redhat.com> for host with reproducer.
---
 hw/arm/boot.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 9496f33..1e48166 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -926,6 +926,15 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     static const ARMInsnFixup *primary_loader;
     AddressSpace *as = arm_boot_address_space(cpu, info);
 
+    /* CPU objects (unlike devices) are not automatically reset on system
+     * reset, so we must always register a handler to do so. If we're
+     * actually loading a kernel, the handler is also responsible for
+     * arranging that we start it correctly.
+     */
+    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
+    }
+
     /* The board code is not supposed to set secure_board_setup unless
      * running its code in secure mode is actually possible, and KVM
      * doesn't support secure.
@@ -1143,15 +1152,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         ARM_CPU(cs)->env.boot_info = info;
     }
 
-    /* CPU objects (unlike devices) are not automatically reset on system
-     * reset, so we must always register a handler to do so. If we're
-     * actually loading a kernel, the handler is also responsible for
-     * arranging that we start it correctly.
-     */
-    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
-        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
-    }
-
     if (!info->skip_dtb_autoload && have_dtb(info)) {
         if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
             exit(1);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios option

[Auger Eric]

Hi Igor,

On 05/23/2018 12:22 PM, Igor Mammedov wrote:
> When QEMU is started with following CLI
>  -machine virt,gic-version=3,accel=kvm -cpu host -bios AAVMF_CODE.fd
> it crashes with abort at
>  accel/kvm/kvm-all.c:2164:
>  KVM_SET_DEVICE_ATTR failed: Group 6 attr 0x000000000000c665: Invalid argument
> 
> Which is caused by implicit dependecy of kvm_arm_gicv3_reset() on
dependency
> arm_gicv3_icc_reset() where the later is called by CPU reset
> reset callback.
> 
> However commit:
>  3b77f6c arm/boot: split load_dtb() from arm_load_kernel()
> broke CPU reset callback registration in case
> 
>   arm_load_kernel()
>       ...
>       if (!info->kernel_filename || info->firmware_loaded)
> 
> branch is taken, i.e. it's sufficient to provide a firmware
> or do not provide kernel on CLI to skip cpu reset callback
> registration, where before offending commit the callback
> has been registered unconditionally.
> 
> Fix it by registering the callback right at the begging of
beginning
> arm_load_kernel() unconditionally instead of doing it at the end.
> 
> NOTE:
>  we probably should eleminate that dependency anyways as well as
eliminate, anyway?
>  separate arch CPU reset parts from arm_load_kernel() into CPU
>  itself, but that refactoring that I probably would have to do
>  anyways later for CPU hotplug to work.
may deserve some rewording.
> 
> Reported-by: Auger Eric <eric.auger@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Thank you for the quick fix. It fixes the reported issue.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  Thanks Andrew Jones <drjones@redhat.com> for host with reproducer.
> ---
>  hw/arm/boot.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 9496f33..1e48166 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -926,6 +926,15 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      static const ARMInsnFixup *primary_loader;
>      AddressSpace *as = arm_boot_address_space(cpu, info);
>  
> +    /* CPU objects (unlike devices) are not automatically reset on system
> +     * reset, so we must always register a handler to do so. If we're
> +     * actually loading a kernel, the handler is also responsible for
> +     * arranging that we start it correctly.
> +     */
> +    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> +        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> +    }
> +
>      /* The board code is not supposed to set secure_board_setup unless
>       * running its code in secure mode is actually possible, and KVM
>       * doesn't support secure.
> @@ -1143,15 +1152,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>          ARM_CPU(cs)->env.boot_info = info;
>      }
>  
> -    /* CPU objects (unlike devices) are not automatically reset on system
> -     * reset, so we must always register a handler to do so. If we're
> -     * actually loading a kernel, the handler is also responsible for
> -     * arranging that we start it correctly.
> -     */
> -    for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
> -        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> -    }
> -
>      if (!info->skip_dtb_autoload && have_dtb(info)) {
>          if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as) < 0) {
>              exit(1);
> 

Re: [Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios option

[Peter Maydell]

On 23 May 2018 at 13:09, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Igor,
>
> On 05/23/2018 12:22 PM, Igor Mammedov wrote:
>> When QEMU is started with following CLI
>>  -machine virt,gic-version=3,accel=kvm -cpu host -bios AAVMF_CODE.fd
>> it crashes with abort at
>>  accel/kvm/kvm-all.c:2164:
>>  KVM_SET_DEVICE_ATTR failed: Group 6 attr 0x000000000000c665: Invalid argument
>>
>> Which is caused by implicit dependecy of kvm_arm_gicv3_reset() on
> dependency
>> arm_gicv3_icc_reset() where the later is called by CPU reset
>> reset callback.
>>
>> However commit:
>>  3b77f6c arm/boot: split load_dtb() from arm_load_kernel()
>> broke CPU reset callback registration in case
>>
>>   arm_load_kernel()
>>       ...
>>       if (!info->kernel_filename || info->firmware_loaded)
>>
>> branch is taken, i.e. it's sufficient to provide a firmware
>> or do not provide kernel on CLI to skip cpu reset callback
>> registration, where before offending commit the callback
>> has been registered unconditionally.
>>
>> Fix it by registering the callback right at the begging of
> beginning
>> arm_load_kernel() unconditionally instead of doing it at the end.
>>
>> NOTE:
>>  we probably should eleminate that dependency anyways as well as
> eliminate, anyway?
>>  separate arch CPU reset parts from arm_load_kernel() into CPU
>>  itself, but that refactoring that I probably would have to do
>>  anyways later for CPU hotplug to work.
> may deserve some rewording.
>>
>> Reported-by: Auger Eric <eric.auger@redhat.com>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Thank you for the quick fix. It fixes the reported issue.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks; applied to target-arm.next with the commit message
typos fixed.

-- PMM