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

[Auger Eric <eric.auger@redhat.com>]

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);
>