| Message ID | cf086a4003c7edad2c08e7f1757f761c6abca561.1302152648.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/07/2011 07:05 AM, Amit Shah wrote: > Commit 93c8cfd9e67a62711b86f4c93747566885eb7928 tried to send a 'no > disc' event after a cdrom change so that guests notice a cd change event > between two 'cd present' states. However, we don't go from > > 'cd present' -> 'no cd' -> 'cd present' > > as the SENSE_UNIT_ATTENTION sense_key is written over by the > ide_atapi_cmd_error() function. > > So for the disc change event, let us ensure the error() function doesn't > trample over that value so we do get to report it the next time around. > Also, ensure we go from 'no cd' to 'cd present' state. > > With this, older Linux guests (< 2.6.38) notice cd changes just fine. > For newer Linux guests (2.6.38+) cd change events break again and that > will be fixed by implementing the GET_EVENT_STATUS_NOTIFICATION command. > > Signed-off-by: Amit Shah<amit.shah@redhat.com> > --- > hw/ide/core.c | 42 +++++++++++++++++++++++++++++++++++++++--- > hw/ide/internal.h | 1 + > 2 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index d55d804..abb577c 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1113,10 +1113,42 @@ static void ide_atapi_cmd(IDEState *s) > } > switch(s->io_buffer[0]) { > case GPCMD_TEST_UNIT_READY: > - if (bdrv_is_inserted(s->bs)&& !s->cdrom_changed) { > - ide_atapi_cmd_ok(s); > + if (bdrv_is_inserted(s->bs)) { > + int sense, asc; > + > + sense = s->sense_key; > + asc = s->asc; > + > + /* > + * First, check if there's any pending media change > + * notification to be given. > + * > + * We want the guest to notice an empty tray between a cd > + * change, so send one MEDIUM_NOT_PRESENT message after a > + * cd change. > + * > + * After we've sent that message, the guest will poke at > + * us again and send the UNIT_ATTENTION message then. > + * Once this is done, reset the UNIT_ATTENTION message to > + * ensure we don't keep repeating it. > + */ > + if (!s->media_change_notified) { > + ide_atapi_cmd_error(s, SENSE_NOT_READY, > + ASC_MEDIUM_NOT_PRESENT); > + s->media_change_notified = 1; > + } else if (s->cdrom_changed) { > + s->sense_key = SENSE_UNIT_ATTENTION; > + s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > + ide_atapi_cmd_ok(s); > + > + s->cdrom_changed = 0; > + sense = SENSE_NONE; > + } else { > + ide_atapi_cmd_ok(s); > + } > + s->sense_key = sense; > + s->asc = asc; > } else { > - s->cdrom_changed = 0; > ide_atapi_cmd_error(s, SENSE_NOT_READY, > ASC_MEDIUM_NOT_PRESENT); > } > @@ -1613,6 +1645,7 @@ static void cdrom_change_cb(void *opaque, int reason) > s->sense_key = SENSE_UNIT_ATTENTION; > s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > s->cdrom_changed = 1; > + s->media_change_notified = 0; > ide_set_irq(s->bus); > } > > @@ -2474,6 +2507,7 @@ static void ide_reset(IDEState *s) > s->sense_key = 0; > s->asc = 0; > s->cdrom_changed = 0; > + s->media_change_notified = 0; > s->packet_transfer_size = 0; > s->elementary_transfer_size = 0; > s->io_buffer_index = 0; > @@ -2713,6 +2747,8 @@ static int ide_drive_post_load(void *opaque, int version_id) > if (s->sense_key == SENSE_UNIT_ATTENTION&& > s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) { > s->cdrom_changed = 1; > + } else { This is within a version_id < 3 condition, so media_change_notified is never set to 1 when loading from a current QEMU. Integrating the new flag into cdrom_changed (0 = ok, 1 = cdrom changed and NOT_PRESENT sent, 2 = cdrom changed and NOT_PRESENT not sent) should also work for older guests. They just treat 2 the same as 1. Paolo
On (Thu) 07 Apr 2011 [09:31:05], Paolo Bonzini wrote: > On 04/07/2011 07:05 AM, Amit Shah wrote: > >Commit 93c8cfd9e67a62711b86f4c93747566885eb7928 tried to send a 'no > >disc' event after a cdrom change so that guests notice a cd change event > >between two 'cd present' states. However, we don't go from > > > > 'cd present' -> 'no cd' -> 'cd present' > > > >as the SENSE_UNIT_ATTENTION sense_key is written over by the > >ide_atapi_cmd_error() function. > > > >So for the disc change event, let us ensure the error() function doesn't > >trample over that value so we do get to report it the next time around. > >Also, ensure we go from 'no cd' to 'cd present' state. > > > >With this, older Linux guests (< 2.6.38) notice cd changes just fine. > >For newer Linux guests (2.6.38+) cd change events break again and that > >will be fixed by implementing the GET_EVENT_STATUS_NOTIFICATION command. > > > >Signed-off-by: Amit Shah<amit.shah@redhat.com> > >--- > > hw/ide/core.c | 42 +++++++++++++++++++++++++++++++++++++++--- > > hw/ide/internal.h | 1 + > > 2 files changed, 40 insertions(+), 3 deletions(-) > > > >diff --git a/hw/ide/core.c b/hw/ide/core.c > >index d55d804..abb577c 100644 > >--- a/hw/ide/core.c > >+++ b/hw/ide/core.c > >@@ -1113,10 +1113,42 @@ static void ide_atapi_cmd(IDEState *s) > > } > > switch(s->io_buffer[0]) { > > case GPCMD_TEST_UNIT_READY: > >- if (bdrv_is_inserted(s->bs)&& !s->cdrom_changed) { > >- ide_atapi_cmd_ok(s); > >+ if (bdrv_is_inserted(s->bs)) { > >+ int sense, asc; > >+ > >+ sense = s->sense_key; > >+ asc = s->asc; > >+ > >+ /* > >+ * First, check if there's any pending media change > >+ * notification to be given. > >+ * > >+ * We want the guest to notice an empty tray between a cd > >+ * change, so send one MEDIUM_NOT_PRESENT message after a > >+ * cd change. > >+ * > >+ * After we've sent that message, the guest will poke at > >+ * us again and send the UNIT_ATTENTION message then. > >+ * Once this is done, reset the UNIT_ATTENTION message to > >+ * ensure we don't keep repeating it. > >+ */ > >+ if (!s->media_change_notified) { > >+ ide_atapi_cmd_error(s, SENSE_NOT_READY, > >+ ASC_MEDIUM_NOT_PRESENT); > >+ s->media_change_notified = 1; > >+ } else if (s->cdrom_changed) { > >+ s->sense_key = SENSE_UNIT_ATTENTION; > >+ s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > >+ ide_atapi_cmd_ok(s); > >+ > >+ s->cdrom_changed = 0; > >+ sense = SENSE_NONE; > >+ } else { > >+ ide_atapi_cmd_ok(s); > >+ } > >+ s->sense_key = sense; > >+ s->asc = asc; > > } else { > >- s->cdrom_changed = 0; > > ide_atapi_cmd_error(s, SENSE_NOT_READY, > > ASC_MEDIUM_NOT_PRESENT); > > } > >@@ -1613,6 +1645,7 @@ static void cdrom_change_cb(void *opaque, int reason) > > s->sense_key = SENSE_UNIT_ATTENTION; > > s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > > s->cdrom_changed = 1; > >+ s->media_change_notified = 0; > > ide_set_irq(s->bus); > > } > > > >@@ -2474,6 +2507,7 @@ static void ide_reset(IDEState *s) > > s->sense_key = 0; > > s->asc = 0; > > s->cdrom_changed = 0; > >+ s->media_change_notified = 0; > > s->packet_transfer_size = 0; > > s->elementary_transfer_size = 0; > > s->io_buffer_index = 0; > >@@ -2713,6 +2747,8 @@ static int ide_drive_post_load(void *opaque, int version_id) > > if (s->sense_key == SENSE_UNIT_ATTENTION&& > > s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) { > > s->cdrom_changed = 1; > >+ } else { > > This is within a version_id < 3 condition, so media_change_notified > is never set to 1 when loading from a current QEMU. Right. > Integrating the new flag into cdrom_changed (0 = ok, 1 = cdrom > changed and NOT_PRESENT sent, 2 = cdrom changed and NOT_PRESENT not > sent) should also work for older guests. They just treat 2 the same > as 1. Yes, thanks -- I was thinking the same. v3 coming up. Amit
On Thu, Apr 07, 2011 at 10:35:19AM +0530, Amit Shah wrote: > + /* > + * First, check if there's any pending media change > + * notification to be given. > + * > + * We want the guest to notice an empty tray between a cd > + * change, so send one MEDIUM_NOT_PRESENT message after a > + * cd change. > + * > + * After we've sent that message, the guest will poke at > + * us again and send the UNIT_ATTENTION message then. > + * Once this is done, reset the UNIT_ATTENTION message to > + * ensure we don't keep repeating it. > + */ Indentation is off here. > + if (!s->media_change_notified) { > + ide_atapi_cmd_error(s, SENSE_NOT_READY, > + ASC_MEDIUM_NOT_PRESENT); > + s->media_change_notified = 1; > + } else if (s->cdrom_changed) { > + s->sense_key = SENSE_UNIT_ATTENTION; > + s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > + ide_atapi_cmd_ok(s); > + > + s->cdrom_changed = 0; > + sense = SENSE_NONE; Indentation is off here. Otherwise looks good. I will test a Windows 2003 Server guest. Stefan
On (Thu) 07 Apr 2011 [09:41:09], Stefan Hajnoczi wrote: > On Thu, Apr 07, 2011 at 10:35:19AM +0530, Amit Shah wrote: > > + /* > > + * First, check if there's any pending media change > > + * notification to be given. > > + * > > + * We want the guest to notice an empty tray between a cd > > + * change, so send one MEDIUM_NOT_PRESENT message after a > > + * cd change. > > + * > > + * After we've sent that message, the guest will poke at > > + * us again and send the UNIT_ATTENTION message then. > > + * Once this is done, reset the UNIT_ATTENTION message to > > + * ensure we don't keep repeating it. > > + */ > > Indentation is off here. Fixed in v3. > > + if (!s->media_change_notified) { > > + ide_atapi_cmd_error(s, SENSE_NOT_READY, > > + ASC_MEDIUM_NOT_PRESENT); > > + s->media_change_notified = 1; > > + } else if (s->cdrom_changed) { > > + s->sense_key = SENSE_UNIT_ATTENTION; > > + s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; > > + ide_atapi_cmd_ok(s); > > + > > + s->cdrom_changed = 0; > > + sense = SENSE_NONE; > > Indentation is off here. > > Otherwise looks good. I will test a Windows 2003 Server guest. Thanks; I tested Win XP and RHEL6 guests. Amit
diff --git a/hw/ide/core.c b/hw/ide/core.c index d55d804..abb577c 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1113,10 +1113,42 @@ static void ide_atapi_cmd(IDEState *s) } switch(s->io_buffer[0]) { case GPCMD_TEST_UNIT_READY: - if (bdrv_is_inserted(s->bs) && !s->cdrom_changed) { - ide_atapi_cmd_ok(s); + if (bdrv_is_inserted(s->bs)) { + int sense, asc; + + sense = s->sense_key; + asc = s->asc; + + /* + * First, check if there's any pending media change + * notification to be given. + * + * We want the guest to notice an empty tray between a cd + * change, so send one MEDIUM_NOT_PRESENT message after a + * cd change. + * + * After we've sent that message, the guest will poke at + * us again and send the UNIT_ATTENTION message then. + * Once this is done, reset the UNIT_ATTENTION message to + * ensure we don't keep repeating it. + */ + if (!s->media_change_notified) { + ide_atapi_cmd_error(s, SENSE_NOT_READY, + ASC_MEDIUM_NOT_PRESENT); + s->media_change_notified = 1; + } else if (s->cdrom_changed) { + s->sense_key = SENSE_UNIT_ATTENTION; + s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; + ide_atapi_cmd_ok(s); + + s->cdrom_changed = 0; + sense = SENSE_NONE; + } else { + ide_atapi_cmd_ok(s); + } + s->sense_key = sense; + s->asc = asc; } else { - s->cdrom_changed = 0; ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT); } @@ -1613,6 +1645,7 @@ static void cdrom_change_cb(void *opaque, int reason) s->sense_key = SENSE_UNIT_ATTENTION; s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED; s->cdrom_changed = 1; + s->media_change_notified = 0; ide_set_irq(s->bus); } @@ -2474,6 +2507,7 @@ static void ide_reset(IDEState *s) s->sense_key = 0; s->asc = 0; s->cdrom_changed = 0; + s->media_change_notified = 0; s->packet_transfer_size = 0; s->elementary_transfer_size = 0; s->io_buffer_index = 0; @@ -2713,6 +2747,8 @@ static int ide_drive_post_load(void *opaque, int version_id) if (s->sense_key == SENSE_UNIT_ATTENTION && s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) { s->cdrom_changed = 1; + } else { + s->media_change_notified = 1; } } return 0; diff --git a/hw/ide/internal.h b/hw/ide/internal.h index d533fb6..5d58be4 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -411,6 +411,7 @@ struct IDEState { uint8_t sense_key; uint8_t asc; uint8_t cdrom_changed; + uint8_t media_change_notified; int packet_transfer_size; int elementary_transfer_size; int io_buffer_index;
Commit 93c8cfd9e67a62711b86f4c93747566885eb7928 tried to send a 'no disc' event after a cdrom change so that guests notice a cd change event between two 'cd present' states. However, we don't go from 'cd present' -> 'no cd' -> 'cd present' as the SENSE_UNIT_ATTENTION sense_key is written over by the ide_atapi_cmd_error() function. So for the disc change event, let us ensure the error() function doesn't trample over that value so we do get to report it the next time around. Also, ensure we go from 'no cd' to 'cd present' state. With this, older Linux guests (< 2.6.38) notice cd changes just fine. For newer Linux guests (2.6.38+) cd change events break again and that will be fixed by implementing the GET_EVENT_STATUS_NOTIFICATION command. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/ide/core.c | 42 +++++++++++++++++++++++++++++++++++++++--- hw/ide/internal.h | 1 + 2 files changed, 40 insertions(+), 3 deletions(-)