Handful of bug fixes to sneak in before rc3. thanks -- PMM The following changes since commit c985266ea5b50e46e07b3568c1346e10064205c9: Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20190726' into staging (2019-07-26 13:52:06 +0100) are available in the Git repository at: https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20190726 for you to fetch changes up to 67505c114e6acc26f3a1a2b74833c61b6a34ff95: hw/arm/boot: Further improve initrd positioning code (2019-07-26 16:17:56 +0100) ---------------------------------------------------------------- target-arm queue: * Fix broken migration on pl330 device * Fix broken migration on stellaris-input device * Add type checks to vmstate varry macros to avoid this class of bugs * hw/arm/boot: Fix some remaining cases where we would put the initrd on top of the kernel image ---------------------------------------------------------------- Damien Hedde (1): pl330: fix vmstate description Peter Maydell (4): stellaris_input: Fix vmstate description of buttons field vmstate.h: Type check VMSTATE_STRUCT_VARRAY macros hw/arm/boot: Rename elf_{low, high}_addr to image_{low, high}_addr hw/arm/boot: Further improve initrd positioning code include/migration/vmstate.h | 30 ++++++++++++++++++++++++------ hw/arm/boot.c | 37 +++++++++++++++++++++++++++---------- hw/dma/pl330.c | 17 +++++++++-------- hw/input/stellaris_input.c | 10 ++++++---- 4 files changed, 66 insertions(+), 28 deletions(-) From: Damien Hedde <damien.hedde@greensocs.com> Fix the pl330 main and queue vmstate description. There were missing POINTER flags causing crashes during incoming migration because: + PL330State chan field is a pointer to an array + PL330Queue queue field is a pointer to an array Also bump corresponding vmsd version numbers. Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Message-id: 20190724143553.21557-1-damien.hedde@greensocs.com Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/dma/pl330.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/hw/dma/pl330.c b/hw/dma/pl330.c index XXXXXXX..XXXXXXX 100644 --- a/hw/dma/pl330.c +++ b/hw/dma/pl330.c @@ -XXX,XX +XXX,XX @@ typedef struct PL330Queue { static const VMStateDescription vmstate_pl330_queue = { .name = "pl330_queue", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { - VMSTATE_STRUCT_VARRAY_UINT32(queue, PL330Queue, queue_size, 1, - vmstate_pl330_queue_entry, PL330QueueEntry), + VMSTATE_STRUCT_VARRAY_POINTER_UINT32(queue, PL330Queue, queue_size, + vmstate_pl330_queue_entry, + PL330QueueEntry), VMSTATE_END_OF_LIST() } }; @@ -XXX,XX +XXX,XX @@ struct PL330State { static const VMStateDescription vmstate_pl330 = { .name = "pl330", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, PL330Chan), - VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0, - vmstate_pl330_chan, PL330Chan), + VMSTATE_STRUCT_VARRAY_POINTER_UINT32(chan, PL330State, num_chnls, + vmstate_pl330_chan, PL330Chan), VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls), VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls), VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo), -- 2.20.1 gamepad_state::buttons is a pointer to an array of structs, not an array of structs, so should be declared in the vmstate with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we corrupt memory on incoming migration. We bump the vmstate version field as the easiest way to deal with the migration break, since migration wouldn't have worked reliably before anyway. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> Message-id: 20190725163710.11703-2-peter.maydell@linaro.org --- hw/input/stellaris_input.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c index XXXXXXX..XXXXXXX 100644 --- a/hw/input/stellaris_input.c +++ b/hw/input/stellaris_input.c @@ -XXX,XX +XXX,XX @@ static const VMStateDescription vmstate_stellaris_button = { static const VMStateDescription vmstate_stellaris_gamepad = { .name = "stellaris_gamepad", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_INT32(extension, gamepad_state), - VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0, - vmstate_stellaris_button, gamepad_button), + VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state, + num_buttons, + vmstate_stellaris_button, + gamepad_button), VMSTATE_END_OF_LIST() } }; -- 2.20.1 The VMSTATE_STRUCT_VARRAY_UINT32 macro is intended to handle migrating a field which is an array of structs, but where instead of migrating the entire array we only migrate a variable number of elements of it. The VMSTATE_STRUCT_VARRAY_POINTER_UINT32 macro is intended to handle migrating a field which is of pointer type, and points to a dynamically allocated array of structs of variable size. We weren't actually checking that the field passed to VMSTATE_STRUCT_VARRAY_UINT32 really is an array, with the result that accidentally using it where the _POINTER_ macro was intended would compile but silently corrupt memory on migration. Add type-checking that enforces that the field passed in is really of the right array type. This applies to all the VMSTATE macros which use flags including VMS_VARRAY_* but not VMS_POINTER. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> Tested-by: Damien Hedde <damien.hedde@greensocs.com> Message-id: 20190725163710.11703-3-peter.maydell@linaro.org --- include/migration/vmstate.h | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index XXXXXXX..XXXXXXX 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_bitmap; extern const VMStateInfo vmstate_info_qtailq; #define type_check_2darray(t1,t2,n,m) ((t1(*)[n][m])0 - (t2*)0) +/* + * Check that type t2 is an array of type t1 of size n, + * e.g. if t1 is 'foo' and n is 32 then t2 must be 'foo[32]' + */ #define type_check_array(t1,t2,n) ((t1(*)[n])0 - (t2*)0) #define type_check_pointer(t1,t2) ((t1**)0 - (t2*)0) +/* + * type of element 0 of the specified (array) field of the type. + * Note that if the field is a pointer then this will return the + * pointed-to type rather than complaining. + */ +#define typeof_elt_of_field(type, field) typeof(((type *)0)->field[0]) +/* Check that field f in struct type t2 is an array of t1, of any size */ +#define type_check_varray(t1, t2, f) \ + (type_check(t1, typeof_elt_of_field(t2, f)) \ + + QEMU_BUILD_BUG_ON_ZERO(!QEMU_IS_ARRAY(((t2 *)0)->f))) #define vmstate_offset_value(_state, _field, _type) \ (offsetof(_state, _field) + \ @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; vmstate_offset_array(_state, _field, uint8_t, \ sizeof(typeof_field(_state, _field))) +#define vmstate_offset_varray(_state, _field, _type) \ + (offsetof(_state, _field) + \ + type_check_varray(_type, _state, _field)) + /* In the macros below, if there is a _version, that means the macro's * field will be processed only if the version being received is >= * the _version specified. In general, if you add a new field, you @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .info = &(_info), \ .size = sizeof(_type), \ .flags = VMS_VARRAY_UINT32|VMS_MULTIPLY_ELEMENTS, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } #define VMSTATE_ARRAY_TEST(_field, _state, _num, _test, _info, _type) {\ @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .info = &(_info), \ .size = sizeof(_type), \ .flags = VMS_VARRAY_INT32, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } #define VMSTATE_VARRAY_INT32(_field, _state, _field_num, _version, _info, _type) {\ @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .info = &(_info), \ .size = sizeof(_type), \ .flags = VMS_VARRAY_UINT16, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } #define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, _type, _struct_version) { \ @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .vmsd = &(_vmsd), \ .size = sizeof(_type), \ .flags = VMS_STRUCT|VMS_VARRAY_UINT8, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } /* a variable length array (i.e. _type *_field) but we know the @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .vmsd = &(_vmsd), \ .size = sizeof(_type), \ .flags = VMS_STRUCT|VMS_VARRAY_INT32, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } #define VMSTATE_STRUCT_VARRAY_UINT32(_field, _state, _field_num, _version, _vmsd, _type) { \ @@ -XXX,XX +XXX,XX @@ extern const VMStateInfo vmstate_info_qtailq; .vmsd = &(_vmsd), \ .size = sizeof(_type), \ .flags = VMS_STRUCT|VMS_VARRAY_UINT32, \ - .offset = offsetof(_state, _field), \ + .offset = vmstate_offset_varray(_state, _field, _type), \ } #define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, _vmsd, _type) {\ -- 2.20.1 Rename the elf_low_addr and elf_high_addr variables to image_low_addr and image_high_addr -- in the next commit we will extend them to be set for other kinds of image file and not just ELF files. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Mark Rutland <mark.rutland@arm.com> Message-id: 20190722151804.25467-2-peter.maydell@linaro.org --- hw/arm/boot.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/hw/arm/boot.c b/hw/arm/boot.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -XXX,XX +XXX,XX @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, int kernel_size; int initrd_size; int is_linux = 0; - uint64_t elf_entry, elf_low_addr, elf_high_addr; + uint64_t elf_entry; + /* Addresses of first byte used and first byte not used by the image */ + uint64_t image_low_addr, image_high_addr; int elf_machine; hwaddr entry; static const ARMInsnFixup *primary_loader; @@ -XXX,XX +XXX,XX @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, info->nb_cpus = 1; /* Assume that raw images are linux kernels, and ELF images are not. */ - kernel_size = arm_load_elf(info, &elf_entry, &elf_low_addr, - &elf_high_addr, elf_machine, as); + kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr, + &image_high_addr, elf_machine, as); if (kernel_size > 0 && have_dtb(info)) { /* * If there is still some room left at the base of RAM, try and put * the DTB there like we do for images loaded with -bios or -pflash. */ - if (elf_low_addr > info->loader_start - || elf_high_addr < info->loader_start) { + if (image_low_addr > info->loader_start + || image_high_addr < info->loader_start) { /* - * Set elf_low_addr as address limit for arm_load_dtb if it may be + * Set image_low_addr as address limit for arm_load_dtb if it may be * pointing into RAM, otherwise pass '0' (no limit) */ - if (elf_low_addr < info->loader_start) { - elf_low_addr = 0; + if (image_low_addr < info->loader_start) { + image_low_addr = 0; } info->dtb_start = info->loader_start; - info->dtb_limit = elf_low_addr; + info->dtb_limit = image_low_addr; } } entry = elf_entry; -- 2.20.1 In commit e6b2b20d9735d4ef we made the boot loader code try to avoid putting the initrd on top of the kernel. However the expression used to calculate the start of the initrd: info->initrd_start = info->loader_start + MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); incorrectly uses 'kernel_size' as the offset within RAM of the highest address to avoid. This is incorrect because the kernel doesn't start at address 0, but slightly higher than that. This means that we can still incorrectly end up overlaying the initrd on the kernel in some cases, for example: * The kernel's image_size is 0x0a7a8000 * The kernel was loaded at 0x40080000 * The end of the kernel is 0x4A828000 * The DTB was loaded at 0x4a800000 To get this right we need to track the actual highest address used by the kernel and use that rather than kernel_size. We already set image_low_addr and image_high_addr for ELF images; set them also for the various other image types we support, and then use image_high_addr as the lowest allowed address for the initrd. (We don't use image_low_addr, but we set it for consistency with the existing code path for ELF files.) Fixes: e6b2b20d9735d4ef Reported-by: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Tested-by: Mark Rutland <mark.rutland@arm.com> Message-id: 20190722151804.25467-3-peter.maydell@linaro.org --- hw/arm/boot.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/hw/arm/boot.c b/hw/arm/boot.c index XXXXXXX..XXXXXXX 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -XXX,XX +XXX,XX @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, int is_linux = 0; uint64_t elf_entry; /* Addresses of first byte used and first byte not used by the image */ - uint64_t image_low_addr, image_high_addr; + uint64_t image_low_addr = 0, image_high_addr = 0; int elf_machine; hwaddr entry; static const ARMInsnFixup *primary_loader; @@ -XXX,XX +XXX,XX @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR; kernel_size = load_uimage_as(info->kernel_filename, &entry, &loadaddr, &is_linux, NULL, NULL, as); + if (kernel_size >= 0) { + image_low_addr = loadaddr; + image_high_addr = image_low_addr + kernel_size; + } } if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) { kernel_size = load_aarch64_image(info->kernel_filename, info->loader_start, &entry, as); is_linux = 1; + if (kernel_size >= 0) { + image_low_addr = entry; + image_high_addr = image_low_addr + kernel_size; + } } else if (kernel_size < 0) { /* 32-bit ARM */ entry = info->loader_start + KERNEL_LOAD_ADDR; kernel_size = load_image_targphys_as(info->kernel_filename, entry, ram_end - KERNEL_LOAD_ADDR, as); is_linux = 1; + if (kernel_size >= 0) { + image_low_addr = entry; + image_high_addr = image_low_addr + kernel_size; + } } if (kernel_size < 0) { error_report("could not load kernel '%s'", info->kernel_filename); @@ -XXX,XX +XXX,XX @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu, * we might still make a bad choice here. */ info->initrd_start = info->loader_start + - MAX(MIN(info->ram_size / 2, 128 * 1024 * 1024), kernel_size); + MIN(info->ram_size / 2, 128 * 1024 * 1024); + if (image_high_addr) { + info->initrd_start = MAX(info->initrd_start, image_high_addr); + } info->initrd_start = TARGET_PAGE_ALIGN(info->initrd_start); if (is_linux) { -- 2.20.1 v3->v4: Windows headers define an INT type which clashed with an enum value name in arm_gicv3_its.c... The following changes since commit eae587e8e3694b1aceab23239493fb4c7e1a80f5: Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2021-09-13' into staging (2021-09-13 11:00:30 +0100) are available in the Git repository at: https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20210913-3 for you to fetch changes up to 28e987a7e7edaa3ca7feeac65edca26145df8814: hw/arm/mps2.c: Mark internal-only I2C buses as 'full' (2021-09-13 21:01:08 +0100) ---------------------------------------------------------------- target-arm queue: * mark MPS2/MPS3 board-internal i2c buses as 'full' so that command line user-created devices are not plugged into them * Take an exception if PSTATE.IL is set * Support an emulated ITS in the virt board * Add support for kudo-bmc board * Probe for KVM_CAP_ARM_VM_IPA_SIZE when creating scratch VM * cadence_uart: Fix clock handling issues that prevented u-boot from running ---------------------------------------------------------------- Bin Meng (6): hw/misc: zynq_slcr: Correctly compute output clocks in the reset exit phase hw/char: cadence_uart: Disable transmit when input clock is disabled hw/char: cadence_uart: Move clock/reset check to uart_can_receive() hw/char: cadence_uart: Convert to memop_with_attrs() ops hw/char: cadence_uart: Ignore access when unclocked or in reset for uart_{read, write}() hw/char: cadence_uart: Log a guest error when device is unclocked or in reset Chris Rauer (1): hw/arm: Add support for kudo-bmc board. Marc Zyngier (1): hw/arm/virt: KVM: Probe for KVM_CAP_ARM_VM_IPA_SIZE when creating scratch VM Peter Maydell (5): target/arm: Take an exception if PSTATE.IL is set qdev: Support marking individual buses as 'full' hw/arm/mps2-tz.c: Add extra data parameter to MakeDevFn hw/arm/mps2-tz.c: Mark internal-only I2C buses as 'full' hw/arm/mps2.c: Mark internal-only I2C buses as 'full' Richard Henderson (1): target/arm: Merge disas_a64_insn into aarch64_tr_translate_insn Shashi Mallela (9): hw/intc: GICv3 ITS initial framework hw/intc: GICv3 ITS register definitions added hw/intc: GICv3 ITS command queue framework hw/intc: GICv3 ITS Command processing hw/intc: GICv3 ITS Feature enablement hw/intc: GICv3 redistributor ITS processing tests/data/acpi/virt: Add IORT files for ITS hw/arm/virt: add ITS support in virt GIC tests/data/acpi/virt: Update IORT files for ITS docs/system/arm/nuvoton.rst | 1 + hw/intc/gicv3_internal.h | 188 ++++- include/hw/arm/virt.h | 2 + include/hw/intc/arm_gicv3_common.h | 13 + include/hw/intc/arm_gicv3_its_common.h | 32 +- include/hw/qdev-core.h | 24 + target/arm/cpu.h | 1 + target/arm/kvm_arm.h | 4 +- target/arm/syndrome.h | 5 + target/arm/translate.h | 2 + hw/arm/mps2-tz.c | 92 ++- hw/arm/mps2.c | 12 +- hw/arm/npcm7xx_boards.c | 34 + hw/arm/virt.c | 29 +- hw/char/cadence_uart.c | 61 +- hw/intc/arm_gicv3.c | 14 + hw/intc/arm_gicv3_common.c | 13 + hw/intc/arm_gicv3_cpuif.c | 7 +- hw/intc/arm_gicv3_dist.c | 5 +- hw/intc/arm_gicv3_its.c | 1322 ++++++++++++++++++++++++++++++++ hw/intc/arm_gicv3_its_common.c | 7 +- hw/intc/arm_gicv3_its_kvm.c | 2 +- hw/intc/arm_gicv3_redist.c | 153 +++- hw/misc/zynq_slcr.c | 31 +- softmmu/qdev-monitor.c | 7 +- target/arm/helper-a64.c | 1 + target/arm/helper.c | 8 + target/arm/kvm.c | 7 +- target/arm/translate-a64.c | 255 +++--- target/arm/translate.c | 21 + hw/intc/meson.build | 1 + tests/data/acpi/virt/IORT | Bin 0 -> 124 bytes tests/data/acpi/virt/IORT.memhp | Bin 0 -> 124 bytes tests/data/acpi/virt/IORT.numamem | Bin 0 -> 124 bytes tests/data/acpi/virt/IORT.pxb | Bin 0 -> 124 bytes 35 files changed, 2144 insertions(+), 210 deletions(-) create mode 100644 hw/intc/arm_gicv3_its.c create mode 100644 tests/data/acpi/virt/IORT create mode 100644 tests/data/acpi/virt/IORT.memhp create mode 100644 tests/data/acpi/virt/IORT.numamem create mode 100644 tests/data/acpi/virt/IORT.pxb