[Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression

[Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression

[John Snow]

Marc noticed that a recent ATAPI permissions fix broke NetBSD 7.0's
installer ISO.

The problem is that it's meaningless to check for !(cmd->flags & nondata)
if the command isn't supported, since all unsupported commands have
_no_ flags. Effectively, all commands default to "Transfer Data" in our
model until we classify them otherwise.

This leads to a problem where we reject a zero byte BCL PIO command that
transfers no data, simply because we have no properties for the command
at all.

Getting an ATA rejection for this command greatly confuses NetBSD.

Correct behavior is to reject the command at the SCSI layer for being
unsupported.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch atapi-bclimit-netbsd
https://github.com/jnsnow/qemu/tree/atapi-bclimit-netbsd

This version is tagged atapi-bclimit-netbsd-v1:
https://github.com/jnsnow/qemu/releases/tag/atapi-bclimit-netbsd-v1

John Snow (2):
  atapi: add byte_count_limit helper
  atapi: Prioritize unknown cmd error over BCL error

 hw/ide/atapi.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] atapi: add byte_count_limit helper

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 747f466..1471ae2 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -167,6 +167,17 @@ void ide_atapi_io_error(IDEState *s, int ret)
     }
 }
 
+static uint16_t atapi_byte_count_limit(IDEState *s)
+{
+    uint16_t bcl;
+
+    bcl = s->lcyl | (s->hcyl << 8);
+    if (bcl == 0xffff) {
+        return 0xfffe;
+    }
+    return bcl;
+}
+
 /* The whole ATAPI transfer logic is handled in this function */
 void ide_atapi_cmd_reply_end(IDEState *s)
 {
@@ -209,12 +220,10 @@ void ide_atapi_cmd_reply_end(IDEState *s)
         } else {
             /* a new transfer is needed */
             s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO;
-            byte_count_limit = s->lcyl | (s->hcyl << 8);
+            byte_count_limit = atapi_byte_count_limit(s);
 #ifdef DEBUG_IDE_ATAPI
             printf("byte_count_limit=%d\n", byte_count_limit);
 #endif
-            if (byte_count_limit == 0xffff)
-                byte_count_limit--;
             size = s->packet_transfer_size;
             if (size > byte_count_limit) {
                 /* byte count limit must be even if this case */
@@ -1265,8 +1274,7 @@ void ide_atapi_cmd(IDEState *s)
      * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
     if (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
         /* TODO: Check IDENTIFY data word 125 for default BCL (currently 0) */
-        uint16_t byte_count_limit = s->lcyl | (s->hcyl << 8);
-        if (!(byte_count_limit || s->atapi_dma)) {
+        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);
             return;
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] atapi: Prioritize unknown cmd error over BCL error

[John Snow]

If we don't know about the command at all, we need to prioritize
that failure above the zero byte-count-limit failure.

This fixes a failure in the sparc64 NetBSD 7.0 installer bootup.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 1471ae2..52a989e 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1188,7 +1188,7 @@ enum {
     NONDATA = 0x04,
 };
 
-static const struct {
+static const struct AtapiCmd {
     void (*handler)(IDEState *s, uint8_t *buf);
     int flags;
 } atapi_cmd_table[0x100] = {
@@ -1215,9 +1215,9 @@ static const struct {
 
 void ide_atapi_cmd(IDEState *s)
 {
-    uint8_t *buf;
+    uint8_t *buf = s->io_buffer;
+    const struct AtapiCmd *cmd = &atapi_cmd_table[s->io_buffer[0]];
 
-    buf = s->io_buffer;
 #ifdef DEBUG_IDE_ATAPI
     {
         int i;
@@ -1228,14 +1228,14 @@ void ide_atapi_cmd(IDEState *s)
         printf("\n");
     }
 #endif
+
     /*
      * If there's a UNIT_ATTENTION condition pending, only command flagged with
      * ALLOW_UA are allowed to complete. with other commands getting a CHECK
      * condition response unless a higher priority status, defined by the drive
      * here, is pending.
      */
-    if (s->sense_key == UNIT_ATTENTION &&
-        !(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA)) {
+    if (s->sense_key == UNIT_ATTENTION && !(cmd->flags & ALLOW_UA)) {
         ide_atapi_cmd_check_status(s);
         return;
     }
@@ -1246,7 +1246,7 @@ void ide_atapi_cmd(IDEState *s)
      * GET_EVENT_STATUS_NOTIFICATION to detect such tray open/close
      * states rely on this behavior.
      */
-    if (!(atapi_cmd_table[s->io_buffer[0]].flags & ALLOW_UA) &&
+    if (!(cmd->flags & ALLOW_UA) &&
         !s->tray_open && blk_is_inserted(s->blk) && s->cdrom_changed) {
 
         if (s->cdrom_changed == 1) {
@@ -1261,7 +1261,7 @@ void ide_atapi_cmd(IDEState *s)
     }
 
     /* Report a Not Ready condition if appropriate for the command */
-    if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
+    if ((cmd->flags & CHECK_READY) &&
         (!media_present(s) || !blk_is_inserted(s->blk)))
     {
         ide_atapi_cmd_error(s, NOT_READY, ASC_MEDIUM_NOT_PRESENT);
@@ -1272,7 +1272,7 @@ void ide_atapi_cmd(IDEState *s)
      * 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 (!(atapi_cmd_table[s->io_buffer[0]].flags & NONDATA)) {
+    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 */
@@ -1282,8 +1282,8 @@ void ide_atapi_cmd(IDEState *s)
     }
 
     /* Execute the command */
-    if (atapi_cmd_table[s->io_buffer[0]].handler) {
-        atapi_cmd_table[s->io_buffer[0]].handler(s, buf);
+    if (cmd->handler) {
+        cmd->handler(s, buf);
         return;
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression

[Mark Cave-Ayland]

On 09/11/15 19:05, John Snow wrote:

> Marc noticed that a recent ATAPI permissions fix broke NetBSD 7.0's
> installer ISO.
> 
> The problem is that it's meaningless to check for !(cmd->flags & nondata)
> if the command isn't supported, since all unsupported commands have
> _no_ flags. Effectively, all commands default to "Transfer Data" in our
> model until we classify them otherwise.
> 
> This leads to a problem where we reject a zero byte BCL PIO command that
> transfers no data, simply because we have no properties for the command
> at all.
> 
> Getting an ATA rejection for this command greatly confuses NetBSD.
> 
> Correct behavior is to reject the command at the SCSI layer for being
> unsupported.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch atapi-bclimit-netbsd
> https://github.com/jnsnow/qemu/tree/atapi-bclimit-netbsd
> 
> This version is tagged atapi-bclimit-netbsd-v1:
> https://github.com/jnsnow/qemu/releases/tag/atapi-bclimit-netbsd-v1
> 
> John Snow (2):
>   atapi: add byte_count_limit helper
>   atapi: Prioritize unknown cmd error over BCL error
> 
>  hw/ide/atapi.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)

I've tested this against my OpenBIOS ISO images here and it fixes the
problem without introducing any further regressions, so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Many thanks,

Mark.