* [Qemu-devel] [PATCH 1/2] atapi: add byte_count_limit helper
2015-11-09 19:05 [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression John Snow
@ 2015-11-09 19:05 ` John Snow
2015-11-09 19:05 ` [Qemu-devel] [PATCH 2/2] atapi: Prioritize unknown cmd error over BCL error John Snow
2015-11-09 21:34 ` [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression Mark Cave-Ayland
2 siblings, 0 replies; 4+ messages in thread
From: John Snow @ 2015-11-09 19:05 UTC (permalink / raw)
To: qemu-block; +Cc: John Snow, mark.cave-ayland, armbru, qemu-devel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH 2/2] atapi: Prioritize unknown cmd error over BCL error
2015-11-09 19:05 [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression John Snow
2015-11-09 19:05 ` [Qemu-devel] [PATCH 1/2] atapi: add byte_count_limit helper John Snow
@ 2015-11-09 19:05 ` John Snow
2015-11-09 21:34 ` [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression Mark Cave-Ayland
2 siblings, 0 replies; 4+ messages in thread
From: John Snow @ 2015-11-09 19:05 UTC (permalink / raw)
To: qemu-block; +Cc: John Snow, mark.cave-ayland, armbru, qemu-devel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression
2015-11-09 19:05 [Qemu-devel] [PATCH 0/2] atapi: fix NetBSD boot regression John Snow
2015-11-09 19:05 ` [Qemu-devel] [PATCH 1/2] atapi: add byte_count_limit helper John Snow
2015-11-09 19:05 ` [Qemu-devel] [PATCH 2/2] atapi: Prioritize unknown cmd error over BCL error John Snow
@ 2015-11-09 21:34 ` Mark Cave-Ayland
2 siblings, 0 replies; 4+ messages in thread
From: Mark Cave-Ayland @ 2015-11-09 21:34 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: armbru, qemu-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread