qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH] atapi: classify read_cd as conditi


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
Date: Mon, 31 Oct 2016 11:10:49 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0



On 10/30/2016 01:49 PM, Hervé Poussineau wrote:
Le 29/10/2016 à 00:32, John Snow a écrit :
For the purposes of byte_count_limit verification, add a new flag that
identifies read_cd as sometimes returning data, then check the BCL in
its command handler after we know that it will indeed return data.

Reported-by: Hervé Poussineau <address@hidden>
Signed-off-by: John Snow <address@hidden>

Thanks for taking care of it.
However, the patch doesn't fix my initial problem (NT4 boot is very long
when a cdrom is inserted).

The hunk adding CONDDATA to read_cd command is missing. With following
hunk added, patch works as expected, and fixes my problem.

--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1321,7 +1321,7 @@ static const struct AtapiCmd {
     [ 0xad ] = { cmd_read_dvd_structure,            CHECK_READY },
     [ 0xbb ] = { cmd_set_speed,                     NONDATA },
     [ 0xbd ] = { cmd_mechanism_status,              0 },
-    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY },
+    [ 0xbe ] = { cmd_read_cd,                       CHECK_READY |
CONDDATA },
     /* [1] handler detects and reports not ready condition itself */
 };

If you add this hunk, you can add
Tested-by: Hervé Poussineau <address@hidden>


Whoops, got ahead of myself. Thanks for the save.

Regards,

Hervé

---
 hw/ide/atapi.c | 49 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 6189675..f26e3f4 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -637,6 +637,23 @@ static unsigned int event_status_media(IDEState *s,
     return 8; /* We wrote to 4 extra bytes from the header */
 }

+/*
+ * Before transferring data or otherwise signalling acceptance of a
command
+ * marked CONDDATA, we must check the validity of the byte_count_limit.
+ */
+static bool validate_bcl(IDEState *s)
+{
+    /* TODO: Check IDENTIFY data word 125 for defacult BCL (currently
0) */
+    if (s->atapi_dma || atapi_byte_count_limit(s)) {
+        return true;
+    }
+
+    /* TODO: Move abort back into core.c and introduce proper error
flow between
+     *       ATAPI layer and IDE core layer */
+    ide_abort_command(s);
+    return false;
+}
+
 static void cmd_get_event_status_notification(IDEState *s,
                                               uint8_t *buf)
 {
@@ -1028,12 +1045,19 @@ static void cmd_read_cd(IDEState *s, uint8_t*
buf)
         return;
     }

-    transfer_request = buf[9];
-    switch(transfer_request & 0xf8) {
-    case 0x00:
+    transfer_request = buf[9] & 0xf8;
+    if (transfer_request == 0x00) {
         /* nothing */
         ide_atapi_cmd_ok(s);
-        break;
+        return;
+    }
+
+    /* Check validity of BCL before transferring data */
+    if (!validate_bcl(s)) {
+        return;
+    }
+
+    switch(transfer_request) {
     case 0x10:
         /* normal read */
         ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
@@ -1266,6 +1290,14 @@ enum {
      * See ATA8-ACS3 "7.21.5 Byte Count Limit"
      */
     NONDATA = 0x04,
+
+    /*
+     * CONDDATA implies a command that transfers data only
conditionally based
+     * on the presence of suboptions. It should be exempt from the
BCL check at
+     * command validation time, but it needs to be checked at the
command
+     * handler level instead.
+     */
+    CONDDATA = 0x08,
 };

 static const struct AtapiCmd {
@@ -1348,15 +1380,12 @@ void ide_atapi_cmd(IDEState *s)
         return;
     }

-    /* Nondata commands permit the byte_count_limit to be 0.
+    /* Commands that don't transfer DATA permit the byte_count_limit
to be 0.
      * If this is a data-transferring PIO command and BCL is 0,
      * we abort at the /ATA/ level, not the ATAPI level.
      * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
-    if (cmd->handler && !(cmd->flags & NONDATA)) {
-        /* TODO: Check IDENTIFY data word 125 for default BCL
(currently 0) */
-        if (!(atapi_byte_count_limit(s) || s->atapi_dma)) {
-            /* TODO: Move abort back into core.c and make static
inline again */
-            ide_abort_command(s);
+    if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) {
+        if (!validate_bcl(s)) {
             return;
         }
     }




--
—js



reply via email to

[Prev in Thread] Current Thread [Next in Thread]