From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52425) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c0uF2-0003p5-10 for qemu-devel@nongnu.org; Sun, 30 Oct 2016 13:49:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c0uF0-0008MH-Vw for qemu-devel@nongnu.org; Sun, 30 Oct 2016 13:49:56 -0400 References: <1477693948-14419-1-git-send-email-jsnow@redhat.com> From: =?UTF-8?Q?Herv=c3=a9_Poussineau?= Message-ID: <11ea9a09-52d0-1f0b-add0-0d97f9f65af7@reactos.org> Date: Sun, 30 Oct 2016 18:49:28 +0100 MIME-Version: 1.0 In-Reply-To: <1477693948-14419-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] atapi: classify read_cd as conditionally returning data List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org Le 29/10/2016 =C3=A0 00:32, John Snow a =C3=A9crit : > 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=C3=A9 Poussineau > Signed-off-by: John Snow 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 hu= nk 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 ] =3D { cmd_read_dvd_structure, CHECK_READY }, [ 0xbb ] =3D { cmd_set_speed, NONDATA }, [ 0xbd ] =3D { cmd_mechanism_status, 0 }, - [ 0xbe ] =3D { cmd_read_cd, CHECK_READY }, + [ 0xbe ] =3D { cmd_read_cd, CHECK_READY | COND= DATA }, /* [1] handler detects and reports not ready condition itself */ }; If you add this hunk, you can add Tested-by: Herv=C3=A9 Poussineau Regards, Herv=C3=A9 > --- > 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 co= mmand > + * 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 fl= ow 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* b= uf) > return; > } > > - transfer_request =3D buf[9]; > - switch(transfer_request & 0xf8) { > - case 0x00: > + transfer_request =3D buf[9] & 0xf8; > + if (transfer_request =3D=3D 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 =3D 0x04, > + > + /* > + * CONDDATA implies a command that transfers data only conditional= ly 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 comm= and > + * handler level instead. > + */ > + CONDDATA =3D 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 t= o 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 (current= ly 0) */ > - if (!(atapi_byte_count_limit(s) || s->atapi_dma)) { > - /* TODO: Move abort back into core.c and make static inlin= e again */ > - ide_abort_command(s); > + if (cmd->handler && !(cmd->flags & (NONDATA | CONDDATA))) { > + if (!validate_bcl(s)) { > return; > } > } >