| Message ID | 1632463ee09a9df5690475f0b507757c0876fce9.1302617257.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 12.04.2011 16:09, schrieb Amit Shah: > Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION > command. This helps us report tray open, tray closed, no media, media > present states to the guest. > > Newer Linux kernels (2.6.38+) rely on this command to revalidate discs > after media change. > > This patch also sends out tray open/closed status to the guest driver > when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). > Without such notification, the guest and qemu's tray open/close status > was frequently out of sync, causing installers like Anaconda detecting > no disc instead of tray open, confusing them terribly. > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > --- > hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > hw/ide/internal.h | 6 +++ > 2 files changed, 110 insertions(+), 5 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index dafc049..209d8e6 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > } > } > > +static unsigned int event_status_media(IDEState *s, > + uint8_t *buf, > + unsigned int max_len) > +{ > + enum media_event_code { > + no_change = 0, /* Status unchanged */ > + eject_requested, /* received a request from user to eject */ > + new_media, /* new media inserted and ready for access */ > + media_removal, /* only for media changers */ > + media_changed, /* only for media changers */ > + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > + }; > + enum media_status { > + tray_open = 1, > + media_present = 2, > + }; > + uint8_t event_code, media_status; > + > + media_status = 0; > + if (s->bs->tray_open) { > + media_status = tray_open; > + } else if (bdrv_is_inserted(s->bs)) { > + media_status = media_present; > + } > + > + /* Event notification descriptor */ > + event_code = no_change; > + if (media_status != tray_open && s->events.new_media) { > + event_code = new_media; > + s->events.new_media = false; > + } > + > + buf[4] = event_code; > + buf[5] = media_status; > + > + /* These fields are reserved, just clear them. */ > + buf[6] = 0; > + buf[7] = 0; > + > + return 8; /* We wrote to 4 extra bytes from the header */ > +} > + > static void handle_get_event_status_notification(IDEState *s, > uint8_t *buf, > const uint8_t *packet) > @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > uint8_t supported_events; > } __attribute((packed)) *gesn_event_header; > > + enum notification_class_request_type { > + reserved1 = 1 << 0, > + operational_change = 1 << 1, > + power_management = 1 << 2, > + external_request = 1 << 3, > + media = 1 << 4, > + multi_host = 1 << 5, > + device_busy = 1 << 6, > + reserved2 = 1 << 7, > + }; > + enum event_notification_class_field { > + enc_no_events = 0, > + enc_operational_change, > + enc_power_management, > + enc_external_request, > + enc_media, > + enc_multiple_hosts, > + enc_device_busy, > + enc_reserved, > + }; > unsigned int max_len, used_len; > > gesn_cdb = (void *)packet; > @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, > > /* polling mode operation */ > > - /* We don't support any event class (yet). */ > - gesn_event_header->supported_events = 0; > - > - gesn_event_header->notification_class = 0x80; /* No event available */ > - used_len = sizeof(*gesn_event_header); > + /* > + * These are the supported events. > + * > + * We currently only support requests of the 'media' type. > + */ > + gesn_event_header->supported_events = media; > > + /* > + * Responses to requests are to be based on request priority. The > + * notification_class_request_type enum above specifies the > + * priority: upper elements are higher prio than lower ones. > + */ > + if (gesn_cdb->class & media) { > + gesn_event_header->notification_class |= enc_media; Am I missing where this field is initialized or should it be = instead of |= ? The rest of the series looks good, but I have seen this code by far too often now, so maybe someone else should take over and find the remaining bugs that I'm missing now... Kevin
On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote: > Am 12.04.2011 16:09, schrieb Amit Shah: > > Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION > > command. This helps us report tray open, tray closed, no media, media > > present states to the guest. > > > > Newer Linux kernels (2.6.38+) rely on this command to revalidate discs > > after media change. > > > > This patch also sends out tray open/closed status to the guest driver > > when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). > > Without such notification, the guest and qemu's tray open/close status > > was frequently out of sync, causing installers like Anaconda detecting > > no disc instead of tray open, confusing them terribly. > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > > --- > > hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > hw/ide/internal.h | 6 +++ > > 2 files changed, 110 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ide/core.c b/hw/ide/core.c > > index dafc049..209d8e6 100644 > > --- a/hw/ide/core.c > > +++ b/hw/ide/core.c > > @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > > } > > } > > > > +static unsigned int event_status_media(IDEState *s, > > + uint8_t *buf, > > + unsigned int max_len) > > +{ > > + enum media_event_code { > > + no_change = 0, /* Status unchanged */ > > + eject_requested, /* received a request from user to eject */ > > + new_media, /* new media inserted and ready for access */ > > + media_removal, /* only for media changers */ > > + media_changed, /* only for media changers */ > > + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > > + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > > + }; > > + enum media_status { > > + tray_open = 1, > > + media_present = 2, > > + }; > > + uint8_t event_code, media_status; > > + > > + media_status = 0; > > + if (s->bs->tray_open) { > > + media_status = tray_open; > > + } else if (bdrv_is_inserted(s->bs)) { > > + media_status = media_present; > > + } > > + > > + /* Event notification descriptor */ > > + event_code = no_change; > > + if (media_status != tray_open && s->events.new_media) { > > + event_code = new_media; > > + s->events.new_media = false; > > + } > > + > > + buf[4] = event_code; > > + buf[5] = media_status; > > + > > + /* These fields are reserved, just clear them. */ > > + buf[6] = 0; > > + buf[7] = 0; > > + > > + return 8; /* We wrote to 4 extra bytes from the header */ > > +} > > + > > static void handle_get_event_status_notification(IDEState *s, > > uint8_t *buf, > > const uint8_t *packet) > > @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > > uint8_t supported_events; > > } __attribute((packed)) *gesn_event_header; > > > > + enum notification_class_request_type { > > + reserved1 = 1 << 0, > > + operational_change = 1 << 1, > > + power_management = 1 << 2, > > + external_request = 1 << 3, > > + media = 1 << 4, > > + multi_host = 1 << 5, > > + device_busy = 1 << 6, > > + reserved2 = 1 << 7, > > + }; > > + enum event_notification_class_field { > > + enc_no_events = 0, > > + enc_operational_change, > > + enc_power_management, > > + enc_external_request, > > + enc_media, > > + enc_multiple_hosts, > > + enc_device_busy, > > + enc_reserved, > > + }; > > unsigned int max_len, used_len; > > > > gesn_cdb = (void *)packet; > > @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, > > > > /* polling mode operation */ > > > > - /* We don't support any event class (yet). */ > > - gesn_event_header->supported_events = 0; > > - > > - gesn_event_header->notification_class = 0x80; /* No event available */ > > - used_len = sizeof(*gesn_event_header); > > + /* > > + * These are the supported events. > > + * > > + * We currently only support requests of the 'media' type. > > + */ > > + gesn_event_header->supported_events = media; > > > > + /* > > + * Responses to requests are to be based on request priority. The > > + * notification_class_request_type enum above specifies the > > + * priority: upper elements are higher prio than lower ones. > > + */ > > + if (gesn_cdb->class & media) { > > + gesn_event_header->notification_class |= enc_media; > > Am I missing where this field is initialized or should it be = instead > of |= ? I considered both: if the 'reserved' bits become available and if we should write to them, |= makes more sense. Currently, = will suffice. Strong preference? Amit
On 04/12/11 16:09, Amit Shah wrote: > diff --git a/hw/ide/core.c b/hw/ide/core.c > index dafc049..209d8e6 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > } > } > > +static unsigned int event_status_media(IDEState *s, > + uint8_t *buf, > + unsigned int max_len) > +{ > + enum media_event_code { > + no_change = 0, /* Status unchanged */ > + eject_requested, /* received a request from user to eject */ > + new_media, /* new media inserted and ready for access */ > + media_removal, /* only for media changers */ > + media_changed, /* only for media changers */ > + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > + }; > + enum media_status { > + tray_open = 1, > + media_present = 2, > + }; Would be nice if you used upper-case with the enums since they are effectively just defines, to avoid confusing them with variables. > + uint8_t event_code, media_status; > + > + media_status = 0; > + if (s->bs->tray_open) { > + media_status = tray_open; > + } else if (bdrv_is_inserted(s->bs)) { > + media_status = media_present; > + } > + > + /* Event notification descriptor */ > + event_code = no_change; > + if (media_status != tray_open && s->events.new_media) { > + event_code = new_media; > + s->events.new_media = false; > + } > + > + buf[4] = event_code; > + buf[5] = media_status; > + > + /* These fields are reserved, just clear them. */ > + buf[6] = 0; > + buf[7] = 0; > + > + return 8; /* We wrote to 4 extra bytes from the header */ Shouldn't you verify that you don't exceed max_len in this? > @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > uint8_t supported_events; > } __attribute((packed)) *gesn_event_header; > > + enum notification_class_request_type { > + reserved1 = 1 << 0, > + operational_change = 1 << 1, > + power_management = 1 << 2, > + external_request = 1 << 3, > + media = 1 << 4, > + multi_host = 1 << 5, > + device_busy = 1 << 6, > + reserved2 = 1 << 7, > + }; > + enum event_notification_class_field { > + enc_no_events = 0, > + enc_operational_change, > + enc_power_management, > + enc_external_request, > + enc_media, > + enc_multiple_hosts, > + enc_device_busy, > + enc_reserved, > + }; More lower-case enums :( Otherwise the series looks good. Jes
On (Tue) 12 Apr 2011 [20:33:39], Amit Shah wrote: > On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote: > > Am 12.04.2011 16:09, schrieb Amit Shah: > > > Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION > > > command. This helps us report tray open, tray closed, no media, media > > > present states to the guest. > > > > > > Newer Linux kernels (2.6.38+) rely on this command to revalidate discs > > > after media change. > > > > > > This patch also sends out tray open/closed status to the guest driver > > > when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). > > > Without such notification, the guest and qemu's tray open/close status > > > was frequently out of sync, causing installers like Anaconda detecting > > > no disc instead of tray open, confusing them terribly. > > > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > > > --- > > > hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > > hw/ide/internal.h | 6 +++ > > > 2 files changed, 110 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/ide/core.c b/hw/ide/core.c > > > index dafc049..209d8e6 100644 > > > --- a/hw/ide/core.c > > > +++ b/hw/ide/core.c > > > @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > > > } > > > } > > > > > > +static unsigned int event_status_media(IDEState *s, > > > + uint8_t *buf, > > > + unsigned int max_len) > > > +{ > > > + enum media_event_code { > > > + no_change = 0, /* Status unchanged */ > > > + eject_requested, /* received a request from user to eject */ > > > + new_media, /* new media inserted and ready for access */ > > > + media_removal, /* only for media changers */ > > > + media_changed, /* only for media changers */ > > > + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > > > + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > > > + }; > > > + enum media_status { > > > + tray_open = 1, > > > + media_present = 2, > > > + }; > > > + uint8_t event_code, media_status; > > > + > > > + media_status = 0; > > > + if (s->bs->tray_open) { > > > + media_status = tray_open; > > > + } else if (bdrv_is_inserted(s->bs)) { > > > + media_status = media_present; > > > + } > > > + > > > + /* Event notification descriptor */ > > > + event_code = no_change; > > > + if (media_status != tray_open && s->events.new_media) { > > > + event_code = new_media; > > > + s->events.new_media = false; > > > + } > > > + > > > + buf[4] = event_code; > > > + buf[5] = media_status; > > > + > > > + /* These fields are reserved, just clear them. */ > > > + buf[6] = 0; > > > + buf[7] = 0; > > > + > > > + return 8; /* We wrote to 4 extra bytes from the header */ > > > +} > > > + > > > static void handle_get_event_status_notification(IDEState *s, > > > uint8_t *buf, > > > const uint8_t *packet) > > > @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > > > uint8_t supported_events; > > > } __attribute((packed)) *gesn_event_header; > > > > > > + enum notification_class_request_type { > > > + reserved1 = 1 << 0, > > > + operational_change = 1 << 1, > > > + power_management = 1 << 2, > > > + external_request = 1 << 3, > > > + media = 1 << 4, > > > + multi_host = 1 << 5, > > > + device_busy = 1 << 6, > > > + reserved2 = 1 << 7, > > > + }; > > > + enum event_notification_class_field { > > > + enc_no_events = 0, > > > + enc_operational_change, > > > + enc_power_management, > > > + enc_external_request, > > > + enc_media, > > > + enc_multiple_hosts, > > > + enc_device_busy, > > > + enc_reserved, > > > + }; > > > unsigned int max_len, used_len; > > > > > > gesn_cdb = (void *)packet; > > > @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, > > > > > > /* polling mode operation */ > > > > > > - /* We don't support any event class (yet). */ > > > - gesn_event_header->supported_events = 0; > > > - > > > - gesn_event_header->notification_class = 0x80; /* No event available */ > > > - used_len = sizeof(*gesn_event_header); > > > + /* > > > + * These are the supported events. > > > + * > > > + * We currently only support requests of the 'media' type. > > > + */ > > > + gesn_event_header->supported_events = media; > > > > > > + /* > > > + * Responses to requests are to be based on request priority. The > > > + * notification_class_request_type enum above specifies the > > > + * priority: upper elements are higher prio than lower ones. > > > + */ > > > + if (gesn_cdb->class & media) { > > > + gesn_event_header->notification_class |= enc_media; > > > > Am I missing where this field is initialized or should it be = instead > > of |= ? > > I considered both: if the 'reserved' bits become available and if we > should write to them, |= makes more sense. Oh; that reminds me: that's why I had the notification_class = 0 initialisation up there. Amit
Am 12.04.2011 17:03, schrieb Amit Shah: > On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote: >> Am 12.04.2011 16:09, schrieb Amit Shah: >>> Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION >>> command. This helps us report tray open, tray closed, no media, media >>> present states to the guest. >>> >>> Newer Linux kernels (2.6.38+) rely on this command to revalidate discs >>> after media change. >>> >>> This patch also sends out tray open/closed status to the guest driver >>> when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). >>> Without such notification, the guest and qemu's tray open/close status >>> was frequently out of sync, causing installers like Anaconda detecting >>> no disc instead of tray open, confusing them terribly. >>> >>> Signed-off-by: Amit Shah <amit.shah@redhat.com> >>> --- >>> hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> hw/ide/internal.h | 6 +++ >>> 2 files changed, 110 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c >>> index dafc049..209d8e6 100644 >>> --- a/hw/ide/core.c >>> +++ b/hw/ide/core.c >>> @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, >>> } >>> } >>> >>> +static unsigned int event_status_media(IDEState *s, >>> + uint8_t *buf, >>> + unsigned int max_len) >>> +{ >>> + enum media_event_code { >>> + no_change = 0, /* Status unchanged */ >>> + eject_requested, /* received a request from user to eject */ >>> + new_media, /* new media inserted and ready for access */ >>> + media_removal, /* only for media changers */ >>> + media_changed, /* only for media changers */ >>> + bg_format_completed, /* MRW or DVD+RW b/g format completed */ >>> + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ >>> + }; >>> + enum media_status { >>> + tray_open = 1, >>> + media_present = 2, >>> + }; >>> + uint8_t event_code, media_status; >>> + >>> + media_status = 0; >>> + if (s->bs->tray_open) { >>> + media_status = tray_open; >>> + } else if (bdrv_is_inserted(s->bs)) { >>> + media_status = media_present; >>> + } >>> + >>> + /* Event notification descriptor */ >>> + event_code = no_change; >>> + if (media_status != tray_open && s->events.new_media) { >>> + event_code = new_media; >>> + s->events.new_media = false; >>> + } >>> + >>> + buf[4] = event_code; >>> + buf[5] = media_status; >>> + >>> + /* These fields are reserved, just clear them. */ >>> + buf[6] = 0; >>> + buf[7] = 0; >>> + >>> + return 8; /* We wrote to 4 extra bytes from the header */ >>> +} >>> + >>> static void handle_get_event_status_notification(IDEState *s, >>> uint8_t *buf, >>> const uint8_t *packet) >>> @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, >>> uint8_t supported_events; >>> } __attribute((packed)) *gesn_event_header; >>> >>> + enum notification_class_request_type { >>> + reserved1 = 1 << 0, >>> + operational_change = 1 << 1, >>> + power_management = 1 << 2, >>> + external_request = 1 << 3, >>> + media = 1 << 4, >>> + multi_host = 1 << 5, >>> + device_busy = 1 << 6, >>> + reserved2 = 1 << 7, >>> + }; >>> + enum event_notification_class_field { >>> + enc_no_events = 0, >>> + enc_operational_change, >>> + enc_power_management, >>> + enc_external_request, >>> + enc_media, >>> + enc_multiple_hosts, >>> + enc_device_busy, >>> + enc_reserved, >>> + }; >>> unsigned int max_len, used_len; >>> >>> gesn_cdb = (void *)packet; >>> @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, >>> >>> /* polling mode operation */ >>> >>> - /* We don't support any event class (yet). */ >>> - gesn_event_header->supported_events = 0; >>> - >>> - gesn_event_header->notification_class = 0x80; /* No event available */ >>> - used_len = sizeof(*gesn_event_header); >>> + /* >>> + * These are the supported events. >>> + * >>> + * We currently only support requests of the 'media' type. >>> + */ >>> + gesn_event_header->supported_events = media; >>> >>> + /* >>> + * Responses to requests are to be based on request priority. The >>> + * notification_class_request_type enum above specifies the >>> + * priority: upper elements are higher prio than lower ones. >>> + */ >>> + if (gesn_cdb->class & media) { >>> + gesn_event_header->notification_class |= enc_media; >> >> Am I missing where this field is initialized or should it be = instead >> of |= ? > > I considered both: if the 'reserved' bits become available and if we > should write to them, |= makes more sense. Currently, = will > suffice. Strong preference? |= is fine if the field is initialized to 0 and you add flags to it. But this patch seems to remove the zero initialization. Maybe it's still there somewhere, I just don't see it right now. Kevin
On 04/12/11 17:13, Kevin Wolf wrote: > Am 12.04.2011 17:03, schrieb Jes Sorensen: >> Shouldn't you verify that you don't exceed max_len in this? > > Not necessary (the buffer is always 2048 bytes), but it looks like the > max_len parameter is unused now, so it could be removed. > > Kevin That works fine too - I just didn't like having a length and then having it ignored completely. Cheers, Jes
Am 12.04.2011 17:03, schrieb Jes Sorensen: > On 04/12/11 16:09, Amit Shah wrote: >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index dafc049..209d8e6 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, >> } >> } >> >> +static unsigned int event_status_media(IDEState *s, >> + uint8_t *buf, >> + unsigned int max_len) >> +{ >> + enum media_event_code { >> + no_change = 0, /* Status unchanged */ >> + eject_requested, /* received a request from user to eject */ >> + new_media, /* new media inserted and ready for access */ >> + media_removal, /* only for media changers */ >> + media_changed, /* only for media changers */ >> + bg_format_completed, /* MRW or DVD+RW b/g format completed */ >> + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ >> + }; >> + enum media_status { >> + tray_open = 1, >> + media_present = 2, >> + }; > > Would be nice if you used upper-case with the enums since they are > effectively just defines, to avoid confusing them with variables. > >> + uint8_t event_code, media_status; >> + >> + media_status = 0; >> + if (s->bs->tray_open) { >> + media_status = tray_open; >> + } else if (bdrv_is_inserted(s->bs)) { >> + media_status = media_present; >> + } >> + >> + /* Event notification descriptor */ >> + event_code = no_change; >> + if (media_status != tray_open && s->events.new_media) { >> + event_code = new_media; >> + s->events.new_media = false; >> + } >> + >> + buf[4] = event_code; >> + buf[5] = media_status; >> + >> + /* These fields are reserved, just clear them. */ >> + buf[6] = 0; >> + buf[7] = 0; >> + >> + return 8; /* We wrote to 4 extra bytes from the header */ > > Shouldn't you verify that you don't exceed max_len in this? Not necessary (the buffer is always 2048 bytes), but it looks like the max_len parameter is unused now, so it could be removed. Kevin
On (Tue) 12 Apr 2011 [17:11:22], Kevin Wolf wrote: > Am 12.04.2011 17:03, schrieb Amit Shah: > > On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote: > >> Am 12.04.2011 16:09, schrieb Amit Shah: > >>> Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION > >>> command. This helps us report tray open, tray closed, no media, media > >>> present states to the guest. > >>> > >>> Newer Linux kernels (2.6.38+) rely on this command to revalidate discs > >>> after media change. > >>> > >>> This patch also sends out tray open/closed status to the guest driver > >>> when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). > >>> Without such notification, the guest and qemu's tray open/close status > >>> was frequently out of sync, causing installers like Anaconda detecting > >>> no disc instead of tray open, confusing them terribly. > >>> > >>> Signed-off-by: Amit Shah <amit.shah@redhat.com> > >>> --- > >>> hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > >>> hw/ide/internal.h | 6 +++ > >>> 2 files changed, 110 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/hw/ide/core.c b/hw/ide/core.c > >>> index dafc049..209d8e6 100644 > >>> --- a/hw/ide/core.c > >>> +++ b/hw/ide/core.c > >>> @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > >>> } > >>> } > >>> > >>> +static unsigned int event_status_media(IDEState *s, > >>> + uint8_t *buf, > >>> + unsigned int max_len) > >>> +{ > >>> + enum media_event_code { > >>> + no_change = 0, /* Status unchanged */ > >>> + eject_requested, /* received a request from user to eject */ > >>> + new_media, /* new media inserted and ready for access */ > >>> + media_removal, /* only for media changers */ > >>> + media_changed, /* only for media changers */ > >>> + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > >>> + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > >>> + }; > >>> + enum media_status { > >>> + tray_open = 1, > >>> + media_present = 2, > >>> + }; > >>> + uint8_t event_code, media_status; > >>> + > >>> + media_status = 0; > >>> + if (s->bs->tray_open) { > >>> + media_status = tray_open; > >>> + } else if (bdrv_is_inserted(s->bs)) { > >>> + media_status = media_present; > >>> + } > >>> + > >>> + /* Event notification descriptor */ > >>> + event_code = no_change; > >>> + if (media_status != tray_open && s->events.new_media) { > >>> + event_code = new_media; > >>> + s->events.new_media = false; > >>> + } > >>> + > >>> + buf[4] = event_code; > >>> + buf[5] = media_status; > >>> + > >>> + /* These fields are reserved, just clear them. */ > >>> + buf[6] = 0; > >>> + buf[7] = 0; > >>> + > >>> + return 8; /* We wrote to 4 extra bytes from the header */ > >>> +} > >>> + > >>> static void handle_get_event_status_notification(IDEState *s, > >>> uint8_t *buf, > >>> const uint8_t *packet) > >>> @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > >>> uint8_t supported_events; > >>> } __attribute((packed)) *gesn_event_header; > >>> > >>> + enum notification_class_request_type { > >>> + reserved1 = 1 << 0, > >>> + operational_change = 1 << 1, > >>> + power_management = 1 << 2, > >>> + external_request = 1 << 3, > >>> + media = 1 << 4, > >>> + multi_host = 1 << 5, > >>> + device_busy = 1 << 6, > >>> + reserved2 = 1 << 7, > >>> + }; > >>> + enum event_notification_class_field { > >>> + enc_no_events = 0, > >>> + enc_operational_change, > >>> + enc_power_management, > >>> + enc_external_request, > >>> + enc_media, > >>> + enc_multiple_hosts, > >>> + enc_device_busy, > >>> + enc_reserved, > >>> + }; > >>> unsigned int max_len, used_len; > >>> > >>> gesn_cdb = (void *)packet; > >>> @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, > >>> > >>> /* polling mode operation */ > >>> > >>> - /* We don't support any event class (yet). */ > >>> - gesn_event_header->supported_events = 0; > >>> - > >>> - gesn_event_header->notification_class = 0x80; /* No event available */ > >>> - used_len = sizeof(*gesn_event_header); > >>> + /* > >>> + * These are the supported events. > >>> + * > >>> + * We currently only support requests of the 'media' type. > >>> + */ > >>> + gesn_event_header->supported_events = media; > >>> > >>> + /* > >>> + * Responses to requests are to be based on request priority. The > >>> + * notification_class_request_type enum above specifies the > >>> + * priority: upper elements are higher prio than lower ones. > >>> + */ > >>> + if (gesn_cdb->class & media) { > >>> + gesn_event_header->notification_class |= enc_media; > >> > >> Am I missing where this field is initialized or should it be = instead > >> of |= ? > > > > I considered both: if the 'reserved' bits become available and if we > > should write to them, |= makes more sense. Currently, = will > > suffice. Strong preference? > > |= is fine if the field is initialized to 0 and you add flags to it. But > this patch seems to remove the zero initialization. Maybe it's still > there somewhere, I just don't see it right now. Yes, I didn't think about it when you asked to remove the 0 init, but got reminded after I saw this comment. I'll do a v5 with the init back in. Amit
diff --git a/hw/ide/core.c b/hw/ide/core.c index dafc049..209d8e6 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, } } +static unsigned int event_status_media(IDEState *s, + uint8_t *buf, + unsigned int max_len) +{ + enum media_event_code { + no_change = 0, /* Status unchanged */ + eject_requested, /* received a request from user to eject */ + new_media, /* new media inserted and ready for access */ + media_removal, /* only for media changers */ + media_changed, /* only for media changers */ + bg_format_completed, /* MRW or DVD+RW b/g format completed */ + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ + }; + enum media_status { + tray_open = 1, + media_present = 2, + }; + uint8_t event_code, media_status; + + media_status = 0; + if (s->bs->tray_open) { + media_status = tray_open; + } else if (bdrv_is_inserted(s->bs)) { + media_status = media_present; + } + + /* Event notification descriptor */ + event_code = no_change; + if (media_status != tray_open && s->events.new_media) { + event_code = new_media; + s->events.new_media = false; + } + + buf[4] = event_code; + buf[5] = media_status; + + /* These fields are reserved, just clear them. */ + buf[6] = 0; + buf[7] = 0; + + return 8; /* We wrote to 4 extra bytes from the header */ +} + static void handle_get_event_status_notification(IDEState *s, uint8_t *buf, const uint8_t *packet) @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, uint8_t supported_events; } __attribute((packed)) *gesn_event_header; + enum notification_class_request_type { + reserved1 = 1 << 0, + operational_change = 1 << 1, + power_management = 1 << 2, + external_request = 1 << 3, + media = 1 << 4, + multi_host = 1 << 5, + device_busy = 1 << 6, + reserved2 = 1 << 7, + }; + enum event_notification_class_field { + enc_no_events = 0, + enc_operational_change, + enc_power_management, + enc_external_request, + enc_media, + enc_multiple_hosts, + enc_device_busy, + enc_reserved, + }; unsigned int max_len, used_len; gesn_cdb = (void *)packet; @@ -1121,12 +1184,25 @@ static void handle_get_event_status_notification(IDEState *s, /* polling mode operation */ - /* We don't support any event class (yet). */ - gesn_event_header->supported_events = 0; - - gesn_event_header->notification_class = 0x80; /* No event available */ - used_len = sizeof(*gesn_event_header); + /* + * These are the supported events. + * + * We currently only support requests of the 'media' type. + */ + gesn_event_header->supported_events = media; + /* + * Responses to requests are to be based on request priority. The + * notification_class_request_type enum above specifies the + * priority: upper elements are higher prio than lower ones. + */ + if (gesn_cdb->class & media) { + gesn_event_header->notification_class |= enc_media; + used_len = event_status_media(s, buf, max_len); + } else { + gesn_event_header->notification_class = 0x80; /* No event available */ + used_len = sizeof(*gesn_event_header); + } gesn_event_header->len = cpu_to_be16(used_len - sizeof(*gesn_event_header)); ide_atapi_cmd_reply(s, used_len, max_len); @@ -1656,6 +1732,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->events.new_media = true; ide_set_irq(s->bus); } @@ -2800,6 +2877,25 @@ static bool ide_drive_pio_state_needed(void *opaque) return (s->status & DRQ_STAT) != 0; } +static bool ide_atapi_gesn_needed(void *opaque) +{ + IDEState *s = opaque; + + return s->events.new_media || s->events.eject_request; +} + +/* Fields for GET_EVENT_STATUS_NOTIFICATION ATAPI command */ +const VMStateDescription vmstate_ide_atapi_gesn_state = { + .name ="ide_drive/atapi/gesn_state", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField []) { + VMSTATE_BOOL(events.new_media, IDEState), + VMSTATE_BOOL(events.eject_request, IDEState), + } +}; + const VMStateDescription vmstate_ide_drive_pio_state = { .name = "ide_drive/pio_state", .version_id = 1, @@ -2854,6 +2950,9 @@ const VMStateDescription vmstate_ide_drive = { .vmsd = &vmstate_ide_drive_pio_state, .needed = ide_drive_pio_state_needed, }, { + .vmsd = &vmstate_ide_atapi_gesn_state, + .needed = ide_atapi_gesn_needed, + }, { /* empty */ } } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index d533fb6..ba7e9a8 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -373,6 +373,11 @@ typedef int DMAFunc(IDEDMA *); typedef int DMAIntFunc(IDEDMA *, int); typedef void DMARestartFunc(void *, int, int); +struct unreported_events { + bool eject_request; + bool new_media; +}; + /* NOTE: IDEState represents in fact one drive */ struct IDEState { IDEBus *bus; @@ -408,6 +413,7 @@ struct IDEState { BlockDriverState *bs; char version[9]; /* ATAPI specific */ + struct unreported_events events; uint8_t sense_key; uint8_t asc; uint8_t cdrom_changed;
Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION command. This helps us report tray open, tray closed, no media, media present states to the guest. Newer Linux kernels (2.6.38+) rely on this command to revalidate discs after media change. This patch also sends out tray open/closed status to the guest driver when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus). Without such notification, the guest and qemu's tray open/close status was frequently out of sync, causing installers like Anaconda detecting no disc instead of tray open, confusing them terribly. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)