Message ID | d010a0d43ee724e74cb7136cff03349af9af478f.1302600061.git.amit.shah@redhat.com |
---|---|
State | New |
Headers | show |
Am 12.04.2011 11:27, 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 | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++- > hw/ide/internal.h | 6 +++ > 2 files changed, 96 insertions(+), 2 deletions(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index fe50d8a..2683070 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1084,6 +1084,47 @@ 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, > + unsigned int event_class, > + unsigned int supported_events) > +{ > + 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; > + > + return 6; /* We wrote to just 2 extra bytes from the header */ I must admit that I don't understand your answer you gave on v1 here. Let me quote: >> > After media_state, there are two more fields for start/end slot (even >> > though they are reserved because we don't have a multiple slot device) > Yes, they're reserved, so we shouldn't change them. Any change might > trigger bad response from guests. I'm not sure what we would be _changing_. We're building a response structure here, not modifying existing data. So what your code does is to receive a short response that leaves out the reserved fields. I don't think this is how it's supposed to work. We should include the reserved fields in the descriptor length and zero them (MMC-5, section 3.6.8): "“Reserved” is a keyword referring to bits, bytes, words, fields and code values that are set aside for future standardization. A reserved bit, byte, word or field shall be set to zero, or in accordance with a future extension to this standard." > +} > + > static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len) > { > cpu_to_ube16(buf, 0x00); /* No event descriptor returned */ > @@ -1107,7 +1148,28 @@ static void handle_get_event_status_notification(IDEState *s, > uint8_t len_lsb; > uint8_t control; > } __attribute__((packed)) *gesn_cdb; > + 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; > + unsigned int supported_events; > > gesn_cdb = (void *)packet; > max_len = ube16_to_cpu(&gesn_cdb->len_msb); > @@ -1122,8 +1184,33 @@ static void handle_get_event_status_notification(IDEState *s, > > /* polling mode operation */ > > - /* We don't support any event class (yet). */ > - used_len = event_status_nea(buf, max_len); > + /* > + * These are the supported events. > + * > + * We currently only support requests of the 'media' type. > + */ > + supported_events = media; > + > + /* > + * Event notification header; will be overwritten by the > + * NO_EVENT_AVAILABLE code if we don't have events: according to > + * MMC-5 6.7.2.2, if nea = 1, event class field should be 0. > + */ > + cpu_to_ube16(buf, max_len); That doesn't look right. I think it needs to be used_len - sizeof(header). > + buf[2] = event_class; > + buf[3] = supported_events; The spec doesn't talk about "event class" for buf[2], but about "notification class" (same applies for the comment above). We should stick to that as buf[3] is called "supported event class", so they might be confused. > + > + /* > + * 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->request & media) { > + used_len = event_status_media(s, buf, max_len, enc_media, > + supported_events); > + } else { > + used_len = event_status_nea(buf, max_len); This expands to: cpu_to_ube16(buf, 0x00); /* No event descriptor returned */ buf[2] = 0x80; /* No Event Available (NEA) */ buf[3] = 0x00; /* Empty supported event classes */ The first one is covered if you fix what I mentioned above (used_len - sizeof(header)). The third one is wrong, we still support the media class. So what is left is one line: buf[2] = 0x80; /* No Event Available (NEA) */ I'm still not convinced that having event_status_nea() as a separate function makes a lot of sense. > + } > ide_atapi_cmd_reply(s, used_len, max_len); > } > > @@ -1650,6 +1737,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); > } > > 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; Kevin
diff --git a/hw/ide/core.c b/hw/ide/core.c index fe50d8a..2683070 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1084,6 +1084,47 @@ 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, + unsigned int event_class, + unsigned int supported_events) +{ + 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; + + return 6; /* We wrote to just 2 extra bytes from the header */ +} + static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len) { cpu_to_ube16(buf, 0x00); /* No event descriptor returned */ @@ -1107,7 +1148,28 @@ static void handle_get_event_status_notification(IDEState *s, uint8_t len_lsb; uint8_t control; } __attribute__((packed)) *gesn_cdb; + 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; + unsigned int supported_events; gesn_cdb = (void *)packet; max_len = ube16_to_cpu(&gesn_cdb->len_msb); @@ -1122,8 +1184,33 @@ static void handle_get_event_status_notification(IDEState *s, /* polling mode operation */ - /* We don't support any event class (yet). */ - used_len = event_status_nea(buf, max_len); + /* + * These are the supported events. + * + * We currently only support requests of the 'media' type. + */ + supported_events = media; + + /* + * Event notification header; will be overwritten by the + * NO_EVENT_AVAILABLE code if we don't have events: according to + * MMC-5 6.7.2.2, if nea = 1, event class field should be 0. + */ + cpu_to_ube16(buf, max_len); + buf[2] = event_class; + buf[3] = supported_events; + + /* + * 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->request & media) { + used_len = event_status_media(s, buf, max_len, enc_media, + supported_events); + } else { + used_len = event_status_nea(buf, max_len); + } ide_atapi_cmd_reply(s, used_len, max_len); } @@ -1650,6 +1737,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); } 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 | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++- hw/ide/internal.h | 6 +++ 2 files changed, 96 insertions(+), 2 deletions(-)