Pull request

-----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCAAdFiEE+ber27ys35W+dsvQfe+BBqr8OQ4FAl4viM8ACgkQfe+BBqr8
 OQ6o6Q//ZoGN0sdAoaaeGDpSU6Rt2wJnKjUs3rtyHVObDO1CGQxBkH5wLNWHnx8K
 2He2N5jaq3GnoWMMbm8yR3GEtD1X2B3WH7tA4RwjAdHXpPKWBgkm5cOfDdvmj2KY
 IPBnPGVhEQ/yUbex0xqY/3AEgVqrjT8c2sC0RcwxeSkRNGZ7U3EpfE5br46Ih7d0
 ka0pqDvESBuMjYHmIf3B5R+8MJKZL5qxu0wgLbIKrUcfPhO9L9yWeH/rO8PEj85K
 2B4unxtLS0CjjgLuJ/ZwV1wjg/mV0tAfMtV2XLEqWiPpO72d4ZMnzE7D5FCzcvjK
 X2FgpnTAWyZNJP0ZExJOG2lDP6HWERWyhPogn2yJ3VFII0QaoW+eyfQSu8eZkpIC
 dFTQlcw9Beoxjtnbu+X7pbhp11lefpWjvKB94XiIcGuB8ennTI1yV1MmpITF8kYx
 0nhbS5+QvhQfsl8mE7k5RvEqna37b06A51r73YP0SYS/L2IM91JLrfHblNM/v/89
 CPEkk9SFtoFSBUMm3ZyahR+d7bYwLKXg5mbuZuYYzLJJs8LAgVV5/sCzgdmV0ziN
 bu7BAAne/0m44oSeIPPMUdAWHpkshwGLs4dQgETwsFsCVZ4jzM0oX1zqpjvYYJdb
 LW7/+0Nlne879h2M3aFSaBPDHn7ilPiSrh5Poxv6saailFRe/tU=
 =h3OK
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/jnsnow/tags/ide-pull-request' into staging

Pull request

# gpg: Signature made Tue 28 Jan 2020 01:05:19 GMT
# gpg:                using RSA key F9B7ABDBBCACDF95BE76CBD07DEF8106AAFC390E
# gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>" [full]
# Primary key fingerprint: FAEB 9711 A12C F475 812F  18F2 88A9 064D 1835 61EB
#      Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76  CBD0 7DEF 8106 AAFC 390E

* remotes/jnsnow/tags/ide-pull-request:
  tests/ide-test: Create a single unit-test covering more PRDT cases
  ide: Fix incorrect handling of some PRDTs in ide_dma_cb()

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

hw/ide/core.c

@@ -849,6 +849,7 @@ static void ide_dma_cb(void *opaque, int ret)
     int64_t sector_num;
     uint64_t offset;
     bool stay_active = false;
+    int32_t prep_size = 0;
 
     if (ret == -EINVAL) {
         ide_dma_error(s);
@@ -863,13 +864,15 @@ static void ide_dma_cb(void *opaque, int ret)
         }
     }
 
-    n = s->io_buffer_size >> 9;
+    if (s->io_buffer_size > s->nsector * 512) {
-    if (n > s->nsector) {
+        /*
-        /* The PRDs were longer than needed for this request. Shorten them so
+         * The PRDs were longer than needed for this request.
-         * we don't get a negative remainder. The Active bit must remain set
+         * The Active bit must remain set after the request completes.
-         * after the request completes. */
+         */
         n = s->nsector;
         stay_active = true;
+    } else {
+        n = s->io_buffer_size >> 9;
     }
 
     sector_num = ide_get_sector(s);
@@ -892,9 +895,20 @@ static void ide_dma_cb(void *opaque, int ret)
     n = s->nsector;
     s->io_buffer_index = 0;
     s->io_buffer_size = n * 512;
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) {
+    prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size);
-        /* The PRDs were too short. Reset the Active bit, but don't raise an
+    /* prepare_buf() must succeed and respect the limit */
-         * interrupt. */
+    assert(prep_size >= 0 && prep_size <= n * 512);
+
+    /*
+     * Now prep_size stores the number of bytes in the sglist, and
+     * s->io_buffer_size stores the number of bytes described by the PRDs.
+     */
+
+    if (prep_size < n * 512) {
+        /*
+         * The PRDs are too short for this request. Error condition!
+         * Reset the Active bit and don't raise the interrupt.
+         */
         s->status = READY_STAT | SEEK_STAT;
         dma_buf_commit(s, 0);
         goto eot;

tests/qtest/ide-test.c

@@ -445,104 +445,81 @@ static void test_bmdma_trim(void)
     test_bmdma_teardown(qts);
 }
 
-static void test_bmdma_short_prdt(void)
+/*
+ * This test is developed according to the Programming Interface for
+ * Bus Master IDE Controller (Revision 1.0 5/16/94)
+ */
+static void test_bmdma_various_prdts(void)
 {
-    QTestState *qts;
+    int sectors = 0;
-    QPCIDevice *dev;
+    uint32_t size = 0;
-    QPCIBar bmdma_bar, ide_bar;
-    uint8_t status;
 
-    PrdtEntry prdt[] = {
+    for (sectors = 1; sectors <= 256; sectors *= 2) {
-        {
+        QTestState *qts = NULL;
-            .addr = 0,
+        QPCIDevice *dev = NULL;
-            .size = cpu_to_le32(0x10 | PRDT_EOT),
+        QPCIBar bmdma_bar, ide_bar;
-        },
-    };
 
-    qts = test_bmdma_setup();
+        qts = test_bmdma_setup();
+        dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
 
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
+        for (size = 0; size < 65536; size += 256) {
+            uint32_t req_size = sectors * 512;
+            uint32_t prd_size = size & 0xfffe; /* bit 0 is always set to 0 */
+            uint8_t ret = 0;
+            uint8_t req_status = 0;
+            uint8_t abort_req_status = 0;
+            PrdtEntry prdt[] = {
+                {
+                    .addr = 0,
+                    .size = cpu_to_le32(size | PRDT_EOT),
+                },
+            };
 
-    /* Normal request */
+            /* A value of zero in PRD size indicates 64K */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
+            if (prd_size == 0) {
-                              prdt, ARRAY_SIZE(prdt), NULL);
+                prd_size = 65536;
-    g_assert_cmphex(status, ==, 0);
+            }
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
 
-    /* Abort the request before it completes */
+            /*
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
+             * 1. If PRDs specified a smaller size than the IDE transfer
-                              prdt, ARRAY_SIZE(prdt), NULL);
+             * size, then the Interrupt and Active bits in the Controller
-    g_assert_cmphex(status, ==, 0);
+             * status register are not set (Error Condition).
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
+             *
-    free_pci_device(dev);
+             * 2. If the size of the physical memory regions was equal to
-    test_bmdma_teardown(qts);
+             * the IDE device transfer size, the Interrupt bit in the
-}
+             * Controller status register is set to 1, Active bit is set to 0.
+             *
+             * 3. If PRDs specified a larger size than the IDE transfer size,
+             * the Interrupt and Active bits in the Controller status register
+             * are both set to 1.
+             */
+            if (prd_size < req_size) {
+                req_status = 0;
+                abort_req_status = 0;
+            } else if (prd_size == req_size) {
+                req_status = BM_STS_INTR;
+                abort_req_status = BM_STS_INTR;
+            } else {
+                req_status = BM_STS_ACTIVE | BM_STS_INTR;
+                abort_req_status = BM_STS_INTR;
+            }
 
-static void test_bmdma_one_sector_short_prdt(void)
+            /* Test the request */
-{
+            ret = send_dma_request(qts, CMD_READ_DMA, 0, sectors,
-    QTestState *qts;
+                                   prdt, ARRAY_SIZE(prdt), NULL);
-    QPCIDevice *dev;
+            g_assert_cmphex(ret, ==, req_status);
-    QPCIBar bmdma_bar, ide_bar;
+            assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    uint8_t status;
 
-    /* Read 2 sectors but only give 1 sector in PRDT */
+            /* Now test aborting the same request */
-    PrdtEntry prdt[] = {
+            ret = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0,
-        {
+                                   sectors, prdt, ARRAY_SIZE(prdt), NULL);
-            .addr = 0,
+            g_assert_cmphex(ret, ==, abort_req_status);
-            .size = cpu_to_le32(0x200 | PRDT_EOT),
+            assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-        },
+        }
-    };
 
-    qts = test_bmdma_setup();
+        free_pci_device(dev);
-
+        test_bmdma_teardown(qts);
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
+    }
-
-    /* Normal request */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 2,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-    /* Abort the request before it completes */
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 2,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    free_pci_device(dev);
-    test_bmdma_teardown(qts);
-}
-
-static void test_bmdma_long_prdt(void)
-{
-    QTestState *qts;
-    QPCIDevice *dev;
-    QPCIBar bmdma_bar, ide_bar;
-    uint8_t status;
-
-    PrdtEntry prdt[] = {
-        {
-            .addr = 0,
-            .size = cpu_to_le32(0x1000 | PRDT_EOT),
-        },
-    };
-
-    qts = test_bmdma_setup();
-
-    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
-
-    /* Normal request */
-    status = send_dma_request(qts, CMD_READ_DMA, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-
-    /* Abort the request before it completes */
-    status = send_dma_request(qts, CMD_READ_DMA | CMDF_ABORT, 0, 1,
-                              prdt, ARRAY_SIZE(prdt), NULL);
-    g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR);
-    free_pci_device(dev);
-    test_bmdma_teardown(qts);
 }
 
 static void test_bmdma_no_busmaster(void)
@@ -1066,10 +1043,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/bmdma/simple_rw", test_bmdma_simple_rw);
     qtest_add_func("/ide/bmdma/trim", test_bmdma_trim);
-    qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
+    qtest_add_func("/ide/bmdma/various_prdts", test_bmdma_various_prdts);
-    qtest_add_func("/ide/bmdma/one_sector_short_prdt",
-                   test_bmdma_one_sector_short_prdt);
-    qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
     qtest_add_func("/ide/bmdma/no_busmaster", test_bmdma_no_busmaster);
 
     qtest_add_func("/ide/flush", test_flush);