* [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
@ 2016-10-28 22:32 John Snow
2016-10-30 17:49 ` Hervé Poussineau
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: John Snow @ 2016-10-28 22:32 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hpoussin, qemu-devel, John Snow
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 <hpoussin@reactos.org>
Signed-off-by: John Snow <jsnow@redhat.com>
---
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;
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
2016-10-28 22:32 [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data John Snow
@ 2016-10-30 17:49 ` Hervé Poussineau
2016-10-31 15:10 ` John Snow
2016-10-31 2:57 ` no-reply
2016-10-31 14:06 ` Kevin Wolf
2 siblings, 1 reply; 6+ messages in thread
From: Hervé Poussineau @ 2016-10-30 17:49 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, qemu-devel
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 <hpoussin@reactos.org>
> Signed-off-by: John Snow <jsnow@redhat.com>
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 <hpoussin@reactos.org>
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;
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
2016-10-28 22:32 [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data John Snow
2016-10-30 17:49 ` Hervé Poussineau
@ 2016-10-31 2:57 ` no-reply
2016-10-31 14:06 ` Kevin Wolf
2 siblings, 0 replies; 6+ messages in thread
From: no-reply @ 2016-10-31 2:57 UTC (permalink / raw)
To: famz, jsnow; +Cc: qemu-block, kwolf, hpoussin, qemu-devel
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
Message-id: 1477693948-14419-1-git-send-email-jsnow@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
55c3ba0 atapi: classify read_cd as conditionally returning data
=== OUTPUT BEGIN ===
fatal: unrecognized argument: --no-patch
Checking PATCH 1/1: ...
ERROR: space required before the open parenthesis '('
#66: FILE: hw/ide/atapi.c:1060:
+ switch(transfer_request) {
total: 1 errors, 0 warnings, 78 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
2016-10-28 22:32 [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data John Snow
2016-10-30 17:49 ` Hervé Poussineau
2016-10-31 2:57 ` no-reply
@ 2016-10-31 14:06 ` Kevin Wolf
2016-10-31 15:12 ` John Snow
2 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2016-10-31 14:06 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, hpoussin, qemu-devel
Am 29.10.2016 um 00:32 hat John Snow geschrieben:
> 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 <hpoussin@reactos.org>
> Signed-off-by: John Snow <jsnow@redhat.com>
Wouldn't it be useful to actually add the new flag to cmd_read_cd then?
While at it, I would also split the patch into one for adding the flag
and one for updating cmd_read_cd.
Also, would it be hard to add a qtest case for this?
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
2016-10-30 17:49 ` Hervé Poussineau
@ 2016-10-31 15:10 ` John Snow
0 siblings, 0 replies; 6+ messages in thread
From: John Snow @ 2016-10-31 15:10 UTC (permalink / raw)
To: Hervé Poussineau, qemu-block; +Cc: kwolf, qemu-devel
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 <hpoussin@reactos.org>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> 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 <hpoussin@reactos.org>
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data
2016-10-31 14:06 ` Kevin Wolf
@ 2016-10-31 15:12 ` John Snow
0 siblings, 0 replies; 6+ messages in thread
From: John Snow @ 2016-10-31 15:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, hpoussin, qemu-devel
On 10/31/2016 10:06 AM, Kevin Wolf wrote:
> Am 29.10.2016 um 00:32 hat John Snow geschrieben:
>> 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 <hpoussin@reactos.org>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Wouldn't it be useful to actually add the new flag to cmd_read_cd then?
> While at it, I would also split the patch into one for adding the flag
> and one for updating cmd_read_cd.
>
As pointed out by Herve, no -- sorry.
> Also, would it be hard to add a qtest case for this?
>
I could add it to AHCI pretty easily.
> Kevin
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-31 15:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-28 22:32 [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data John Snow
2016-10-30 17:49 ` Hervé Poussineau
2016-10-31 15:10 ` John Snow
2016-10-31 2:57 ` no-reply
2016-10-31 14:06 ` Kevin Wolf
2016-10-31 15:12 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).