[PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[Christian Quante]

The test cases in fdc-test share one QEMU instance and, with it, the
state of the floppy controller.  Several of them only pass in the order
they happen to be registered in: read_no_dma_1, _18 and _19 fail when
run on their own, and fuzz-registers leaves an unacknowledged interrupt
behind, so it works only where it sits today.

This takes up Kevin Wolf's invitation from the review of d00567f7fb and
431f59a34d: "If you feel like addressing the wider ordering problem in
the tests, too, you're welcome to do so."

Patch 1 registers the test cases from a table and runs each one in a
QEMU instance of its own.  No test function changes what it tests; the
three read_no_dma cases insert their own medium instead of inheriting
one, and test_read_id stops ejecting at the end.

Patch 2 skips the test cases when the pc machine is not part of the
build, after 4e37f749dd, 6500a6abe6 and bb98963d6d did the same for
drive_del-test, device-plug-test and pxe-test.  Patch 1 routes every
test case through a single function, so one check covers all of them.

Patch 3 adds a test case that keeps d00567f7fb ("hw/block/fdc: select
the drive named by the READ ID command") from being lost again -- with
one drive, answering for the wrong one cannot be told from answering
for the right one.  It needs a machine with two drives, which is what
the table's args field is for.

Measured on this series:

  * every test case passes on its own; on master three of them fail
  * the suite passes with the registration order reversed; on master it
    aborts in send_seek()
  * removing SET_CUR_DRV() from fdctrl_handle_readid() again turns
    exactly one test case red -- the new one -- and leaves the other
    sixteen green
  * each of the three commits builds and passes on its own

The price is wall-clock time: the fifteen test cases in the table now
start an instance each instead of sharing one, which takes the suite
from 0.78s to 2.8s.  The qtest timeout is 60s.

The pattern is not new here.  Of the 195 files in tests/qtest, 57
register their test cases with qtest_add_data_func() and none uses glib
fixtures; boot-serial-test.c is the closest model, with a table of test
definitions and a shared runner that starts one instance per entry.  72
files still include libqtest-single.h, which is why the test functions
keep reaching their instance through global_qtest instead of carrying a
QTestState around -- that would have touched every one of them.

Christian Quante (3):
  tests/qtest/fdc-test: give each test case its own QEMU instance
  tests/qtest/fdc-test: skip test cases when the 'pc' machine is missing
  tests/qtest/fdc-test: check that READ ID answers for the drive it
    names

 tests/qtest/fdc-test.c | 127 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 107 insertions(+), 20 deletions(-)


base-commit: eea8fe61b8be8f3016e522e6af24924a0266ca95
-- 
2.53.0

[PATCH 1/3] tests/qtest/fdc-test: give each test case its own QEMU instance

[Christian Quante]

The test cases share one QEMU instance and, with it, the state of the
floppy controller.  Several of them therefore only pass in the order they
happen to be registered in: test_read_no_dma_1, _18 and _19 read the medium
that test_media_insert leaves in the drive, so running any of them on its
own fails.  fuzz-registers is worse -- it writes 1000 random values to the
eight I/O ports at FLOPPY_BASE and restores none of them, which is harmless
only because it is the last test case to run in the shared instance; the
two registered after it start one of their own.  Moving it anywhere else
leaves an unacknowledged interrupt behind and the next test dies in
send_seek().

Register the test cases from a table instead, through a wrapper that starts
a QEMU instance, runs one test and stops it again.  The test functions are
unchanged: they reach the current instance through global_qtest, which
qtest_start() reassigns for every test.  The three read_no_dma tests now
insert their own medium, and test_read_id no longer ejects at the end -- it
did that only to leave the drive as the next test expected to find it.

The two CVE test cases keep qtest_add_func(); they already run a QEMU
instance of their own and do not use global_qtest.

The same pattern is used by tests/qtest/boot-serial-test.c.

Every test case now passes on its own, and the suite passes with the
registration order reversed.

Signed-off-by: Christian Quante <christian@quante.one>
---
 tests/qtest/fdc-test.c | 71 ++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 20 deletions(-)

diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index 1e1dd8659d..fd38475bf7 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -510,9 +510,6 @@ static void test_read_id(void)
     g_assert_cmpint(cyl, ==, 8);
     g_assert_cmpint(head, ==, 1);
     g_assert_cmpint(st0, ==, head << 2);
-
-    /* Leave the drive empty, the way the machine starts up. */
-    media_eject();
 }
 
 /*
@@ -552,6 +549,7 @@ static void test_read_no_dma_1(void)
 {
     uint8_t ret;
 
+    media_insert();
     outb(FLOPPY_BASE + reg_dor, inb(FLOPPY_BASE + reg_dor) & ~0x08);
     send_seek(0);
     ret = send_read_no_dma_command(1, 0x04);
@@ -562,6 +560,7 @@ static void test_read_no_dma_18(void)
 {
     uint8_t ret;
 
+    media_insert();
     outb(FLOPPY_BASE + reg_dor, inb(FLOPPY_BASE + reg_dor) & ~0x08);
     send_seek(0);
     ret = send_read_no_dma_command(18, 0x04);
@@ -572,6 +571,7 @@ static void test_read_no_dma_19(void)
 {
     uint8_t ret;
 
+    media_insert();
     outb(FLOPPY_BASE + reg_dor, inb(FLOPPY_BASE + reg_dor) & ~0x08);
     send_seek(0);
     ret = send_read_no_dma_command(19, 0x20);
@@ -656,10 +656,53 @@ static void test_cve_2021_3507(void)
     qtest_quit(s);
 }
 
+/*
+ * Each test gets its own QEMU instance, so that none of them depends on the
+ * state another one left behind.  Without this, several tests only pass in
+ * the order they happen to be registered in: the read_no_dma tests need the
+ * medium that test_media_insert leaves in the drive, and fuzz-registers
+ * writes random values to the eight I/O ports at FLOPPY_BASE and restores
+ * none of them.
+ */
+typedef struct {
+    const char *path;
+    void (*fn)(void);
+} FDCTest;
+
+static const FDCTest fdc_tests[] = {
+    { "/fdc/cmos",              test_cmos },
+    { "/fdc/no_media_on_start", test_no_media_on_start },
+    { "/fdc/read_without_media", test_read_without_media },
+    { "/fdc/media_change",      test_media_change },
+    { "/fdc/sense_interrupt",   test_sense_interrupt },
+    { "/fdc/relative_seek",     test_relative_seek },
+    { "/fdc/read_id",           test_read_id },
+    { "/fdc/read_id_no_media",  test_read_id_no_media },
+    { "/fdc/verify",            test_verify },
+    { "/fdc/media_insert",      test_media_insert },
+    { "/fdc/read_no_dma_1",     test_read_no_dma_1 },
+    { "/fdc/read_no_dma_18",    test_read_no_dma_18 },
+    { "/fdc/read_no_dma_19",    test_read_no_dma_19 },
+    { "/fdc/fuzz-registers",    fuzz_registers },
+};
+
+static void run_isolated(const void *data)
+{
+    const FDCTest *test = data;
+
+    qtest_start("-machine pc -device floppy,id=floppy0");
+    qtest_irq_intercept_in(global_qtest, "ioapic");
+
+    test->fn();
+
+    qtest_end();
+}
+
 int main(int argc, char **argv)
 {
     int fd;
     int ret;
+    size_t i;
 
     /* Create a temporary raw image */
     fd = g_file_open_tmp("qtest.XXXXXX", &test_image, NULL);
@@ -671,29 +714,17 @@ int main(int argc, char **argv)
     /* Run the tests */
     g_test_init(&argc, &argv, NULL);
 
-    qtest_start("-machine pc -device floppy,id=floppy0");
-    qtest_irq_intercept_in(global_qtest, "ioapic");
-    qtest_add_func("/fdc/cmos", test_cmos);
-    qtest_add_func("/fdc/no_media_on_start", test_no_media_on_start);
-    qtest_add_func("/fdc/read_without_media", test_read_without_media);
-    qtest_add_func("/fdc/media_change", test_media_change);
-    qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
-    qtest_add_func("/fdc/relative_seek", test_relative_seek);
-    qtest_add_func("/fdc/read_id", test_read_id);
-    qtest_add_func("/fdc/read_id_no_media", test_read_id_no_media);
-    qtest_add_func("/fdc/verify", test_verify);
-    qtest_add_func("/fdc/media_insert", test_media_insert);
-    qtest_add_func("/fdc/read_no_dma_1", test_read_no_dma_1);
-    qtest_add_func("/fdc/read_no_dma_18", test_read_no_dma_18);
-    qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19);
-    qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
+    for (i = 0; i < ARRAY_SIZE(fdc_tests); i++) {
+        qtest_add_data_func(fdc_tests[i].path, &fdc_tests[i], run_isolated);
+    }
+
+    /* These use a QTestState of their own instead of global_qtest */
     qtest_add_func("/fdc/fuzz/cve_2021_20196", test_cve_2021_20196);
     qtest_add_func("/fdc/fuzz/cve_2021_3507", test_cve_2021_3507);
 
     ret = g_test_run();
 
     /* Cleanup */
-    qtest_end();
     unlink(test_image);
     g_free(test_image);
 
-- 
2.53.0

[PATCH 2/3] tests/qtest/fdc-test: skip test cases when the 'pc' machine is missing

[Christian Quante]

run_isolated() starts every test case in fdc_tests[] with "-machine pc".
A QEMU built without the i440fx/pc machine fails them with "unsupported
machine type 'pc'" rather than skipping them.

Check for the machine in run_isolated() and skip there.  The previous
patch routed every table entry through that one function, so one check
covers all of them; 4e37f749dd ("tests/qtest/drive_del-test: skip pc
tests when 'pc' machine is unavailable") needed the same three lines
four times over.

The two CVE test cases are left alone.  They start QEMU without
-machine and so ask for the default machine, which is a separate
question from the one this patch answers.

Signed-off-by: Christian Quante <christian@quante.one>
---
 tests/qtest/fdc-test.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index fd38475bf7..f480f4ba09 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -690,6 +690,11 @@ static void run_isolated(const void *data)
 {
     const FDCTest *test = data;
 
+    if (!qtest_has_machine("pc")) {
+        g_test_skip("Machine 'pc' is not available");
+        return;
+    }
+
     qtest_start("-machine pc -device floppy,id=floppy0");
     qtest_irq_intercept_in(global_qtest, "ioapic");
 
-- 
2.53.0

[PATCH 3/3] tests/qtest/fdc-test: check that READ ID answers for the drive it names

[Christian Quante]

Commit d00567f7fb ("hw/block/fdc: select the drive named by the READ ID
command") added the missing SET_CUR_DRV() to fdctrl_handle_readid(), but
nothing keeps it there: the suite runs a machine with a single floppy
drive, where answering for the wrong drive cannot be told from answering
for the right one.  Removing the call again leaves every existing test
case green.

Add a test case on a machine with two drives.  Drive 0 holds a medium and
is selected by a SEEK, the way a driver spinning up the motor would select
it; drive 1 is empty.  READ ID is then issued for drive 1, and two
independent properties of the answer have to hold: fdctrl_stop_transfer()
reports the drive in ST0, and an empty drive terminates abnormally with
ST1.MA.  Without the fix the controller answers for drive 0, which
terminates normally with ST0 = 0x00.

Test cases carry their own command line, so that a test can ask for
something other than the default single drive.  Both command lines name
the pc machine, so the check added by the previous patch still covers
every test case.

Signed-off-by: Christian Quante <christian@quante.one>
---
 tests/qtest/fdc-test.c | 81 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c
index f480f4ba09..33d1b83aeb 100644
--- a/tests/qtest/fdc-test.c
+++ b/tests/qtest/fdc-test.c
@@ -67,6 +67,7 @@ enum {
 enum {
     ST0_IC_MASK  = 0xc0,    /* interrupt code */
     ST0_IC_ABNTERM = 0x40,  /* abnormal termination */
+    ST0_DS_MASK  = 0x03,    /* drive the answer is about */
 
     ST1_MA       = 0x01,    /* missing address mark */
 };
@@ -545,6 +546,47 @@ static void test_read_id_no_media(void)
     g_assert_cmpint(st1 & ST1_MA, ==, ST1_MA);
 }
 
+/*
+ * READ ID carries the drive number in its command byte and has to latch it.
+ * A controller that fails to do so answers for whichever drive was selected
+ * last.  Here drive 0 holds a medium and is selected by the SEEK; drive 1 is
+ * empty, so answering for the wrong drive terminates normally (ST0 = 0x00)
+ * where the command has to fail, and names drive 0 rather than drive 1.
+ */
+static void test_read_id_other_drive(void)
+{
+    uint8_t drive = 1;
+    uint8_t head = 0;
+    uint8_t st0, st1;
+
+    media_insert();
+
+    /* SEEK latches drive 0, the way a driver spinning up the motor would */
+    send_seek(0);
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    st1 = floppy_recv();
+    floppy_recv();                  /* ST2 */
+    floppy_recv();                  /* cylinder */
+    floppy_recv();                  /* head */
+    floppy_recv();                  /* sector */
+    g_assert(get_irq(FLOPPY_IRQ));
+    floppy_recv();                  /* sector size */
+    g_assert(!get_irq(FLOPPY_IRQ));
+
+    g_assert_cmpint(st0 & ST0_DS_MASK, ==, drive);
+    g_assert_cmpint(st0 & ST0_IC_MASK, ==, ST0_IC_ABNTERM);
+    g_assert_cmpint(st1 & ST1_MA, ==, ST1_MA);
+}
+
 static void test_read_no_dma_1(void)
 {
     uint8_t ret;
@@ -656,6 +698,11 @@ static void test_cve_2021_3507(void)
     qtest_quit(s);
 }
 
+#define MACHINE_ONE_DRIVE "-machine pc -device floppy,id=floppy0"
+#define MACHINE_TWO_DRIVES \
+    "-machine pc -device floppy,id=floppy0,unit=0" \
+    " -device floppy,id=floppy1,unit=1"
+
 /*
  * Each test gets its own QEMU instance, so that none of them depends on the
  * state another one left behind.  Without this, several tests only pass in
@@ -667,35 +714,39 @@ static void test_cve_2021_3507(void)
 typedef struct {
     const char *path;
     void (*fn)(void);
+    const char *args;
 } FDCTest;
 
 static const FDCTest fdc_tests[] = {
-    { "/fdc/cmos",              test_cmos },
-    { "/fdc/no_media_on_start", test_no_media_on_start },
-    { "/fdc/read_without_media", test_read_without_media },
-    { "/fdc/media_change",      test_media_change },
-    { "/fdc/sense_interrupt",   test_sense_interrupt },
-    { "/fdc/relative_seek",     test_relative_seek },
-    { "/fdc/read_id",           test_read_id },
-    { "/fdc/read_id_no_media",  test_read_id_no_media },
-    { "/fdc/verify",            test_verify },
-    { "/fdc/media_insert",      test_media_insert },
-    { "/fdc/read_no_dma_1",     test_read_no_dma_1 },
-    { "/fdc/read_no_dma_18",    test_read_no_dma_18 },
-    { "/fdc/read_no_dma_19",    test_read_no_dma_19 },
-    { "/fdc/fuzz-registers",    fuzz_registers },
+    { "/fdc/cmos",              test_cmos,              MACHINE_ONE_DRIVE },
+    { "/fdc/no_media_on_start", test_no_media_on_start, MACHINE_ONE_DRIVE },
+    { "/fdc/read_without_media", test_read_without_media, MACHINE_ONE_DRIVE },
+    { "/fdc/media_change",      test_media_change,      MACHINE_ONE_DRIVE },
+    { "/fdc/sense_interrupt",   test_sense_interrupt,   MACHINE_ONE_DRIVE },
+    { "/fdc/relative_seek",     test_relative_seek,     MACHINE_ONE_DRIVE },
+    { "/fdc/read_id",           test_read_id,           MACHINE_ONE_DRIVE },
+    { "/fdc/read_id_no_media",  test_read_id_no_media,  MACHINE_ONE_DRIVE },
+    { "/fdc/read_id_other_drive", test_read_id_other_drive,
+                                                        MACHINE_TWO_DRIVES },
+    { "/fdc/verify",            test_verify,            MACHINE_ONE_DRIVE },
+    { "/fdc/media_insert",      test_media_insert,      MACHINE_ONE_DRIVE },
+    { "/fdc/read_no_dma_1",     test_read_no_dma_1,     MACHINE_ONE_DRIVE },
+    { "/fdc/read_no_dma_18",    test_read_no_dma_18,    MACHINE_ONE_DRIVE },
+    { "/fdc/read_no_dma_19",    test_read_no_dma_19,    MACHINE_ONE_DRIVE },
+    { "/fdc/fuzz-registers",    fuzz_registers,         MACHINE_ONE_DRIVE },
 };
 
 static void run_isolated(const void *data)
 {
     const FDCTest *test = data;
 
+    /* Every command line in fdc_tests[] names the pc machine */
     if (!qtest_has_machine("pc")) {
         g_test_skip("Machine 'pc' is not available");
         return;
     }
 
-    qtest_start("-machine pc -device floppy,id=floppy0");
+    qtest_start(test->args);
     qtest_irq_intercept_in(global_qtest, "ioapic");
 
     test->fn();
-- 
2.53.0

Re: [PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[Marc-André Lureau]

Hi

On Sat, Aug 22, 2026 at 2:45 PM Christian Quante <christian@quante.one> wrote:
>
> The test cases in fdc-test share one QEMU instance and, with it, the
> state of the floppy controller.  Several of them only pass in the order
> they happen to be registered in: read_no_dma_1, _18 and _19 fail when
> run on their own, and fuzz-registers leaves an unacknowledged interrupt
> behind, so it works only where it sits today.
>
> This takes up Kevin Wolf's invitation from the review of d00567f7fb and
> 431f59a34d: "If you feel like addressing the wider ordering problem in
> the tests, too, you're welcome to do so."
>
> Patch 1 registers the test cases from a table and runs each one in a
> QEMU instance of its own.  No test function changes what it tests; the
> three read_no_dma cases insert their own medium instead of inheriting
> one, and test_read_id stops ejecting at the end.
>
> Patch 2 skips the test cases when the pc machine is not part of the
> build, after 4e37f749dd, 6500a6abe6 and bb98963d6d did the same for
> drive_del-test, device-plug-test and pxe-test.  Patch 1 routes every
> test case through a single function, so one check covers all of them.
>
> Patch 3 adds a test case that keeps d00567f7fb ("hw/block/fdc: select
> the drive named by the READ ID command") from being lost again -- with
> one drive, answering for the wrong one cannot be told from answering
> for the right one.  It needs a machine with two drives, which is what
> the table's args field is for.
>
> Measured on this series:
>
>   * every test case passes on its own; on master three of them fail
>   * the suite passes with the registration order reversed; on master it
>     aborts in send_seek()
>   * removing SET_CUR_DRV() from fdctrl_handle_readid() again turns
>     exactly one test case red -- the new one -- and leaves the other
>     sixteen green
>   * each of the three commits builds and passes on its own
>
> The price is wall-clock time: the fifteen test cases in the table now
> start an instance each instead of sharing one, which takes the suite
> from 0.78s to 2.8s.  The qtest timeout is 60s.
>
> The pattern is not new here.  Of the 195 files in tests/qtest, 57
> register their test cases with qtest_add_data_func() and none uses glib
> fixtures; boot-serial-test.c is the closest model, with a table of test
> definitions and a shared runner that starts one instance per entry.  72
> files still include libqtest-single.h, which is why the test functions
> keep reaching their instance through global_qtest instead of carrying a
> QTestState around -- that would have touched every one of them.
>
> Christian Quante (3):
>   tests/qtest/fdc-test: give each test case its own QEMU instance
>   tests/qtest/fdc-test: skip test cases when the 'pc' machine is missing
>   tests/qtest/fdc-test: check that READ ID answers for the drive it
>     names
>

Series lgtm, however it takes ~10s more now.
Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>


I guess I should submit my old patch
https://patchew.org/QEMU/20260512065633.3542562-1-marcandre.lureau@redhat.com/

and perhaps add fdc to the slow_qtests dictionary..

Re: [PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[Christian Quante]

Hi Marc-André,

thanks for testing it.

> Series lgtm, however it takes ~10s more now.

I can't reproduce that here: fdc-test goes from 0.8s to 2.6s, so
+1.8s. With an --enable-asan build it is 3.4s, still nowhere near
+10s. The series takes the test from 2 QEMU instances to 16, at about
128ms each.

What are you measuring, and on what kind of build -- fdc-test on its
own?

Christian

Re: [PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[Fabiano Rosas]

Christian Quante <christian@quante.one> writes:

> Hi Marc-André,
>
> thanks for testing it.
>
>> Series lgtm, however it takes ~10s more now.
>
> I can't reproduce that here: fdc-test goes from 0.8s to 2.6s, so
> +1.8s. With an --enable-asan build it is 3.4s, still nowhere near
> +10s. The series takes the test from 2 QEMU instances to 16, at about
> 128ms each.
>
> What are you measuring, and on what kind of build -- fdc-test on its
> own?
>
> Christian

The timing looks ok on my side as well. I'll queue this series for the
moment. @Marc-André, feel free to object.

Thanks!

Re: [PATCH 0/3] tests/qtest/fdc-test: give each test case its own instance

[Marc-André Lureau]

Hi

On Thu, Sep 3, 2026 at 5:56 PM Fabiano Rosas <farosas@suse.de> wrote:
>
> Christian Quante <christian@quante.one> writes:
>
> > Hi Marc-André,
> >
> > thanks for testing it.
> >
> >> Series lgtm, however it takes ~10s more now.
> >
> > I can't reproduce that here: fdc-test goes from 0.8s to 2.6s, so
> > +1.8s. With an --enable-asan build it is 3.4s, still nowhere near
> > +10s. The series takes the test from 2 QEMU instances to 16, at about
> > 128ms each.
> >
> > What are you measuring, and on what kind of build -- fdc-test on its
> > own?
> >
> > Christian
>
> The timing looks ok on my side as well. I'll queue this series for the
> moment. @Marc-André, feel free to object.
>

No problem, I couldn't immediately reproduce it. Yes, I was running
with ASAN, and another without it, and noticed those +10s. But I was
probably doing something heavy at the same time.