All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access
@ 2022-07-07  3:11 Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 2/7] tests/qtest/ide-test.c: Create disk image for use as a secondary Lev Kujawski
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

Eliminate the remaining TODOs in hw/ide/piix.c by:
* Using pci_set_{size} functions to write the PIIX PCI configuration
  space instead of manipulating it directly as an array; and
* Documenting the default register values by reference to the
  controlling specification.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/piix.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9a9b28078e..de1f4f0efb 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -21,6 +21,10 @@
  * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
+ *
+ * References:
+ *  [1] 82371FB (PIIX) AND 82371SB (PIIX3) PCI ISA IDE XCELERATOR,
+ *      290550-002, Intel Corporation, April 1997.
  */
 
 #include "qemu/osdep.h"
@@ -114,14 +118,11 @@ static void piix_ide_reset(DeviceState *dev)
         ide_bus_reset(&d->bus[i]);
     }
 
-    /* TODO: this is the default. do not override. */
-    pci_conf[PCI_COMMAND] = 0x00;
-    /* TODO: this is the default. do not override. */
-    pci_conf[PCI_COMMAND + 1] = 0x00;
-    /* TODO: use pci_set_word */
-    pci_conf[PCI_STATUS] = PCI_STATUS_FAST_BACK;
-    pci_conf[PCI_STATUS + 1] = PCI_STATUS_DEVSEL_MEDIUM >> 8;
-    pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
+    /* PCI command register default value (0000h) per [1, p.48].  */
+    pci_set_word(pci_conf + PCI_COMMAND, 0x0000);
+    pci_set_word(pci_conf + PCI_STATUS,
+                 PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
+    pci_set_byte(pci_conf + 0x20, 0x01);  /* BMIBA: 20-23h */
 }
 
 static int pci_piix_init_ports(PCIIDEState *d)
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/7] tests/qtest/ide-test.c: Create disk image for use as a secondary
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 3/7] hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE DIAGNOSTIC Lev Kujawski
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

Change 'tmp_path' into an array of two members to accommodate another
disk image of size TEST_IMAGE_SIZE.  This facilitates testing ATA
protocol aspects peculiar to secondary devices on the same controller.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 tests/qtest/ide-test.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 5bcb75a7e5..1ff707d2cd 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -121,7 +121,9 @@ enum {
 static QPCIBus *pcibus = NULL;
 static QGuestAllocator guest_malloc;
 
-static char tmp_path[] = "/tmp/qtest.XXXXXX";
+#define TMP_PATH_LEN 18
+
+static char tmp_path[2][TMP_PATH_LEN];
 static char debug_path[] = "/tmp/qtest-blkdebug.XXXXXX";
 
 static QTestState *ide_test_start(const char *cmdline_fmt, ...)
@@ -310,7 +312,7 @@ static QTestState *test_bmdma_setup(void)
     qts = ide_test_start(
         "-drive file=%s,if=ide,cache=writeback,format=raw "
         "-global ide-hd.serial=%s -global ide-hd.ver=%s",
-        tmp_path, "testdisk", "version");
+        tmp_path[0], "testdisk", "version");
     qtest_irq_intercept_in(qts, "ioapic");
 
     return qts;
@@ -574,7 +576,7 @@ static void test_identify(void)
     qts = ide_test_start(
         "-drive file=%s,if=ide,cache=writeback,format=raw "
         "-global ide-hd.serial=%s -global ide-hd.ver=%s",
-        tmp_path, "testdisk", "version");
+        tmp_path[0], "testdisk", "version");
 
     dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
 
@@ -662,7 +664,7 @@ static void test_flush(void)
 
     qts = ide_test_start(
         "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
-        tmp_path);
+        tmp_path[0]);
 
     dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
 
@@ -713,7 +715,7 @@ static void test_pci_retry_flush(void)
     qts = ide_test_start(
         "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,format=raw,"
         "rerror=stop,werror=stop",
-        debug_path, tmp_path);
+        debug_path, tmp_path[0]);
 
     dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
 
@@ -892,14 +894,14 @@ static void cdrom_pio_impl(int nblocks)
 
     /* Prepopulate the CDROM with an interesting pattern */
     generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
-    fh = fopen(tmp_path, "w+");
+    fh = fopen(tmp_path[0], "w+");
     ret = fwrite(pattern, ATAPI_BLOCK_SIZE, patt_blocks, fh);
     g_assert_cmpint(ret, ==, patt_blocks);
     fclose(fh);
 
     qts = ide_test_start(
             "-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
-            "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
+            "-device ide-cd,drive=sr0,bus=ide.0", tmp_path[0]);
     dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
     qtest_irq_intercept_in(qts, "ioapic");
 
@@ -985,7 +987,7 @@ static void test_cdrom_dma(void)
 
     qts = ide_test_start(
             "-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
-            "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
+            "-device ide-cd,drive=sr0,bus=ide.0", tmp_path[0]);
     qtest_irq_intercept_in(qts, "ioapic");
 
     guest_buf = guest_alloc(&guest_malloc, len);
@@ -993,7 +995,7 @@ static void test_cdrom_dma(void)
     prdt[0].size = cpu_to_le32(len | PRDT_EOT);
 
     generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE);
-    fh = fopen(tmp_path, "w+");
+    fh = fopen(tmp_path[0], "w+");
     ret = fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh);
     g_assert_cmpint(ret, ==, 16);
     fclose(fh);
@@ -1011,6 +1013,7 @@ static void test_cdrom_dma(void)
 
 int main(int argc, char **argv)
 {
+    int i;
     int fd;
     int ret;
 
@@ -1020,11 +1023,14 @@ int main(int argc, char **argv)
     close(fd);
 
     /* Create a temporary raw image */
-    fd = mkstemp(tmp_path);
-    g_assert(fd >= 0);
-    ret = ftruncate(fd, TEST_IMAGE_SIZE);
-    g_assert(ret == 0);
-    close(fd);
+    for (i = 0; i < 2; ++i) {
+        strncpy(tmp_path[i], "/tmp/qtest.XXXXXX", TMP_PATH_LEN);
+        fd = mkstemp(tmp_path[i]);
+        g_assert(fd >= 0);
+        ret = ftruncate(fd, TEST_IMAGE_SIZE);
+        g_assert(ret == 0);
+        close(fd);
+    }
 
     /* Run the tests */
     g_test_init(&argc, &argv, NULL);
@@ -1048,7 +1054,9 @@ int main(int argc, char **argv)
     ret = g_test_run();
 
     /* Cleanup */
-    unlink(tmp_path);
+    for (i = 0; i < 2; ++i) {
+        unlink(tmp_path[i]);
+    }
     unlink(debug_path);
 
     return ret;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/7] hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE DIAGNOSTIC
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 2/7] tests/qtest/ide-test.c: Create disk image for use as a secondary Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 4/7] tests/qtest/ide-test: Verify that DIAGNOSTIC clears DEV to zero Lev Kujawski
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

Prior to this patch, cmd_exec_dev_diagnostic relied upon
ide_set_signature to clear the device register.  While the
preservation of the drive bit by ide_set_signature is necessary for
the DEVICE RESET, IDENTIFY DEVICE, and READ SECTOR commands,
ATA/ATAPI-6 specifies that "DEV shall be cleared to zero" for EXECUTE
DEVICE DIAGNOSTIC.

This deviation was uncovered by the ATACT Device Testing Program
written by Hale Landis.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7cbc0a54a7..b747191ebf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1704,8 +1704,14 @@ static bool cmd_identify_packet(IDEState *s, uint8_t cmd)
     return false;
 }
 
+/* EXECUTE DEVICE DIAGNOSTIC */
 static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd)
 {
+    /*
+     * Clear the device register per the ATA (v6) specification,
+     * because ide_set_signature does not clear LBA or drive bits.
+     */
+    s->select = (ATA_DEV_ALWAYS_ON);
     ide_set_signature(s);
 
     if (s->drive_kind == IDE_CD) {
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/7] tests/qtest/ide-test: Verify that DIAGNOSTIC clears DEV to zero
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 2/7] tests/qtest/ide-test.c: Create disk image for use as a secondary Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 3/7] hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE DIAGNOSTIC Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 5/7] qpci_device_enable: Allow for command bits hardwired to 0 Lev Kujawski
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

Verify correction of EXECUTE DEVICE DIAGNOSTIC introduced in commit
72423831c3 (hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE
DIAGNOSTIC, 2022-05-28).

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 tests/qtest/ide-test.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index 1ff707d2cd..dfcf59cee8 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -90,6 +90,7 @@ enum {
 
 enum {
     CMD_DSM         = 0x06,
+    CMD_DIAGNOSE    = 0x90,
     CMD_READ_DMA    = 0xc8,
     CMD_WRITE_DMA   = 0xca,
     CMD_FLUSH_CACHE = 0xe7,
@@ -616,6 +617,36 @@ static void test_identify(void)
     free_pci_device(dev);
 }
 
+static void test_diagnostic(void)
+{
+    QTestState *qts;
+    QPCIDevice *dev;
+    QPCIBar bmdma_bar, ide_bar;
+    uint8_t data;
+
+    qts = ide_test_start(
+        "-blockdev driver=file,node-name=hda,filename=%s "
+        "-blockdev driver=file,node-name=hdb,filename=%s "
+        "-device ide-hd,drive=hda,bus=ide.0,unit=0 "
+        "-device ide-hd,drive=hdb,bus=ide.0,unit=1 ",
+        tmp_path[0], tmp_path[1]);
+
+    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
+
+    /* DIAGNOSE command on device 1 */
+    qpci_io_writeb(dev, ide_bar, reg_device, DEV);
+    data = qpci_io_readb(dev, ide_bar, reg_device);
+    g_assert_cmphex(data & DEV, ==, DEV);
+    qpci_io_writeb(dev, ide_bar, reg_command, CMD_DIAGNOSE);
+
+    /* Verify that DEVICE is now 0 */
+    data = qpci_io_readb(dev, ide_bar, reg_device);
+    g_assert_cmphex(data & DEV, ==, 0);
+
+    ide_test_quit(qts);
+    free_pci_device(dev);
+}
+
 /*
  * Write sector 1 with random data to make IDE storage dirty
  * Needed for flush tests so that flushes actually go though the block layer
@@ -1037,6 +1068,8 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/identify", test_identify);
 
+    qtest_add_func("/ide/diagnostic", test_diagnostic);
+
     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/various_prdts", test_bmdma_various_prdts);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/7] qpci_device_enable: Allow for command bits hardwired to 0
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
                   ` (2 preceding siblings ...)
  2022-07-07  3:11 ` [PATCH v2 4/7] tests/qtest/ide-test: Verify that DIAGNOSTIC clears DEV to zero Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 6/7] hw/ide/piix: Ignore writes of hardwired PCI command register bits Lev Kujawski
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

Devices like the PIIX3/4 IDE controller do not support certain modes
of operation, such as memory space accesses, and indicate this lack of
support by hardwiring the applicable bits to zero.  Extend the QEMU
PCI device testing framework to accommodate such devices.

* tests/qtest/libqos/pci.h: Add the command_disabled word to indicate
  bits hardwired to 0.
* tests/qtest/libqos/pci.c: Verify that hardwired bits are actually
  hardwired.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 tests/qtest/libqos/pci.c | 13 +++++++------
 tests/qtest/libqos/pci.h |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/libqos/pci.c b/tests/qtest/libqos/pci.c
index b23d72346b..4f3d28d8d9 100644
--- a/tests/qtest/libqos/pci.c
+++ b/tests/qtest/libqos/pci.c
@@ -220,18 +220,19 @@ int qpci_secondary_buses_init(QPCIBus *bus)
 
 void qpci_device_enable(QPCIDevice *dev)
 {
-    uint16_t cmd;
+    const uint16_t enable_bits =
+        PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    uint16_t cmd, new_cmd;
 
     /* FIXME -- does this need to be a bus callout? */
     cmd = qpci_config_readw(dev, PCI_COMMAND);
-    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+    cmd |= enable_bits;
     qpci_config_writew(dev, PCI_COMMAND, cmd);
 
     /* Verify the bits are now set. */
-    cmd = qpci_config_readw(dev, PCI_COMMAND);
-    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
-    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
-    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
+    new_cmd = qpci_config_readw(dev, PCI_COMMAND);
+    new_cmd &= enable_bits;
+    g_assert_cmphex(new_cmd, ==, enable_bits & ~dev->command_disabled);
 }
 
 /**
diff --git a/tests/qtest/libqos/pci.h b/tests/qtest/libqos/pci.h
index 8389614523..eaedb98588 100644
--- a/tests/qtest/libqos/pci.h
+++ b/tests/qtest/libqos/pci.h
@@ -68,6 +68,7 @@ struct QPCIDevice
     bool msix_enabled;
     QPCIBar msix_table_bar, msix_pba_bar;
     uint64_t msix_table_off, msix_pba_off;
+    uint16_t command_disabled;
 };
 
 struct QPCIAddress {
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 6/7] hw/ide/piix: Ignore writes of hardwired PCI command register bits
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
                   ` (3 preceding siblings ...)
  2022-07-07  3:11 ` [PATCH v2 5/7] qpci_device_enable: Allow for command bits hardwired to 0 Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-07-07  3:11 ` [PATCH v2 7/7] hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command Lev Kujawski
  2022-09-30 15:17 ` [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

One method to enable PCI bus mastering for IDE controllers, often used
by x86 firmware, is to write 0x7 to the PCI command register.  Neither
the PIIX3 specification nor actual hardware (a Tyan S1686D system)
permit modification of the Memory Space Enable (MSE) bit, 1, and thus
the command register would be left in an unspecified state without
this patch.

* hw/ide/piix.c
  a) Add a reference to the PIIX4 data sheet.
  b) Mask the MSE bit using the QEMU PCI device wmask field.
* tests/qtest/ide-test.c
  Use the command_disabled field of the QPCIDevice testing model to
  indicate that PCI_COMMAND_MEMORY is hardwired in the PIIX3/4 IDE
  controller.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/piix.c          | 15 +++++++++++++++
 tests/qtest/ide-test.c |  1 +
 2 files changed, 16 insertions(+)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index de1f4f0efb..64620c5778 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -25,6 +25,8 @@
  * References:
  *  [1] 82371FB (PIIX) AND 82371SB (PIIX3) PCI ISA IDE XCELERATOR,
  *      290550-002, Intel Corporation, April 1997.
+ *  [2] 82371AB PCI-TO-ISA / IDE XCELERATOR (PIIX4), 290562-001,
+ *      Intel Corporation, April 1997.
  */
 
 #include "qemu/osdep.h"
@@ -160,6 +162,19 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     uint8_t *pci_conf = dev->config;
     int rc;
 
+    /*
+     * Mask all IDE PCI command register bits except for Bus Master
+     * Function Enable (bit 2) and I/O Space Enable (bit 1), as the
+     * remainder are hardwired to 0 [1, p.48] [2, p.89-90].
+     *
+     * NOTE: According to the PIIX3 datasheet [1], the Memory Space
+     * Enable (MSE bit) is hardwired to 1, but this is contradicted by
+     * actual PIIX3 hardware, the datasheet itself (viz., Default
+     * Value: 0000h), and the PIIX4 datasheet [2].
+     */
+    pci_set_word(dev->wmask + PCI_COMMAND,
+                 PCI_COMMAND_MASTER | PCI_COMMAND_IO);
+
     pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
 
     bmdma_setup_bar(d);
diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index dfcf59cee8..728e8ce00f 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -176,6 +176,7 @@ static QPCIDevice *get_pci_device(QTestState *qts, QPCIBar *bmdma_bar,
 
     *ide_bar = qpci_legacy_iomap(dev, IDE_BASE);
 
+    dev->command_disabled = PCI_COMMAND_MEMORY;
     qpci_device_enable(dev);
 
     return dev;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 7/7] hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
                   ` (4 preceding siblings ...)
  2022-07-07  3:11 ` [PATCH v2 6/7] hw/ide/piix: Ignore writes of hardwired PCI command register bits Lev Kujawski
@ 2022-07-07  3:11 ` Lev Kujawski
  2022-09-30 15:17 ` [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Lev Kujawski @ 2022-07-07  3:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, qemu-block, Laurent Vivier, Thomas Huth,
	John Snow, Lev Kujawski

CHS-based disk utilities and operating systems may adjust the logical
geometry of a hard drive to cope with the expectations or limitations
of software using the ATA INITIALIZE_DEVICE_PARAMETERS command.

Prior to this patch, INITIALIZE_DEVICE_PARAMETERS was a nop that
always returned success, raising the possibility of data loss or
corruption if the CHS<->LBA translation redirected a write to the
wrong sector.

* hw/ide/core.c
ide_reset():
  Reset the logical CHS geometry of the hard disk when the power-on
  defaults feature is enabled.
cmd_specify():
  a) New function implementing INITIALIZE_DEVICE_PARAMETERS.
  b) Ignore calls for empty or ATAPI devices.
cmd_set_features():
  Implement the power-on defaults enable and disable features.
struct ide_cmd_table:
  Switch WIN_SPECIFY from cmd_nop() to cmd_specify().
ide_init_drive():
  Set new fields 'drive_heads' and 'drive_sectors' based upon the
  actual disk geometry.

* include/hw/ide/internal.h
struct IDEState:
a) Store the actual drive CHS values within the new fields
   'drive_heads' and 'drive_sectors.'
b) Track whether a soft IDE reset should also reset the logical CHS
   geometry of the hard disk within the new field 'reset_reverts'.

Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>
---
 hw/ide/core.c             | 29 ++++++++++++++++++++++++++---
 include/hw/ide/internal.h |  3 +++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b747191ebf..39afdc0006 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1340,6 +1340,11 @@ static void ide_reset(IDEState *s)
         s->pio_aiocb = NULL;
     }
 
+    if (s->reset_reverts) {
+        s->reset_reverts = false;
+        s->heads         = s->drive_heads;
+        s->sectors       = s->drive_sectors;
+    }
     if (s->drive_kind == IDE_CFATA)
         s->mult_sectors = 0;
     else
@@ -1618,6 +1623,20 @@ static bool cmd_check_power_mode(IDEState *s, uint8_t cmd)
     return true;
 }
 
+/* INITIALIZE DEVICE PARAMETERS */
+static bool cmd_specify(IDEState *s, uint8_t cmd)
+{
+    if (s->blk && s->drive_kind != IDE_CD) {
+        s->heads = (s->select & (ATA_DEV_HS)) + 1;
+        s->sectors = s->nsector;
+        ide_set_irq(s->bus);
+    } else {
+        ide_abort_command(s);
+    }
+
+    return true;
+}
+
 static bool cmd_set_features(IDEState *s, uint8_t cmd)
 {
     uint16_t *identify_data;
@@ -1641,7 +1660,11 @@ static bool cmd_set_features(IDEState *s, uint8_t cmd)
         ide_flush_cache(s);
         return false;
     case 0xcc: /* reverting to power-on defaults enable */
+        s->reset_reverts = true;
+        return true;
     case 0x66: /* reverting to power-on defaults disable */
+        s->reset_reverts = false;
+        return true;
     case 0xaa: /* read look-ahead enable */
     case 0x55: /* read look-ahead disable */
     case 0x05: /* set advanced power management mode */
@@ -2051,7 +2074,7 @@ static const struct {
     [WIN_SEEK]                    = { cmd_seek, HD_CFA_OK | SET_DSC },
     [CFA_TRANSLATE_SECTOR]        = { cmd_cfa_translate_sector, CFA_OK },
     [WIN_DIAGNOSE]                = { cmd_exec_dev_diagnostic, ALL_OK },
-    [WIN_SPECIFY]                 = { cmd_nop, HD_CFA_OK | SET_DSC },
+    [WIN_SPECIFY]                 = { cmd_specify, HD_CFA_OK | SET_DSC },
     [WIN_STANDBYNOW2]             = { cmd_nop, HD_CFA_OK },
     [WIN_IDLEIMMEDIATE2]          = { cmd_nop, HD_CFA_OK },
     [WIN_STANDBY2]                = { cmd_nop, HD_CFA_OK },
@@ -2541,8 +2564,8 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
 
     blk_get_geometry(blk, &nb_sectors);
     s->cylinders = cylinders;
-    s->heads = heads;
-    s->sectors = secs;
+    s->heads = s->drive_heads = heads;
+    s->sectors = s->drive_sectors = secs;
     s->chs_trans = chs_trans;
     s->nb_sectors = nb_sectors;
     s->wwn = wwn;
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index 97e7e59dc5..b17f36df95 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -375,6 +375,7 @@ struct IDEState {
     uint8_t unit;
     /* ide config */
     IDEDriveKind drive_kind;
+    int drive_heads, drive_sectors;
     int cylinders, heads, sectors, chs_trans;
     int64_t nb_sectors;
     int mult_sectors;
@@ -401,6 +402,8 @@ struct IDEState {
     uint8_t select;
     uint8_t status;
 
+    bool reset_reverts;
+
     /* set for lba48 access */
     uint8_t lba48;
     BlockBackend *blk;
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access
  2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
                   ` (5 preceding siblings ...)
  2022-07-07  3:11 ` [PATCH v2 7/7] hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command Lev Kujawski
@ 2022-09-30 15:17 ` Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2022-09-30 15:17 UTC (permalink / raw)
  To: Lev Kujawski
  Cc: qemu-devel, Paolo Bonzini, qemu-block, Laurent Vivier,
	Thomas Huth, John Snow

Am 07.07.2022 um 05:11 hat Lev Kujawski geschrieben:
> Eliminate the remaining TODOs in hw/ide/piix.c by:
> * Using pci_set_{size} functions to write the PIIX PCI configuration
>   space instead of manipulating it directly as an array; and
> * Documenting the default register values by reference to the
>   controlling specification.
> 
> Signed-off-by: Lev Kujawski <lkujaw@member.fsf.org>

Thanks, dropped patches 5 and 6 because I see that you have posted a
newer version of them, and applied the rest to the block branch.

Kevin



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-30 15:21 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07  3:11 [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 2/7] tests/qtest/ide-test.c: Create disk image for use as a secondary Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 3/7] hw/ide/core: Clear LBA and drive bits for EXECUTE DEVICE DIAGNOSTIC Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 4/7] tests/qtest/ide-test: Verify that DIAGNOSTIC clears DEV to zero Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 5/7] qpci_device_enable: Allow for command bits hardwired to 0 Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 6/7] hw/ide/piix: Ignore writes of hardwired PCI command register bits Lev Kujawski
2022-07-07  3:11 ` [PATCH v2 7/7] hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command Lev Kujawski
2022-09-30 15:17 ` [PATCH v2 1/7] piix_ide_reset: Use pci_set_* functions instead of direct access Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.