From 7110776b01fac48e4a2f49b6b8d5020ecfc158a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 26 Sep 2019 15:19:54 +0400 Subject: [PATCH 1/5] tests: fix usb-hcd-ehci-test compilation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes commit e5758de4e836c3b2edc2befd904651fc6967d74f ("tests/libqtest: Make qtest_qmp_device_add/del independent from global_qtest") and commit dd210749727530cdef7c335040edbf81c3c5d041 ("tests/libqtest: Use libqtest-single.h in tests that require global_qtest"). Cc: Thomas Huth Signed-off-by: Marc-André Lureau Message-Id: <20190926111955.17276-2-marcandre.lureau@redhat.com> Reviewed-by: Laurent Vivier Signed-off-by: Thomas Huth --- tests/usb-hcd-ehci-test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c index 8bc3e44189..5251d539e9 100644 --- a/tests/usb-hcd-ehci-test.c +++ b/tests/usb-hcd-ehci-test.c @@ -8,7 +8,7 @@ */ #include "qemu/osdep.h" -#include "libqtest.h" +#include "libqtest-single.h" #include "libqos/pci-pc.h" #include "hw/usb/uhci-regs.h" #include "hw/usb/ehci-regs.h" @@ -139,7 +139,7 @@ static void pci_ehci_port_3_hotplug(void) static void pci_ehci_port_hotplug(void) { - usb_test_hotplug("ich9-ehci-1", "3", pci_ehci_port_3_hotplug); + usb_test_hotplug(global_qtest, "ich9-ehci-1", "3", pci_ehci_port_3_hotplug); } From 343143a6651bb440a1661fbaf163ef6d9b05721a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 26 Sep 2019 15:19:55 +0400 Subject: [PATCH 2/5] tests: fix echi/ehci typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While at it, simplify using $(land). Signed-off-by: Marc-André Lureau Message-Id: <20190926111955.17276-3-marcandre.lureau@redhat.com> Reviewed-by: Laurent Vivier Reviewed-by: Thomas Huth Fixes: dad5ddcea3b661 ("check: Only test usb-ehci when it is compiled in") Signed-off-by: Thomas Huth --- tests/Makefile.include | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index 0595914526..3543451ed3 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -182,9 +182,7 @@ check-qtest-i386-$(CONFIG_PVPANIC) += tests/pvpanic-test$(EXESUF) check-qtest-i386-$(CONFIG_I82801B11) += tests/i82801b11-test$(EXESUF) check-qtest-i386-$(CONFIG_IOH3420) += tests/ioh3420-test$(EXESUF) check-qtest-i386-$(CONFIG_USB_UHCI) += tests/usb-hcd-uhci-test$(EXESUF) -ifeq ($(CONFIG_USB_ECHI)$(CONFIG_USB_UHCI),yy) -check-qtest-i386-y += tests/usb-hcd-ehci-test$(EXESUF) -endif +check-qtest-i386-$(call land,$(CONFIG_USB_EHCI),$(CONFIG_USB_UHCI)) += tests/usb-hcd-ehci-test$(EXESUF) check-qtest-i386-$(CONFIG_USB_XHCI_NEC) += tests/usb-hcd-xhci-test$(EXESUF) check-qtest-i386-y += tests/cpu-plug-test$(EXESUF) check-qtest-i386-y += tests/q35-test$(EXESUF) From cd4fc142074f47fdcc850676a13d29bb3facc100 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Sat, 21 Sep 2019 11:17:38 +0200 Subject: [PATCH 3/5] hw/m68k/next-cube: Avoid static RTC variables and introduce control register Coverity currently complains that the "if (0x00 & (0x80 >> (phase - 8))" in next-cube.c can never be true. Right it is. The "0x00" is meant as value of the control register of the RTC, which is currently not implemented yet. Thus, let's add a register variable for this now. However, the RTC registers are currently defined as static variables in nextscr2_write(), which is quite ugly. Thus let's also move the RTC variables to the main machine state instead. In the long run, we should likely even refactor the whole RTC code into a separate device in a separate file, but that's something for calm winter nights later... as a first step, cleaning up the static variables and shutting up the warning from Coverity should be sufficient. Reviewed-by: Richard Henderson Message-Id: <20190921091738.26953-1-huth@tuxfamily.org> Signed-off-by: Thomas Huth --- hw/m68k/next-cube.c | 73 +++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c index 9a4a7328f9..e5343348d0 100644 --- a/hw/m68k/next-cube.c +++ b/hw/m68k/next-cube.c @@ -60,6 +60,15 @@ typedef struct next_dma { uint32_t size; } next_dma; +typedef struct NextRtc { + uint8_t ram[32]; + uint8_t command; + uint8_t value; + uint8_t status; + uint8_t control; + uint8_t retval; +} NextRtc; + typedef struct { MachineState parent; @@ -77,7 +86,7 @@ typedef struct { uint32_t scr1; uint32_t scr2; - uint8_t rtc_ram[32]; + NextRtc rtc; } NeXTState; /* Thanks to NeXT forums for this */ @@ -105,11 +114,8 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size) static int led; static int phase; static uint8_t old_scr2; - static uint8_t rtc_command; - static uint8_t rtc_value; - static uint8_t rtc_status = 0x90; - static uint8_t rtc_return; uint8_t scr2_2; + NextRtc *rtc = &s->rtc; if (size == 4) { scr2_2 = (val >> 8) & 0xFF; @@ -135,52 +141,52 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size) if (((old_scr2 & SCR2_RTCLK) != (scr2_2 & SCR2_RTCLK)) && ((scr2_2 & SCR2_RTCLK) == 0)) { if (phase < 8) { - rtc_command = (rtc_command << 1) | - ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->command = (rtc->command << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); } if (phase >= 8 && phase < 16) { - rtc_value = (rtc_value << 1) | ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->value = (rtc->value << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); /* if we read RAM register, output RT_DATA bit */ - if (rtc_command <= 0x1F) { + if (rtc->command <= 0x1F) { scr2_2 = scr2_2 & (~SCR2_RTDATA); - if (s->rtc_ram[rtc_command] & (0x80 >> (phase - 8))) { + if (rtc->ram[rtc->command] & (0x80 >> (phase - 8))) { scr2_2 |= SCR2_RTDATA; } - rtc_return = (rtc_return << 1) | - ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->retval = (rtc->retval << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); } /* read the status 0x30 */ - if (rtc_command == 0x30) { + if (rtc->command == 0x30) { scr2_2 = scr2_2 & (~SCR2_RTDATA); /* for now status = 0x98 (new rtc + FTU) */ - if (rtc_status & (0x80 >> (phase - 8))) { + if (rtc->status & (0x80 >> (phase - 8))) { scr2_2 |= SCR2_RTDATA; } - rtc_return = (rtc_return << 1) | - ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->retval = (rtc->retval << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); } /* read the status 0x31 */ - if (rtc_command == 0x31) { + if (rtc->command == 0x31) { scr2_2 = scr2_2 & (~SCR2_RTDATA); - /* for now 0x00 */ - if (0x00 & (0x80 >> (phase - 8))) { + if (rtc->control & (0x80 >> (phase - 8))) { scr2_2 |= SCR2_RTDATA; } - rtc_return = (rtc_return << 1) | - ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->retval = (rtc->retval << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); } - if ((rtc_command >= 0x20) && (rtc_command <= 0x2F)) { + if ((rtc->command >= 0x20) && (rtc->command <= 0x2F)) { scr2_2 = scr2_2 & (~SCR2_RTDATA); /* for now 0x00 */ time_t time_h = time(NULL); struct tm *info = localtime(&time_h); int ret = 0; - switch (rtc_command) { + switch (rtc->command) { case 0x20: ret = SCR2_TOBCD(info->tm_sec); break; @@ -205,22 +211,22 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size) if (ret & (0x80 >> (phase - 8))) { scr2_2 |= SCR2_RTDATA; } - rtc_return = (rtc_return << 1) | - ((scr2_2 & SCR2_RTDATA) ? 1 : 0); + rtc->retval = (rtc->retval << 1) | + ((scr2_2 & SCR2_RTDATA) ? 1 : 0); } } phase++; if (phase == 16) { - if (rtc_command >= 0x80 && rtc_command <= 0x9F) { - s->rtc_ram[rtc_command - 0x80] = rtc_value; + if (rtc->command >= 0x80 && rtc->command <= 0x9F) { + rtc->ram[rtc->command - 0x80] = rtc->value; } /* write to x30 register */ - if (rtc_command == 0xB1) { + if (rtc->command == 0xB1) { /* clear FTU */ - if (rtc_value & 0x04) { - rtc_status = rtc_status & (~0x18); + if (rtc->value & 0x04) { + rtc->status = rtc->status & (~0x18); s->int_status = s->int_status & (~0x04); } } @@ -229,8 +235,8 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size) } else { /* else end or abort */ phase = -1; - rtc_command = 0; - rtc_value = 0; + rtc->command = 0; + rtc->value = 0; } s->scr2 = val & 0xFFFF00FF; s->scr2 |= scr2_2 << 8; @@ -881,9 +887,10 @@ static void next_cube_init(MachineState *machine) /* 0x0000XX00 << vital bits */ ns->scr1 = 0x00011102; ns->scr2 = 0x00ff0c80; + ns->rtc.status = 0x90; /* Load RTC RAM - TODO: provide possibility to load contents from file */ - memcpy(ns->rtc_ram, rtc_ram2, 32); + memcpy(ns->rtc.ram, rtc_ram2, 32); /* 64MB RAM starting at 0x04000000 */ memory_region_allocate_system_memory(ram, NULL, "next.ram", ram_size); From e423455c4f23a1a828901c78fe6d03b7dde79319 Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Wed, 25 Sep 2019 14:16:43 +0200 Subject: [PATCH 4/5] hw/core/loader: Fix possible crash in rom_copy() Both, "rom->addr" and "addr" are derived from the binary image that can be loaded with the "-kernel" paramer. The code in rom_copy() then calculates: d = dest + (rom->addr - addr); and uses "d" as destination in a memcpy() some lines later. Now with bad kernel images, it is possible that rom->addr is smaller than addr, thus "rom->addr - addr" gets negative and the memcpy() then tries to copy contents from the image to a bad memory location. This could maybe be used to inject code from a kernel image into the QEMU binary, so we better fix it with an additional sanity check here. Cc: qemu-stable@nongnu.org Reported-by: Guangming Liu Buglink: https://bugs.launchpad.net/qemu/+bug/1844635 Message-Id: <20190925130331.27825-1-thuth@redhat.com> Reviewed-by: Michael S. Tsirkin Signed-off-by: Thomas Huth --- hw/core/loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/core/loader.c b/hw/core/loader.c index 0d60219364..5099f27dc8 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size) if (rom->addr + rom->romsize < addr) { continue; } - if (rom->addr > end) { + if (rom->addr > end || rom->addr < addr) { break; } From 3d5e90a50bd4ffa199166e01a365f5c5995534ae Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Mon, 23 Sep 2019 14:00:29 +0200 Subject: [PATCH 5/5] Disallow colons in the parameter of "-accel" Everybody who used something like "-machine accel=kvm:tcg" in the past might be tempted to specify a similar list with the -accel parameter, too, for example "-accel kvm:tcg". However, this is not how this options is thought to be used, since each "-accel" should only take care of one specific accelerator. In the long run, we really should rework the "-accel" code completely, so that it does not set "-machine accel=..." anymore internally, but is completely independent from "-machine". For the short run, let's make sure that users cannot use "-accel xyz:tcg", so that we avoid that we have to deal with such cases in the wild later. Message-Id: <20190930123505.11607-1-thuth@redhat.com> Signed-off-by: Thomas Huth --- tests/cdrom-test.c | 2 +- vl.c | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c index 05611da648..34e9974634 100644 --- a/tests/cdrom-test.c +++ b/tests/cdrom-test.c @@ -120,7 +120,7 @@ static void test_cdboot(gconstpointer data) { QTestState *qts; - qts = qtest_initf("-accel kvm:tcg -no-shutdown %s%s", (const char *)data, + qts = qtest_initf("-M accel=kvm:tcg -no-shutdown %s%s", (const char *)data, isoimage); boot_sector_test(qts); qtest_quit(qts); diff --git a/vl.c b/vl.c index 630f5c5e9c..002bf4919e 100644 --- a/vl.c +++ b/vl.c @@ -3554,6 +3554,11 @@ int main(int argc, char **argv, char **envp) g_slist_free(accel_list); exit(0); } + if (optarg && strchr(optarg, ':')) { + error_report("Don't use ':' with -accel, " + "use -M accel=... for now instead"); + exit(1); + } opts = qemu_opts_create(qemu_find_opts("machine"), NULL, false, &error_abort); qemu_opt_set(opts, "accel", optarg, &error_abort);