| Message ID | f6b374c65b0b3c95edc73d6b39abae1fdbcd0750.1302152648.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/07/2011 07:05 AM, Amit Shah wrote: > We restrict the commands that a guest can send us after a cdrom change > event. The current list includes REQUEST_SENSE and INQUIRY commands. > Guests can also issue TEST_UNIT_READY to inquire for the status, so > allow this command as well. > > Signed-off-by: Amit Shah<amit.shah@redhat.com> > --- > hw/ide/core.c | 9 +++++---- > 1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 007a4ee..d55d804 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s) > /* If there's a UNIT_ATTENTION condition pending, only > REQUEST_SENSE and INQUIRY commands are allowed to complete. */ > if (s->sense_key == SENSE_UNIT_ATTENTION&& > - s->io_buffer[0] != GPCMD_REQUEST_SENSE&& > - s->io_buffer[0] != GPCMD_INQUIRY) { > - ide_atapi_cmd_check_status(s); > - return; > + s->io_buffer[0] != GPCMD_REQUEST_SENSE&& > + s->io_buffer[0] != GPCMD_INQUIRY&& > + s->io_buffer[0] != GPCMD_TEST_UNIT_READY) { > + ide_atapi_cmd_check_status(s); > + return; > } > switch(s->io_buffer[0]) { > case GPCMD_TEST_UNIT_READY: ACK Paolo
On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote: > @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s) > /* If there's a UNIT_ATTENTION condition pending, only > REQUEST_SENSE and INQUIRY commands are allowed to complete. */ > if (s->sense_key == SENSE_UNIT_ATTENTION && > - s->io_buffer[0] != GPCMD_REQUEST_SENSE && > - s->io_buffer[0] != GPCMD_INQUIRY) { > - ide_atapi_cmd_check_status(s); > - return; > + s->io_buffer[0] != GPCMD_REQUEST_SENSE && > + s->io_buffer[0] != GPCMD_INQUIRY && > + s->io_buffer[0] != GPCMD_TEST_UNIT_READY) { > + ide_atapi_cmd_check_status(s); > + return; Looks good but please update the comment. Stefan
On (Thu) 07 Apr 2011 [09:42:30], Stefan Hajnoczi wrote: > On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote: > > @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s) > > /* If there's a UNIT_ATTENTION condition pending, only > > REQUEST_SENSE and INQUIRY commands are allowed to complete. */ > > if (s->sense_key == SENSE_UNIT_ATTENTION && > > - s->io_buffer[0] != GPCMD_REQUEST_SENSE && > > - s->io_buffer[0] != GPCMD_INQUIRY) { > > - ide_atapi_cmd_check_status(s); > > - return; > > + s->io_buffer[0] != GPCMD_REQUEST_SENSE && > > + s->io_buffer[0] != GPCMD_INQUIRY && > > + s->io_buffer[0] != GPCMD_TEST_UNIT_READY) { > > + ide_atapi_cmd_check_status(s); > > + return; > > Looks good but please update the comment. Argh! I'll update to a generic one as there are more commands that we need to allow. Amit
Am 07.04.2011 07:05, schrieb Amit Shah: > We restrict the commands that a guest can send us after a cdrom change > event. The current list includes REQUEST_SENSE and INQUIRY commands. > Guests can also issue TEST_UNIT_READY to inquire for the status, so > allow this command as well. > > Signed-off-by: Amit Shah <amit.shah@redhat.com> Hm... MMC-5, section 4.1.6.1 seems to conflict with this: "If a Host issues a command other than GET CONFIGURATION, GET EVENT STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention condition exists for that Host, the Drive shall not perform the command and shall report CHECK CONDITION status unless a higher priority status as defined by the Drive is also pending." So while you're right that our list is incomplete, TEST UNIT READY doesn't seem to be among the missing commands. Kevin
On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote: > Am 07.04.2011 07:05, schrieb Amit Shah: > > We restrict the commands that a guest can send us after a cdrom change > > event. The current list includes REQUEST_SENSE and INQUIRY commands. > > Guests can also issue TEST_UNIT_READY to inquire for the status, so > > allow this command as well. > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > > Hm... MMC-5, section 4.1.6.1 seems to conflict with this: > > "If a Host issues a command other than GET CONFIGURATION, GET EVENT > STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention > condition exists for that Host, the Drive shall not perform the command > and shall report CHECK CONDITION status unless a higher priority status > as defined by the Drive is also pending." > > So while you're right that our list is incomplete, TEST UNIT READY > doesn't seem to be among the missing commands. Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's commit mentioned, rely on this command to get CD change notifications: /* identical to scsi_test_unit_ready except that it doesn't * eat the NOT_READY returns for removable media */ int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr) { int retries = MAX_RETRIES; int the_result; u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 }; /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION * conditions are gone, or a timeout happens */ do { the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, sshdr, SR_TIMEOUT, retries--, NULL); if (scsi_sense_valid(sshdr) && sshdr->sense_key == UNIT_ATTENTION) sdev->changed = 1; } while (retries > 0 && (!scsi_status_is_good(the_result) || (scsi_sense_valid(sshdr) && sshdr->sense_key == UNIT_ATTENTION))); return the_result; } There could be something else brewing as well. We never report a 'tray open' condition; maybe the guest will be happy with such a thing as well. Will have to check. Amit
Am 07.04.2011 11:11, schrieb Amit Shah: > On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote: >> Am 07.04.2011 07:05, schrieb Amit Shah: >>> We restrict the commands that a guest can send us after a cdrom change >>> event. The current list includes REQUEST_SENSE and INQUIRY commands. >>> Guests can also issue TEST_UNIT_READY to inquire for the status, so >>> allow this command as well. >>> >>> Signed-off-by: Amit Shah <amit.shah@redhat.com> >> >> Hm... MMC-5, section 4.1.6.1 seems to conflict with this: >> >> "If a Host issues a command other than GET CONFIGURATION, GET EVENT >> STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention >> condition exists for that Host, the Drive shall not perform the command >> and shall report CHECK CONDITION status unless a higher priority status >> as defined by the Drive is also pending." >> >> So while you're right that our list is incomplete, TEST UNIT READY >> doesn't seem to be among the missing commands. > > Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's > commit mentioned, rely on this command to get CD change notifications: > > /* identical to scsi_test_unit_ready except that it doesn't > * eat the NOT_READY returns for removable media */ > int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr) > { > int retries = MAX_RETRIES; > int the_result; > u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 }; > > /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION > * conditions are gone, or a timeout happens > */ > do { > the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, > 0, sshdr, SR_TIMEOUT, > retries--, NULL); > if (scsi_sense_valid(sshdr) && > sshdr->sense_key == UNIT_ATTENTION) > sdev->changed = 1; > > } while (retries > 0 && > (!scsi_status_is_good(the_result) || > (scsi_sense_valid(sshdr) && > sshdr->sense_key == UNIT_ATTENTION))); > return the_result; > } I think the scsi_execute_req() call might issue a REQUEST SENSE internally and therefore clear the unit attention condition. Tried to check that in the source, but I'm hopelessly lost in the kernel... Kevin
On (Thu) 07 Apr 2011 [12:20:16], Kevin Wolf wrote: > Am 07.04.2011 11:11, schrieb Amit Shah: > > On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote: > >> Am 07.04.2011 07:05, schrieb Amit Shah: > >>> We restrict the commands that a guest can send us after a cdrom change > >>> event. The current list includes REQUEST_SENSE and INQUIRY commands. > >>> Guests can also issue TEST_UNIT_READY to inquire for the status, so > >>> allow this command as well. > >>> > >>> Signed-off-by: Amit Shah <amit.shah@redhat.com> > >> > >> Hm... MMC-5, section 4.1.6.1 seems to conflict with this: > >> > >> "If a Host issues a command other than GET CONFIGURATION, GET EVENT > >> STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention > >> condition exists for that Host, the Drive shall not perform the command > >> and shall report CHECK CONDITION status unless a higher priority status > >> as defined by the Drive is also pending." > >> > >> So while you're right that our list is incomplete, TEST UNIT READY > >> doesn't seem to be among the missing commands. > > > > Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's > > commit mentioned, rely on this command to get CD change notifications: > > > > /* identical to scsi_test_unit_ready except that it doesn't > > * eat the NOT_READY returns for removable media */ > > int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr) > > { > > int retries = MAX_RETRIES; > > int the_result; > > u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 }; > > > > /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION > > * conditions are gone, or a timeout happens > > */ > > do { > > the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, > > 0, sshdr, SR_TIMEOUT, > > retries--, NULL); > > if (scsi_sense_valid(sshdr) && > > sshdr->sense_key == UNIT_ATTENTION) > > sdev->changed = 1; > > > > } while (retries > 0 && > > (!scsi_status_is_good(the_result) || > > (scsi_sense_valid(sshdr) && > > sshdr->sense_key == UNIT_ATTENTION))); > > return the_result; > > } > > I think the scsi_execute_req() call might issue a REQUEST SENSE > internally and therefore clear the unit attention condition. Tried to > check that in the source, but I'm hopelessly lost in the kernel... No, it doesn't. It's clear the 2nd patch is needed; the 1st one must be papering over something else. Right now I'm inclined to think that it's because we don't report tray open and no media as separate events to the guest, as Markus has found out. Amit
diff --git a/hw/ide/core.c b/hw/ide/core.c index 007a4ee..d55d804 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s) /* If there's a UNIT_ATTENTION condition pending, only REQUEST_SENSE and INQUIRY commands are allowed to complete. */ if (s->sense_key == SENSE_UNIT_ATTENTION && - s->io_buffer[0] != GPCMD_REQUEST_SENSE && - s->io_buffer[0] != GPCMD_INQUIRY) { - ide_atapi_cmd_check_status(s); - return; + s->io_buffer[0] != GPCMD_REQUEST_SENSE && + s->io_buffer[0] != GPCMD_INQUIRY && + s->io_buffer[0] != GPCMD_TEST_UNIT_READY) { + ide_atapi_cmd_check_status(s); + return; } switch(s->io_buffer[0]) { case GPCMD_TEST_UNIT_READY:
We restrict the commands that a guest can send us after a cdrom change event. The current list includes REQUEST_SENSE and INQUIRY commands. Guests can also issue TEST_UNIT_READY to inquire for the status, so allow this command as well. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/ide/core.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)