From 8c06fbdf36bf4d4d486116200248730887a4d7d6 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:48 +0000 Subject: [PATCH 01/22] scripts/checkpatch.pl: Enforce multiline comment syntax We now require Linux-kernel-style multiline comments: /* * line one * line two */ Enforce this in checkpatch.pl, by backporting the relevant parts of the Linux kernel's checkpatch.pl. (The only changes needed are that Linux's checkpatch.pl WARN() function takes an extra argument that ours does not, and the kernel has a special case for networking code we don't want.)" The kernel's checkpatch does not enforce "leading /* on a line of its own, so that part is unique to QEMU's checkpatch. Sample warning output: WARNING: Block comments use a leading /* on a separate line #34: FILE: hw/intc/arm_gicv3_common.c:39: + /* Older versions of QEMU had a bug in the handling of state save/restore Signed-off-by: Peter Maydell Acked-by: Thomas Huth Reviewed-by: Markus Armbruster --- scripts/checkpatch.pl | 48 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a892a6cc7c..18e16b79df 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1569,6 +1569,54 @@ sub process { # check we are in a valid C source file if not then ignore this hunk next if ($realfile !~ /\.(h|c|cpp)$/); +# Block comment styles + + # Block comments use /* on a line of its own + if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ && #inline /*...*/ + $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank + WARN("Block comments use a leading /* on a separate line\n" . $herecurr); + } + +# Block comments use * on subsequent lines + if ($prevline =~ /$;[ \t]*$/ && #ends in comment + $prevrawline =~ /^\+.*?\/\*/ && #starting /* + $prevrawline !~ /\*\/[ \t]*$/ && #no trailing */ + $rawline =~ /^\+/ && #line is new + $rawline !~ /^\+[ \t]*\*/) { #no leading * + WARN("Block comments use * on subsequent lines\n" . $hereprev); + } + +# Block comments use */ on trailing lines + if ($rawline !~ m@^\+[ \t]*\*/[ \t]*$@ && #trailing */ + $rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ && #inline /*...*/ + $rawline !~ m@^\+.*\*{2,}/[ \t]*$@ && #trailing **/ + $rawline =~ m@^\+[ \t]*.+\*\/[ \t]*$@) { #non blank */ + WARN("Block comments use a trailing */ on a separate line\n" . $herecurr); + } + +# Block comment * alignment + if ($prevline =~ /$;[ \t]*$/ && #ends in comment + $line =~ /^\+[ \t]*$;/ && #leading comment + $rawline =~ /^\+[ \t]*\*/ && #leading * + (($prevrawline =~ /^\+.*?\/\*/ && #leading /* + $prevrawline !~ /\*\/[ \t]*$/) || #no trailing */ + $prevrawline =~ /^\+[ \t]*\*/)) { #leading * + my $oldindent; + $prevrawline =~ m@^\+([ \t]*/?)\*@; + if (defined($1)) { + $oldindent = expand_tabs($1); + } else { + $prevrawline =~ m@^\+(.*/?)\*@; + $oldindent = expand_tabs($1); + } + $rawline =~ m@^\+([ \t]*)\*@; + my $newindent = $1; + $newindent = expand_tabs($newindent); + if (length($oldindent) ne length($newindent)) { + WARN("Block comments should align the * on each line\n" . $hereprev); + } + } + # Check for potential 'bare' types my ($stat, $cond, $line_nr_next, $remain_next, $off_next, $realline_next); From 75693e14113c0d1c1ebc1e8405e00879d2a11c84 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:48 +0000 Subject: [PATCH 02/22] exec.c: Rename cpu_physical_memory_write_rom_internal() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename cpu_physical_memory_write_rom_internal() to address_space_write_rom_internal(), and make it take MemTxAttrs and return a MemTxResult. This brings its API into line with address_space_write(). This is an internal function to exec.c; fixing its API will allow us to change the global function cpu_physical_memory_write_rom(). Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Acked-by: Michael S. Tsirkin Message-id: 20181122133507.30950-2-peter.maydell@linaro.org --- exec.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/exec.c b/exec.c index bb6170dbff..92679508ba 100644 --- a/exec.c +++ b/exec.c @@ -3388,8 +3388,12 @@ enum write_rom_type { FLUSH_CACHE, }; -static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as, - hwaddr addr, const uint8_t *buf, int len, enum write_rom_type type) +static inline MemTxResult address_space_write_rom_internal(AddressSpace *as, + hwaddr addr, + MemTxAttrs attrs, + const uint8_t *buf, + int len, + enum write_rom_type type) { hwaddr l; uint8_t *ptr; @@ -3399,8 +3403,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as, rcu_read_lock(); while (len > 0) { l = len; - mr = address_space_translate(as, addr, &addr1, &l, true, - MEMTXATTRS_UNSPECIFIED); + mr = address_space_translate(as, addr, &addr1, &l, true, attrs); if (!(memory_region_is_ram(mr) || memory_region_is_romd(mr))) { @@ -3423,13 +3426,15 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as, addr += l; } rcu_read_unlock(); + return MEMTX_OK; } /* used for ROM loading : can write in RAM and ROM */ void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr, const uint8_t *buf, int len) { - cpu_physical_memory_write_rom_internal(as, addr, buf, len, WRITE_DATA); + address_space_write_rom_internal(as, addr, MEMTXATTRS_UNSPECIFIED, + buf, len, WRITE_DATA); } void cpu_flush_icache_range(hwaddr start, int len) @@ -3444,8 +3449,9 @@ void cpu_flush_icache_range(hwaddr start, int len) return; } - cpu_physical_memory_write_rom_internal(&address_space_memory, - start, NULL, len, FLUSH_CACHE); + address_space_write_rom_internal(&address_space_memory, + start, MEMTXATTRS_UNSPECIFIED, + NULL, len, FLUSH_CACHE); } typedef struct { From 3c8133f973767460f8e42c9e656f2f3ed703d00d Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:48 +0000 Subject: [PATCH 03/22] Rename cpu_physical_memory_write_rom() to address_space_write_rom() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API of cpu_physical_memory_write_rom() is odd, because it takes an AddressSpace, unlike all the other cpu_physical_memory_* access functions. Rename it to address_space_write_rom(), and bring its API into line with address_space_write(). Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Acked-by: Michael S. Tsirkin Message-id: 20181122133507.30950-3-peter.maydell@linaro.org --- docs/devel/loads-stores.rst | 35 ++++++++++++++++------------------- exec.c | 14 ++++++++------ hw/core/loader.c | 4 ++-- hw/intc/apic.c | 7 ++++--- hw/misc/tz-mpc.c | 2 +- hw/sparc/sun4m.c | 5 +++-- include/exec/cpu-common.h | 2 -- include/exec/memory.h | 26 ++++++++++++++++++++++++++ 8 files changed, 60 insertions(+), 35 deletions(-) diff --git a/docs/devel/loads-stores.rst b/docs/devel/loads-stores.rst index 57d8c524bf..c74cd090e6 100644 --- a/docs/devel/loads-stores.rst +++ b/docs/devel/loads-stores.rst @@ -253,6 +253,22 @@ Regexes for git grep - ``\`` - ``\`` +``address_space_write_rom`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +This function performs a write by physical address like +``address_space_write``, except that if the write is to a ROM then +the ROM contents will be modified, even though a write by the guest +CPU to the ROM would be ignored. This is used for non-guest writes +like writes from the gdb debug stub or initial loading of ROM contents. + +Note that portions of the write which attempt to write data to a +device will be silently ignored -- only real RAM and ROM will +be written to. + +Regexes for git grep + - ``address_space_write_rom`` + ``{ld,st}*_phys`` ~~~~~~~~~~~~~~~~~ @@ -315,25 +331,6 @@ For new code they are better avoided: Regexes for git grep - ``\`` -``cpu_physical_memory_write_rom`` -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -This function performs a write by physical address like -``address_space_write``, except that if the write is to a ROM then -the ROM contents will be modified, even though a write by the guest -CPU to the ROM would be ignored. - -Note that unlike ``cpu_physical_memory_write()`` this function takes -an AddressSpace argument, but unlike ``address_space_write()`` this -function does not take a ``MemTxAttrs`` or return a ``MemTxResult``. - -**TODO**: we should probably clean up this inconsistency and -turn the function into ``address_space_write_rom`` with an API -matching ``address_space_write``. - -``cpu_physical_memory_write_rom`` - - ``cpu_memory_rw_debug`` ~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/exec.c b/exec.c index 92679508ba..6e875f0640 100644 --- a/exec.c +++ b/exec.c @@ -3430,11 +3430,12 @@ static inline MemTxResult address_space_write_rom_internal(AddressSpace *as, } /* used for ROM loading : can write in RAM and ROM */ -void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr, - const uint8_t *buf, int len) +MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr, + MemTxAttrs attrs, + const uint8_t *buf, int len) { - address_space_write_rom_internal(as, addr, MEMTXATTRS_UNSPECIFIED, - buf, len, WRITE_DATA); + return address_space_write_rom_internal(as, addr, attrs, + buf, len, WRITE_DATA); } void cpu_flush_icache_range(hwaddr start, int len) @@ -3879,8 +3880,9 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, l = len; phys_addr += (addr & ~TARGET_PAGE_MASK); if (is_write) { - cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as, - phys_addr, buf, l); + address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr, + MEMTXATTRS_UNSPECIFIED, + buf, l); } else { address_space_rw(cpu->cpu_ases[asidx].as, phys_addr, MEMTXATTRS_UNSPECIFIED, diff --git a/hw/core/loader.c b/hw/core/loader.c index aa0b3fc867..66a616608a 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1103,8 +1103,8 @@ static void rom_reset(void *unused) void *host = memory_region_get_ram_ptr(rom->mr); memcpy(host, rom->data, rom->datasize); } else { - cpu_physical_memory_write_rom(rom->as, rom->addr, rom->data, - rom->datasize); + address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED, + rom->data, rom->datasize); } if (rom->isrom) { /* rom needs to be written only once */ diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 97ffdd820f..c9dd65b3a0 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -122,9 +122,10 @@ static void apic_sync_vapic(APICCommonState *s, int sync_type) } vapic_state.irr = vector & 0xff; - cpu_physical_memory_write_rom(&address_space_memory, - s->vapic_paddr + start, - ((void *)&vapic_state) + start, length); + address_space_write_rom(&address_space_memory, + s->vapic_paddr + start, + MEMTXATTRS_UNSPECIFIED, + ((void *)&vapic_state) + start, length); } } diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c index e0c58ba37e..fb48a1540b 100644 --- a/hw/misc/tz-mpc.c +++ b/hw/misc/tz-mpc.c @@ -448,7 +448,7 @@ static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs) { /* We treat unspecified attributes like secure. Transactions with * unspecified attributes come from places like - * cpu_physical_memory_write_rom() for initial image load, and we want + * rom_reset() for initial image load, and we want * those to pass through the from-reset "everything is secure" config. * All the real during-emulation transactions from the CPU will * specify attributes. diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c index 3c29b68e67..639906cca3 100644 --- a/hw/sparc/sun4m.c +++ b/hw/sparc/sun4m.c @@ -559,8 +559,9 @@ static void idreg_init(hwaddr addr) s = SYS_BUS_DEVICE(dev); sysbus_mmio_map(s, 0, addr); - cpu_physical_memory_write_rom(&address_space_memory, - addr, idreg_data, sizeof(idreg_data)); + address_space_write_rom(&address_space_memory, addr, + MEMTXATTRS_UNSPECIFIED, + idreg_data, sizeof(idreg_data)); } #define MACIO_ID_REGISTER(obj) \ diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index 18b40d6145..2ad2d6d86b 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -111,8 +111,6 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr); */ void qemu_flush_coalesced_mmio_buffer(void); -void cpu_physical_memory_write_rom(AddressSpace *as, hwaddr addr, - const uint8_t *buf, int len); void cpu_flush_icache_range(hwaddr start, int len); extern struct MemoryRegion io_mem_rom; diff --git a/include/exec/memory.h b/include/exec/memory.h index 8e61450de3..ffd23ed8d8 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1792,6 +1792,32 @@ MemTxResult address_space_write(AddressSpace *as, hwaddr addr, MemTxAttrs attrs, const uint8_t *buf, int len); +/** + * address_space_write_rom: write to address space, including ROM. + * + * This function writes to the specified address space, but will + * write data to both ROM and RAM. This is used for non-guest + * writes like writes from the gdb debug stub or initial loading + * of ROM contents. + * + * Note that portions of the write which attempt to write data to + * a device will be silently ignored -- only real RAM and ROM will + * be written to. + * + * Return a MemTxResult indicating whether the operation succeeded + * or failed (eg unassigned memory, device rejected the transaction, + * IOMMU fault). + * + * @as: #AddressSpace to be accessed + * @addr: address within that address space + * @attrs: memory transaction attributes + * @buf: buffer with the data transferred + * @len: the number of bytes to write + */ +MemTxResult address_space_write_rom(AddressSpace *as, hwaddr addr, + MemTxAttrs attrs, + const uint8_t *buf, int len); + /* address_space_ld*: load from an address space * address_space_st*: store to an address space * From f2c6abc8d5d7f04e807bf91430a26f5548997783 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:49 +0000 Subject: [PATCH 04/22] disas.c: Use address_space_read() to read memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently disas.c reads physical memory using cpu_physical_memory_read(). This effectively hard-codes assuming that all CPUs have the same view of physical memory. Switch to address_space_read() instead, which lets us use the AddressSpace for the CPU we're disassembling for. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181122172653.3413-2-peter.maydell@linaro.org --- disas.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/disas.c b/disas.c index 5325b7e6be..f9c517b358 100644 --- a/disas.c +++ b/disas.c @@ -588,7 +588,10 @@ static int physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length, struct disassemble_info *info) { - cpu_physical_memory_read(memaddr, myaddr, length); + CPUDebug *s = container_of(info, CPUDebug, info); + + address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED, + myaddr, length); return 0; } From 6f89ae5816e38cd93419d2c8e8465b6dea00abee Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:49 +0000 Subject: [PATCH 05/22] monitor: Use address_space_read() to read memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently monitor.c reads physical memory using cpu_physical_memory_read(). This effectively hard-codes assuming that all CPUs have the same view of physical memory. Switch to address_space_read() instead, which lets us use the AddressSpace for the CPU we're reading memory for (falling back to address_space_memory if there is no CPU, as happens with the "none" board). As a bonus, this allows us to detect failures to read memory. Signed-off-by: Peter Maydell Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181122172653.3413-3-peter.maydell@linaro.org --- monitor.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/monitor.c b/monitor.c index 6e81b09294..4c8d8c2a5a 100644 --- a/monitor.c +++ b/monitor.c @@ -1605,7 +1605,13 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize, if (l > line_size) l = line_size; if (is_physical) { - cpu_physical_memory_read(addr, buf, l); + AddressSpace *as = cs ? cs->as : &address_space_memory; + MemTxResult r = address_space_read(as, addr, + MEMTXATTRS_UNSPECIFIED, buf, l); + if (r != MEMTX_OK) { + monitor_printf(mon, " Cannot access memory\n"); + break; + } } else { if (cpu_memory_rw_debug(cs, addr, buf, l, 0) < 0) { monitor_printf(mon, " Cannot access memory\n"); From ed31504097cd038097a720efd82a69273c04cf61 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:49 +0000 Subject: [PATCH 06/22] elf_ops.h: Use address_space_write() to write memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the load_elf function in elf_ops.h uses cpu_physical_memory_write() to write the ELF file to memory if it is not handling it as a ROM blob. This means we ignore the AddressSpace that the function is passed to define where it should be loaded. Use address_space_write() instead. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181122172653.3413-4-peter.maydell@linaro.org --- include/hw/elf_ops.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index 81cecaf27e..74679ff8da 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -482,7 +482,9 @@ static int glue(load_elf, SZ)(const char *name, int fd, rom_add_elf_program(label, data, file_size, mem_size, addr, as); } else { - cpu_physical_memory_write(addr, data, file_size); + address_space_write(as ? as : &address_space_memory, + addr, MEMTXATTRS_UNSPECIFIED, + data, file_size); g_free(data); } } From 9776874f0339ae0c9e45fb3cb674721384dcadba Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:50 +0000 Subject: [PATCH 07/22] hw/ppc/mac_newworld, mac_oldworld: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Use the glib g_file_get_contents() function instead, which does the whole "allocate memory for the file and read it in" operation. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Acked-by: David Gibson Message-id: 20181130151712.2312-2-peter.maydell@linaro.org --- hw/ppc/mac_newworld.c | 10 ++++------ hw/ppc/mac_oldworld.c | 10 ++++------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c index 14273a123e..7e45afae7c 100644 --- a/hw/ppc/mac_newworld.c +++ b/hw/ppc/mac_newworld.c @@ -127,8 +127,7 @@ static void ppc_core99_init(MachineState *machine) MACIOIDEState *macio_ide; BusState *adb_bus; MacIONVRAMState *nvr; - int bios_size, ndrv_size; - uint8_t *ndrv_file; + int bios_size; int ppc_boot_device; DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; void *fw_cfg; @@ -510,11 +509,10 @@ static void ppc_core99_init(MachineState *machine) /* MacOS NDRV VGA driver */ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME); if (filename) { - ndrv_size = get_image_size(filename); - if (ndrv_size != -1) { - ndrv_file = g_malloc(ndrv_size); - ndrv_size = load_image(filename, ndrv_file); + gchar *ndrv_file; + gsize ndrv_size; + if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL)) { fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file, ndrv_size); } g_free(filename); diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c index 9891c325a9..817f70e52c 100644 --- a/hw/ppc/mac_oldworld.c +++ b/hw/ppc/mac_oldworld.c @@ -99,8 +99,7 @@ static void ppc_heathrow_init(MachineState *machine) SysBusDevice *s; DeviceState *dev, *pic_dev; BusState *adb_bus; - int bios_size, ndrv_size; - uint8_t *ndrv_file; + int bios_size; uint16_t ppc_boot_device; DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; void *fw_cfg; @@ -361,11 +360,10 @@ static void ppc_heathrow_init(MachineState *machine) /* MacOS NDRV VGA driver */ filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, NDRV_VGA_FILENAME); if (filename) { - ndrv_size = get_image_size(filename); - if (ndrv_size != -1) { - ndrv_file = g_malloc(ndrv_size); - ndrv_size = load_image(filename, ndrv_file); + gchar *ndrv_file; + gsize ndrv_size; + if (g_file_get_contents(filename, &ndrv_file, &ndrv_size, NULL)) { fw_cfg_add_file(fw_cfg, "ndrv/qemu_vga.ndrv", ndrv_file, ndrv_size); } g_free(filename); From 214b63cd9310d770b22262dbff4c113a98f61503 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:50 +0000 Subject: [PATCH 08/22] hw/ppc/ppc405_boards: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Acked-by: David Gibson Message-id: 20181130151712.2312-3-peter.maydell@linaro.org --- hw/ppc/ppc405_boards.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c index 3be3fe4432..1b0a0a8ba3 100644 --- a/hw/ppc/ppc405_boards.c +++ b/hw/ppc/ppc405_boards.c @@ -219,9 +219,11 @@ static void ref405ep_init(MachineState *machine) bios_name = BIOS_FILENAME; filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); if (filename) { - bios_size = load_image(filename, memory_region_get_ram_ptr(bios)); + bios_size = load_image_size(filename, + memory_region_get_ram_ptr(bios), + BIOS_SIZE); g_free(filename); - if (bios_size < 0 || bios_size > BIOS_SIZE) { + if (bios_size < 0) { error_report("Could not load PowerPC BIOS '%s'", bios_name); exit(1); } @@ -515,9 +517,11 @@ static void taihu_405ep_init(MachineState *machine) &error_fatal); filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); if (filename) { - bios_size = load_image(filename, memory_region_get_ram_ptr(bios)); + bios_size = load_image_size(filename, + memory_region_get_ram_ptr(bios), + BIOS_SIZE); g_free(filename); - if (bios_size < 0 || bios_size > BIOS_SIZE) { + if (bios_size < 0) { error_report("Could not load PowerPC BIOS '%s'", bios_name); exit(1); } From b7abb791e4f7b0ba626f6658dd4a4e9201435f88 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:50 +0000 Subject: [PATCH 09/22] hw/smbios/smbios.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-4-peter.maydell@linaro.org --- hw/smbios/smbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index 920939454e..04811279a0 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -982,7 +982,7 @@ void smbios_entry_add(QemuOpts *opts, Error **errp) header = (struct smbios_structure_header *)(smbios_tables + smbios_tables_len); - if (load_image(val, (uint8_t *)header) != size) { + if (load_image_size(val, (uint8_t *)header, size) != size) { error_setg(errp, "Failed to load SMBIOS file %s", val); return; } From 36bde0911f7a1eec2ea0a9e1405d69b922d63f25 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:51 +0000 Subject: [PATCH 10/22] hw/pci/pci.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). While we are converting this code, add an error-check for read failure. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-5-peter.maydell@linaro.org --- hw/pci/pci.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 56b13b3320..efb5ce196f 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2261,7 +2261,11 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, pdev->has_rom = true; memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, size, &error_fatal); ptr = memory_region_get_ram_ptr(&pdev->rom); - load_image(path, ptr); + if (load_image_size(path, ptr, size) < 0) { + error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile); + g_free(path); + return; + } g_free(path); if (is_default_rom) { From c24323dd5f04d2a8c89f1d0d8daf0322b6fa6fc7 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:51 +0000 Subject: [PATCH 11/22] hw/i386/pc.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Use the glib g_file_get_contents() function instead, which does the whole "allocate memory for the file and read it in" operation. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-6-peter.maydell@linaro.org --- hw/i386/pc.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 4cd2fbca4d..115bc2825c 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -839,10 +839,9 @@ static void load_linux(PCMachineState *pcms, { uint16_t protocol; int setup_size, kernel_size, cmdline_size; - int64_t initrd_size = 0; int dtb_size, setup_data_offset; uint32_t initrd_max; - uint8_t header[8192], *setup, *kernel, *initrd_data; + uint8_t header[8192], *setup, *kernel; hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0; FILE *f; char *vmode; @@ -965,27 +964,30 @@ static void load_linux(PCMachineState *pcms, /* load initrd */ if (initrd_filename) { + gsize initrd_size; + gchar *initrd_data; + GError *gerr = NULL; + if (protocol < 0x200) { fprintf(stderr, "qemu: linux kernel too old to load a ram disk\n"); exit(1); } - initrd_size = get_image_size(initrd_filename); - if (initrd_size < 0) { + if (!g_file_get_contents(initrd_filename, &initrd_data, + &initrd_size, &gerr)) { fprintf(stderr, "qemu: error reading initrd %s: %s\n", - initrd_filename, strerror(errno)); + initrd_filename, gerr->message); exit(1); - } else if (initrd_size >= initrd_max) { + } + if (initrd_size >= initrd_max) { fprintf(stderr, "qemu: initrd is too large, cannot support." - "(max: %"PRIu32", need %"PRId64")\n", initrd_max, initrd_size); + "(max: %"PRIu32", need %"PRId64")\n", + initrd_max, (uint64_t)initrd_size); exit(1); } initrd_addr = (initrd_max-initrd_size) & ~4095; - initrd_data = g_malloc(initrd_size); - load_image(initrd_filename, initrd_data); - fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size); From 5250b09e57d9989efc69353d18416aedde00995e Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:51 +0000 Subject: [PATCH 12/22] hw/i386/multiboot.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). While we are converting the code, add the missing error check. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-7-peter.maydell@linaro.org --- hw/i386/multiboot.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/i386/multiboot.c b/hw/i386/multiboot.c index 1a4344f5fc..62340687e8 100644 --- a/hw/i386/multiboot.c +++ b/hw/i386/multiboot.c @@ -343,7 +343,11 @@ int load_multiboot(FWCfgState *fw_cfg, mbs.mb_buf_size = TARGET_PAGE_ALIGN(mb_mod_length + mbs.mb_buf_size); mbs.mb_buf = g_realloc(mbs.mb_buf, mbs.mb_buf_size); - load_image(one_file, (unsigned char *)mbs.mb_buf + offs); + if (load_image_size(one_file, (unsigned char *)mbs.mb_buf + offs, + mbs.mb_buf_size - offs) < 0) { + error_report("Error loading file '%s'", one_file); + exit(1); + } mb_add_mod(&mbs, mbs.mb_buf_phys + offs, mbs.mb_buf_phys + offs + mb_mod_length, c); From 93e8c20110c69dbaa711838a14b131bdb6fab643 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:52 +0000 Subject: [PATCH 13/22] hw/block/tc58128.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-8-peter.maydell@linaro.org --- hw/block/tc58128.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/block/tc58128.c b/hw/block/tc58128.c index 808ad76ba6..d0fae248dc 100644 --- a/hw/block/tc58128.c +++ b/hw/block/tc58128.c @@ -38,7 +38,8 @@ static void init_dev(tc58128_dev * dev, const char *filename) memset(dev->flash_contents, 0xff, FLASH_SIZE); if (filename) { /* Load flash image skipping the first block */ - ret = load_image(filename, dev->flash_contents + 528 * 32); + ret = load_image_size(filename, dev->flash_contents + 528 * 32, + FLASH_SIZE - 528 * 32); if (ret < 0) { if (!qtest_enabled()) { error_report("Could not load flash image %s", filename); From da885fe1ee8b4589047484bd7fa05a4905b52b17 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:52 +0000 Subject: [PATCH 14/22] device_tree.c: Don't use load_image() The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Instead use load_image_size(). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-9-peter.maydell@linaro.org --- device_tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/device_tree.c b/device_tree.c index 6d9c9726f6..296278e12a 100644 --- a/device_tree.c +++ b/device_tree.c @@ -91,7 +91,7 @@ void *load_device_tree(const char *filename_path, int *sizep) /* First allocate space in qemu for device tree */ fdt = g_malloc0(dt_size); - dt_file_load_size = load_image(filename_path, fdt); + dt_file_load_size = load_image_size(filename_path, fdt, dt_size); if (dt_file_load_size < 0) { error_report("Unable to open device tree file '%s'", filename_path); From 2933f6980b82ed3dcf1289c8f729fb9ac63e52fa Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:53 +0000 Subject: [PATCH 15/22] hw/core/loader.c: Remove load_image() The load_image() function is now no longer used anywhere, so we can remove it completely. (Use load_image_size() or g_file_get_contents() instead.) Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-10-peter.maydell@linaro.org --- hw/core/loader.c | 25 ------------------------- include/hw/loader.h | 1 - 2 files changed, 26 deletions(-) diff --git a/hw/core/loader.c b/hw/core/loader.c index 66a616608a..fa41842280 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -73,31 +73,6 @@ int64_t get_image_size(const char *filename) return size; } -/* return the size or -1 if error */ -/* deprecated, because caller does not specify buffer size! */ -int load_image(const char *filename, uint8_t *addr) -{ - int fd, size; - fd = open(filename, O_RDONLY | O_BINARY); - if (fd < 0) - return -1; - size = lseek(fd, 0, SEEK_END); - if (size == -1) { - fprintf(stderr, "file %-20s: get size error: %s\n", - filename, strerror(errno)); - close(fd); - return -1; - } - - lseek(fd, 0, SEEK_SET); - if (read(fd, addr, size) != size) { - close(fd); - return -1; - } - close(fd); - return size; -} - /* return the size or -1 if error */ ssize_t load_image_size(const char *filename, void *addr, size_t size) { diff --git a/include/hw/loader.h b/include/hw/loader.h index 67a0af84ac..3766559bc2 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -11,7 +11,6 @@ * On error, errno is also set as appropriate. */ int64_t get_image_size(const char *filename); -int load_image(const char *filename, uint8_t *addr); /* deprecated */ ssize_t load_image_size(const char *filename, void *addr, size_t size); /**load_image_targphys_as: From 061923298fe34e1bf5f32006f8d725a547fc4118 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:53 +0000 Subject: [PATCH 16/22] include/hw/loader.h: Document load_image_size() Add a documentation comment for load_image_size(). Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Stefan Hajnoczi Reviewed-by: Michael S. Tsirkin Reviewed-by: Eric Blake Message-id: 20181130151712.2312-11-peter.maydell@linaro.org --- include/hw/loader.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/hw/loader.h b/include/hw/loader.h index 3766559bc2..0a0ad808ea 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -11,6 +11,22 @@ * On error, errno is also set as appropriate. */ int64_t get_image_size(const char *filename); +/** + * load_image_size: load an image file into specified buffer + * @filename: Path to the image file + * @addr: Buffer to load image into + * @size: Size of buffer in bytes + * + * Load an image file from disk into the specified buffer. + * If the image is larger than the specified buffer, only + * @size bytes are read (this is not considered an error). + * + * Prefer to use the GLib function g_file_get_contents() rather + * than a "get_image_size()/g_malloc()/load_image_size()" sequence. + * + * Returns the number of bytes read, or -1 on error. On error, + * errno is also set as appropriate. + */ ssize_t load_image_size(const char *filename, void *addr, size_t size); /**load_image_targphys_as: From ac87e5072e2cbfcf8e80caac7ef43ceb6914c7af Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:53 +0000 Subject: [PATCH 17/22] target/arm: Free name string in ARMCPRegInfo hashtable entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we add a new entry to the ARMCPRegInfo hash table in add_cpreg_to_hashtable(), we allocate memory for tehe ARMCPRegInfo struct itself, and we also g_strdup() the name string. So the hashtable's value destructor function must free the name string as well as the struct. Spotted by clang's leak sanitizer. The leak here is a small one-off leak at startup, because we don't support CPU hotplug, and so the only time when we destroy hash table entries is for the case where ARM_CP_OVERRIDE means we register a wildcard entry and then override it later. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181204132952.2601-2-peter.maydell@linaro.org --- target/arm/cpu.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 60411f6bfe..b84a6c0e67 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -642,6 +642,20 @@ uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz) return (Aff1 << ARM_AFF1_SHIFT) | Aff0; } +static void cpreg_hashtable_data_destroy(gpointer data) +{ + /* + * Destroy function for cpu->cp_regs hashtable data entries. + * We must free the name string because it was g_strdup()ed in + * add_cpreg_to_hashtable(). It's OK to cast away the 'const' + * from r->name because we know we definitely allocated it. + */ + ARMCPRegInfo *r = data; + + g_free((void *)r->name); + g_free(r); +} + static void arm_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -649,7 +663,7 @@ static void arm_cpu_initfn(Object *obj) cs->env_ptr = &cpu->env; cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, - g_free, g_free); + g_free, cpreg_hashtable_data_destroy); QLIST_INIT(&cpu->pre_el_change_hooks); QLIST_INIT(&cpu->el_change_hooks); From 7081e9b6b2d695c4367e6b9ed3e852bcb6f42907 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:54 +0000 Subject: [PATCH 18/22] hw/arm/mps2-tz.c: Free mscname string in make_dma() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clang leak sanitizer spots a (one-off, trivial) memory leak in make_dma() due to a missing free. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181204132952.2601-3-peter.maydell@linaro.org --- hw/arm/mps2-tz.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c index 6dd02ae47e..82b1d020a5 100644 --- a/hw/arm/mps2-tz.c +++ b/hw/arm/mps2-tz.c @@ -322,6 +322,7 @@ static MemoryRegion *make_dma(MPS2TZMachineState *mms, void *opaque, sysbus_connect_irq(s, 2, qdev_get_gpio_in_named(iotkitdev, "EXP_IRQ", 57 + i * 3)); + g_free(mscname); return sysbus_mmio_get_region(s, 0); } From c0983085d127b1efb287337de26679c051abda07 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:54 +0000 Subject: [PATCH 19/22] hw/sd/sdhci: Don't leak memory region in sdhci_sysbus_realize() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In sdhci_sysbus_realize() we override the initialization of s->iomem that sdhci_common_realize() performs. However we don't destroy the old memory region before reinitializing it, which means that the memory allocated for mr->name in memory_region_do_init() is leaked. Since sdhci_initfn() already initializes s->io_ops to &sdhci_mmio_ops, always use that in sdhci_common_realize() and remove the now-unnecessary reinitialization of the MMIO region from sdhci_sysbus_realize(). Spotted by clang's leak sanitizer. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181204132952.2601-4-peter.maydell@linaro.org --- hw/sd/sdhci.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 81bbf03279..83f1574ffd 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1371,7 +1371,7 @@ static void sdhci_common_realize(SDHCIState *s, Error **errp) s->buf_maxsz = sdhci_get_fifolen(s); s->fifo_buffer = g_malloc0(s->buf_maxsz); - memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci", + memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", SDHC_REGISTERS_MAP_SIZE); } @@ -1565,9 +1565,6 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp) sysbus_init_irq(sbd, &s->irq); - memory_region_init_io(&s->iomem, OBJECT(s), s->io_ops, s, "sdhci", - SDHC_REGISTERS_MAP_SIZE); - sysbus_init_mmio(sbd, &s->iomem); } From 0012eb98f165e98ede33b5b57fb1985f1fe224b7 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:54 +0000 Subject: [PATCH 20/22] tests/test-arm-mptimer: Don't leak string memory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test-arm-mptimer setup creates a lot of test names using g_strdup_printf() and never frees them. This is entirely harmless since it's one-shot test code, but it clutters up the output from clang's LeakSanitizer. Refactor to use a helper function so we can free the memory. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181204132952.2601-5-peter.maydell@linaro.org --- tests/test-arm-mptimer.c | 153 ++++++++++++++++++--------------------- 1 file changed, 69 insertions(+), 84 deletions(-) diff --git a/tests/test-arm-mptimer.c b/tests/test-arm-mptimer.c index cb8f2df914..156a39f50d 100644 --- a/tests/test-arm-mptimer.c +++ b/tests/test-arm-mptimer.c @@ -991,10 +991,25 @@ static void test_timer_zero_load_nonscaled_periodic_to_prescaled_oneshot(void) g_assert_cmpuint(timer_get_and_clr_int_sts(), ==, 0); } +/* + * Add a qtest test that comes in two versions: one with + * a timer scaler setting, and one with the timer nonscaled. + */ +static void add_scaler_test(const char *str, bool scale, + void (*fn)(const void *)) +{ + char *name; + int *scaler = scale ? &scaled : &nonscaled; + + name = g_strdup_printf("%s=%d", str, *scaler); + qtest_add_data_func(name, scaler, fn); + g_free(name); +} + int main(int argc, char **argv) { - int *scaler = &nonscaled; int ret; + int scale; g_test_init(&argc, &argv, NULL); @@ -1012,89 +1027,59 @@ int main(int argc, char **argv) qtest_add_func("mptimer/prescaler", test_timer_prescaler); qtest_add_func("mptimer/prescaler_on_the_fly", test_timer_prescaler_on_the_fly); -tests_with_prescaler_arg: - qtest_add_data_func( - g_strdup_printf("mptimer/oneshot scaler=%d", *scaler), - scaler, test_timer_oneshot); - qtest_add_data_func( - g_strdup_printf("mptimer/pause scaler=%d", *scaler), - scaler, test_timer_pause); - qtest_add_data_func( - g_strdup_printf("mptimer/reload scaler=%d", *scaler), - scaler, test_timer_reload); - qtest_add_data_func( - g_strdup_printf("mptimer/periodic scaler=%d", *scaler), - scaler, test_timer_periodic); - qtest_add_data_func( - g_strdup_printf("mptimer/oneshot_to_periodic scaler=%d", *scaler), - scaler, test_timer_oneshot_to_periodic); - qtest_add_data_func( - g_strdup_printf("mptimer/periodic_to_oneshot scaler=%d", *scaler), - scaler, test_timer_periodic_to_oneshot); - qtest_add_data_func( - g_strdup_printf("mptimer/set_oneshot_counter_to_0 scaler=%d", *scaler), - scaler, test_timer_set_oneshot_counter_to_0); - qtest_add_data_func( - g_strdup_printf("mptimer/set_periodic_counter_to_0 scaler=%d", *scaler), - scaler, test_timer_set_periodic_counter_to_0); - qtest_add_data_func( - g_strdup_printf("mptimer/noload_oneshot scaler=%d", *scaler), - scaler, test_timer_noload_oneshot); - qtest_add_data_func( - g_strdup_printf("mptimer/noload_periodic scaler=%d", *scaler), - scaler, test_timer_noload_periodic); - qtest_add_data_func( - g_strdup_printf("mptimer/zero_load_oneshot scaler=%d", *scaler), - scaler, test_timer_zero_load_oneshot); - qtest_add_data_func( - g_strdup_printf("mptimer/zero_load_periodic scaler=%d", *scaler), - scaler, test_timer_zero_load_periodic); - qtest_add_data_func( - g_strdup_printf("mptimer/zero_load_oneshot_to_nonzero scaler=%d", *scaler), - scaler, test_timer_zero_load_oneshot_to_nonzero); - qtest_add_data_func( - g_strdup_printf("mptimer/zero_load_periodic_to_nonzero scaler=%d", *scaler), - scaler, test_timer_zero_load_periodic_to_nonzero); - qtest_add_data_func( - g_strdup_printf("mptimer/nonzero_load_oneshot_to_zero scaler=%d", *scaler), - scaler, test_timer_nonzero_load_oneshot_to_zero); - qtest_add_data_func( - g_strdup_printf("mptimer/nonzero_load_periodic_to_zero scaler=%d", *scaler), - scaler, test_timer_nonzero_load_periodic_to_zero); - qtest_add_data_func( - g_strdup_printf("mptimer/set_periodic_counter_on_the_fly scaler=%d", *scaler), - scaler, test_timer_set_periodic_counter_on_the_fly); - qtest_add_data_func( - g_strdup_printf("mptimer/enable_and_set_counter scaler=%d", *scaler), - scaler, test_timer_enable_and_set_counter); - qtest_add_data_func( - g_strdup_printf("mptimer/set_counter_and_enable scaler=%d", *scaler), - scaler, test_timer_set_counter_and_enable); - qtest_add_data_func( - g_strdup_printf("mptimer/oneshot_with_counter_0_on_start scaler=%d", *scaler), - scaler, test_timer_oneshot_with_counter_0_on_start); - qtest_add_data_func( - g_strdup_printf("mptimer/periodic_with_counter_0_on_start scaler=%d", *scaler), - scaler, test_timer_periodic_with_counter_0_on_start); - qtest_add_data_func( - g_strdup_printf("mptimer/periodic_counter scaler=%d", *scaler), - scaler, test_periodic_counter); - qtest_add_data_func( - g_strdup_printf("mptimer/set_counter_periodic_with_zero_load scaler=%d", *scaler), - scaler, test_timer_set_counter_periodic_with_zero_load); - qtest_add_data_func( - g_strdup_printf("mptimer/set_oneshot_load_to_0 scaler=%d", *scaler), - scaler, test_timer_set_oneshot_load_to_0); - qtest_add_data_func( - g_strdup_printf("mptimer/set_periodic_load_to_0 scaler=%d", *scaler), - scaler, test_timer_set_periodic_load_to_0); - qtest_add_data_func( - g_strdup_printf("mptimer/zero_load_mode_switch scaler=%d", *scaler), - scaler, test_timer_zero_load_mode_switch); - - if (scaler == &nonscaled) { - scaler = &scaled; - goto tests_with_prescaler_arg; + for (scale = 0; scale < 2; scale++) { + add_scaler_test("mptimer/oneshot scaler", + scale, test_timer_oneshot); + add_scaler_test("mptimer/pause scaler", + scale, test_timer_pause); + add_scaler_test("mptimer/reload scaler", + scale, test_timer_reload); + add_scaler_test("mptimer/periodic scaler", + scale, test_timer_periodic); + add_scaler_test("mptimer/oneshot_to_periodic scaler", + scale, test_timer_oneshot_to_periodic); + add_scaler_test("mptimer/periodic_to_oneshot scaler", + scale, test_timer_periodic_to_oneshot); + add_scaler_test("mptimer/set_oneshot_counter_to_0 scaler", + scale, test_timer_set_oneshot_counter_to_0); + add_scaler_test("mptimer/set_periodic_counter_to_0 scaler", + scale, test_timer_set_periodic_counter_to_0); + add_scaler_test("mptimer/noload_oneshot scaler", + scale, test_timer_noload_oneshot); + add_scaler_test("mptimer/noload_periodic scaler", + scale, test_timer_noload_periodic); + add_scaler_test("mptimer/zero_load_oneshot scaler", + scale, test_timer_zero_load_oneshot); + add_scaler_test("mptimer/zero_load_periodic scaler", + scale, test_timer_zero_load_periodic); + add_scaler_test("mptimer/zero_load_oneshot_to_nonzero scaler", + scale, test_timer_zero_load_oneshot_to_nonzero); + add_scaler_test("mptimer/zero_load_periodic_to_nonzero scaler", + scale, test_timer_zero_load_periodic_to_nonzero); + add_scaler_test("mptimer/nonzero_load_oneshot_to_zero scaler", + scale, test_timer_nonzero_load_oneshot_to_zero); + add_scaler_test("mptimer/nonzero_load_periodic_to_zero scaler", + scale, test_timer_nonzero_load_periodic_to_zero); + add_scaler_test("mptimer/set_periodic_counter_on_the_fly scaler", + scale, test_timer_set_periodic_counter_on_the_fly); + add_scaler_test("mptimer/enable_and_set_counter scaler", + scale, test_timer_enable_and_set_counter); + add_scaler_test("mptimer/set_counter_and_enable scaler", + scale, test_timer_set_counter_and_enable); + add_scaler_test("mptimer/oneshot_with_counter_0_on_start scaler", + scale, test_timer_oneshot_with_counter_0_on_start); + add_scaler_test("mptimer/periodic_with_counter_0_on_start scaler", + scale, test_timer_periodic_with_counter_0_on_start); + add_scaler_test("mptimer/periodic_counter scaler", + scale, test_periodic_counter); + add_scaler_test("mptimer/set_counter_periodic_with_zero_load scaler", + scale, test_timer_set_counter_periodic_with_zero_load); + add_scaler_test("mptimer/set_oneshot_load_to_0 scaler", + scale, test_timer_set_oneshot_load_to_0); + add_scaler_test("mptimer/set_periodic_load_to_0 scaler", + scale, test_timer_set_periodic_load_to_0); + add_scaler_test("mptimer/zero_load_mode_switch scaler", + scale, test_timer_zero_load_mode_switch); } qtest_start("-machine vexpress-a9"); From 397cd31f01228c42d32261a6db3ead140cac8a7c Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Fri, 14 Dec 2018 13:30:55 +0000 Subject: [PATCH 21/22] target/arm: Create timers in realize, not init The timer_new() function allocates memory; this means that if we call it in the CPU's init method we would need to provide an instance_finalize method to free it. Defer the timer creation to the realize function instead. This fixes a memory leak spotted by clang LeakSanitizer when a CPU object is created for introspection. Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20181204132952.2601-6-peter.maydell@linaro.org --- target/arm/cpu.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index b84a6c0e67..0e7138c9bf 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -679,14 +679,6 @@ static void arm_cpu_initfn(Object *obj) qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4); } - cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - arm_gt_ptimer_cb, cpu); - cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - arm_gt_vtimer_cb, cpu); - cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - arm_gt_htimer_cb, cpu); - cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - arm_gt_stimer_cb, cpu); qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs, ARRAY_SIZE(cpu->gt_timer_outputs)); @@ -882,6 +874,15 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) return; } } + + cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, + arm_gt_ptimer_cb, cpu); + cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, + arm_gt_vtimer_cb, cpu); + cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, + arm_gt_htimer_cb, cpu); + cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, + arm_gt_stimer_cb, cpu); #endif cpu_exec_realizefn(cs, &local_err); From bbac02f1e8edfd0663543f6fdad1e7094d860b29 Mon Sep 17 00:00:00 2001 From: Eduardo Habkost Date: Fri, 14 Dec 2018 13:30:55 +0000 Subject: [PATCH 22/22] virt: Fix broken indentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I introduced indentation using tabs instead of spaces in another commit. Peter reported the problem, and I failed to fix that before sending my pull request. Reported-by: Peter Maydell Reviewed-by: Thomas Huth Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181212003147.29604-1-ehabkost@redhat.com Fixes: 951597607696 ("virt: Eliminate separate instance_init functions") Signed-off-by: Eduardo Habkost Signed-off-by: Peter Maydell --- hw/arm/virt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 17f1b49d11..5b678237b7 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1854,7 +1854,7 @@ static const TypeInfo virt_machine_info = { .instance_size = sizeof(VirtMachineState), .class_size = sizeof(VirtMachineClass), .class_init = virt_machine_class_init, - .instance_init = virt_instance_init, + .instance_init = virt_instance_init, .interfaces = (InterfaceInfo[]) { { TYPE_HOTPLUG_HANDLER }, { }