[PULL 0/5] qtest and misc patches

[PULL 0/5] qtest and misc patches

[Thomas Huth]

 Hi Peter!

The following changes since commit 95e9d74fe4281f7ad79a5a7511400541729aa44a:

  Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20190930' into staging (2019-09-30 14:21:56 +0100)

are available in the Git repository at:

  https://gitlab.com/huth/qemu.git tags/pull-request-2019-10-01

for you to fetch changes up to 3d5e90a50bd4ffa199166e01a365f5c5995534ae:

  Disallow colons in the parameter of "-accel" (2019-10-01 11:54:17 +0200)

----------------------------------------------------------------
- Fix and re-enable the usb-hcd-ehci-test
- Silence a Coverity warning in hw/m68k/next-cube.c
- Fix crash that can occur when using bad binaries with "-kernel"
- Disallow colons in the "-accel" parameter
----------------------------------------------------------------

Marc-André Lureau (2):
      tests: fix usb-hcd-ehci-test compilation
      tests: fix echi/ehci typo

Thomas Huth (3):
      hw/m68k/next-cube: Avoid static RTC variables and introduce control register
      hw/core/loader: Fix possible crash in rom_copy()
      Disallow colons in the parameter of "-accel"

 hw/core/loader.c          |  2 +-
 hw/m68k/next-cube.c       | 73 ++++++++++++++++++++++++++---------------------
 tests/Makefile.include    |  4 +--
 tests/cdrom-test.c        |  2 +-
 tests/usb-hcd-ehci-test.c |  4 +--
 vl.c                      |  5 ++++
 6 files changed, 50 insertions(+), 40 deletions(-)

[PULL 1/5] tests: fix usb-hcd-ehci-test compilation

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

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 <thuth@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190926111955.17276-2-marcandre.lureau@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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);
 }
 
 
-- 
2.18.1

[PULL 2/5] tests: fix echi/ehci typo

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

While at it, simplify using $(land).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190926111955.17276-3-marcandre.lureau@redhat.com>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Fixes: dad5ddcea3b661 ("check: Only test usb-ehci when it is compiled in")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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)
-- 
2.18.1

[PULL 3/5] hw/m68k/next-cube: Avoid static RTC variables and introduce control register

[Thomas Huth]

From: Thomas Huth <huth@tuxfamily.org>

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 <richard.henderson@linaro.org>
Message-Id: <20190921091738.26953-1-huth@tuxfamily.org>
Signed-off-by: Thomas Huth <huth@tuxfamily.org>
---
 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);
-- 
2.18.1

[PULL 4/5] hw/core/loader: Fix possible crash in rom_copy()

[Thomas Huth]

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 <mst@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 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;
         }
 
-- 
2.18.1

[PULL 5/5] Disallow colons in the parameter of "-accel"

[Thomas Huth]

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 <thuth@redhat.com>
---
 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);
-- 
2.18.1

Re: [PULL 0/5] qtest and misc patches

[Peter Maydell]

On Tue, 1 Oct 2019 at 11:51, Thomas Huth <thuth@redhat.com> wrote:
>
>  Hi Peter!
>
> The following changes since commit 95e9d74fe4281f7ad79a5a7511400541729aa44a:
>
>   Merge remote-tracking branch 'remotes/borntraeger/tags/s390x-20190930' into staging (2019-09-30 14:21:56 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/huth/qemu.git tags/pull-request-2019-10-01
>
> for you to fetch changes up to 3d5e90a50bd4ffa199166e01a365f5c5995534ae:
>
>   Disallow colons in the parameter of "-accel" (2019-10-01 11:54:17 +0200)
>
> ----------------------------------------------------------------
> - Fix and re-enable the usb-hcd-ehci-test
> - Silence a Coverity warning in hw/m68k/next-cube.c
> - Fix crash that can occur when using bad binaries with "-kernel"
> - Disallow colons in the "-accel" parameter
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM