From f6c98f91f56031141a47f86225fdc30f0f9f84fb Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 6 Nov 2018 11:32:13 +0000 Subject: [PATCH 1/5] target/arm: Remove can't-happen if() from handle_vec_simd_shli() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In handle_vec_simd_shli() we have a check: if (size > 3 && !is_q) { unallocated_encoding(s); return; } However this can never be true, because we calculate int size = 32 - clz32(immh) - 1; where immh is a 4 bit field which we know cannot be all-zeroes. So the clz32() return must be in {28,29,30,31} and the resulting size is in {0,1,2,3}, and "size > 3" is never true. This unnecessary code confuses Coverity's analysis: in CID 1396476 it thinks we might later index off the end of an array because the condition implies that we might have a size > 3. Remove the code, and instead assert that the size is in [0..3], since the decode that enforces that is somewhat distant from this function. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée Tested-by: Alex Bennée Message-id: 20181030162517.21816-1-peter.maydell@linaro.org --- target/arm/translate-a64.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 88195ab949..fd36425f1a 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -9483,12 +9483,10 @@ static void handle_vec_simd_shli(DisasContext *s, bool is_q, bool insert, int immhb = immh << 3 | immb; int shift = immhb - (8 << size); - if (extract32(immh, 3, 1) && !is_q) { - unallocated_encoding(s); - return; - } + /* Range of size is limited by decode: immh is a non-zero 4 bit field */ + assert(size >= 0 && size <= 3); - if (size > 3 && !is_q) { + if (extract32(immh, 3, 1) && !is_q) { unallocated_encoding(s); return; } From 40af11eb7f80aac6c45d75e4fa6fa71ff930c651 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 6 Nov 2018 11:32:14 +0000 Subject: [PATCH 2/5] milkymist: Check for failure trying to load BIOS image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check the return value from load_image_targphys(), which tells us whether our attempt to load the BIOS image into RAM failed. (Spotted by Coverity, CID 1190305.) Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Acked-by: Michael Walle Message-id: 20181030170032.1844-1-peter.maydell@linaro.org --- hw/lm32/milkymist.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hw/lm32/milkymist.c b/hw/lm32/milkymist.c index 321f184595..63c6894c95 100644 --- a/hw/lm32/milkymist.c +++ b/hw/lm32/milkymist.c @@ -138,7 +138,10 @@ milkymist_init(MachineState *machine) bios_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); if (bios_filename) { - load_image_targphys(bios_filename, BIOS_OFFSET, BIOS_SIZE); + if (load_image_targphys(bios_filename, BIOS_OFFSET, BIOS_SIZE) < 0) { + error_report("could not load bios '%s'", bios_filename); + exit(1); + } } reset_info->bootstrap_pc = BIOS_OFFSET; From 76a82ced1184f26c1e2426938241442d80b1c13e Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 6 Nov 2018 11:32:14 +0000 Subject: [PATCH 3/5] hw/arm/exynos4210: Zero memory allocated for Exynos4210State MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In exynos4210_init() we allocate memory for an Exynos4210State struct. Generally devices can assume that the memory allocated for their state struct is zero-initialized; we broke that assumption here by using g_new(). Use g_new0() instead. (In particular, some code assumes that the various irq arrays in the Exynos4210Irq sub-struct are zero-initialized.) In the longer term, this code should be QOMified, and then the struct memory will be allocated elsewhere and by functions which always zero-initalize it; but for 3.1 this is a simple fix. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Tested-by: Philippe Mathieu-Daudé Message-id: 20181105151132.13884-1-peter.maydell@linaro.org --- hw/arm/exynos4210.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c index 827318a003..af82e95542 100644 --- a/hw/arm/exynos4210.c +++ b/hw/arm/exynos4210.c @@ -162,7 +162,7 @@ static uint64_t exynos4210_calc_affinity(int cpu) Exynos4210State *exynos4210_init(MemoryRegion *system_mem) { - Exynos4210State *s = g_new(Exynos4210State, 1); + Exynos4210State *s = g_new0(Exynos4210State, 1); qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS]; SysBusDevice *busdev; DeviceState *dev; From 0f7b791b35f24cb1333f779705a3f6472e6935de Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 6 Nov 2018 11:32:14 +0000 Subject: [PATCH 4/5] target/arm: Set S and PTW in 64-bit PAR format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In do_ats_write() we construct a PAR value based on the result of the translation. A comment says "S2WLK and FSTAGE are always zero, because we don't implement virtualization". Since we do in fact now implement virtualization, add the missing code that sets these bits based on the reported ARMMMUFaultInfo. (These bits are named PTW and S in ARMv8, so we follow that convention in the new comments in this patch.) Signed-off-by: Peter Maydell Reviewed-by: Edgar E. Iglesias Reviewed-by: Alex Bennée Message-id: 20181016093703.10637-2-peter.maydell@linaro.org --- target/arm/helper.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 0ea95b0815..69f684abd8 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2347,10 +2347,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, par64 |= 1; /* F */ par64 |= (fsr & 0x3f) << 1; /* FS */ - /* Note that S2WLK and FSTAGE are always zero, because we don't - * implement virtualization and therefore there can't be a stage 2 - * fault. - */ + if (fi.stage2) { + par64 |= (1 << 9); /* S */ + } + if (fi.s1ptw) { + par64 |= (1 << 8); /* PTW */ + } } } else { /* fsr is a DFSR/IFSR value for the short descriptor From 23463e0e4aeb2f0a9c60549a2c163f4adc0b8512 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 6 Nov 2018 11:32:14 +0000 Subject: [PATCH 5/5] target/arm: Fix ATS1Hx instructions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ATS1HR and ATS1HW (which allow AArch32 EL2 to do address translations on the EL2 translation regime) were implemented in commit 14db7fe09a2c8. However, we got them wrong: these should do stage 1 address translations as defined for NS-EL2, which is ARMMMUIdx_S1E2. We were incorrectly making them perform stage 2 translations. A few years later in commit 1313e2d7e2cd we forgot entirely that we'd implemented ATS1Hx, and added a comment that ATS1Hx were "not supported yet". Remove the comment; there is no extra code needed to handle these operations in do_ats_write(), because arm_s1_regime_using_lpae_format() returns true for ARMMMUIdx_S1E2, which forces 64-bit PAR format. Signed-off-by: Peter Maydell Reviewed-by: Alex Bennée Message-id: 20181016093703.10637-3-peter.maydell@linaro.org Reviewed-by: Edgar E. Iglesias --- target/arm/helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index 69f684abd8..96301930cc 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2319,7 +2319,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, * * (Note that HCR.DC makes HCR.VM behave as if it is 1.) * - * ATS1Hx always uses the 64bit format (not supported yet). + * ATS1Hx always uses the 64bit format. */ format64 = arm_s1_regime_using_lpae_format(env, mmu_idx); @@ -2444,7 +2444,7 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri, MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD; uint64_t par64; - par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S2NS); + par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S1E2); A32_BANKED_CURRENT_REG_SET(env, par, par64); }