[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios optio
From: |
Auger Eric |
Subject: |
Re: [Qemu-devel] [PATCH] arm: fix qemu crash on startup with -bios option |
Date: |
Wed, 23 May 2018 14:09:20 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 |
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 <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>
Thank you for the quick fix. It fixes the reported issue.
Reviewed-by: Eric Auger <address@hidden>
Tested-by: Eric Auger <address@hidden>
Thanks
Eric
> ---
> Thanks Andrew Jones <address@hidden> 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);
>