On (Fri) 08 Apr 2011 [18:17:38], Kevin Wolf wrote: > Am 08.04.2011 09:15, 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.s...@redhat.com> > > --- > > hw/ide/core.c | 93 > > +++++++++++++++++++++++++++++++++++++++++++++++++++- > > hw/ide/internal.h | 6 +++ > > 2 files changed, 97 insertions(+), 2 deletions(-) > > > > diff --git a/hw/ide/core.c b/hw/ide/core.c > > index cdc2c56..627b2cf 100644 > > --- a/hw/ide/core.c > > +++ b/hw/ide/core.c > > @@ -1084,6 +1084,57 @@ 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; > > + > > + if (max_len < 6) { > > + /* We're going to use 6 bytes; don't report anything if we have > > less */ > > + return 0; > > + } > > Why? Shouldn't we report the first few bytes?
This shouldn't be necessary at all now, isn't it? > > + > > + /* Event notification header */ > > + cpu_to_ube16(buf, max_len); > > + buf[2] = event_class; > > + buf[3] = supported_events; > > This should be done in handle_get_event_status_notification as it's not > specific for media events. Yes, I moved it here just before the submission. With the used_len stuff not being needed anymore, this can be put back there and code won't look too ugly. > > + 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 */ > > 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. > So I think we should return 8 and possible zero bytes 6 and 7. > > > +} > > + > > static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len) > > { > > unsigned int used_len; > > @@ -1125,7 +1176,28 @@ static void > > handle_get_event_status_notification(IDEState *s, > > allocation_length_lsb = 8, > > control = 9, > > }; > > + 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; > > > > max_len = ube16_to_cpu(packet + allocation_length_msb); > > > > @@ -1139,8 +1211,24 @@ 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; > > + > > + /* > > + * 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 (packet[request] & media) { > > + used_len = event_status_media(s, buf, max_len, enc_media, > > + supported_events); > > + } else { > > + used_len = event_status_nea(buf, max_len); > > + } > > Once you put the event header here, there's no need for event_status_nea > any more because it just mean not adding any descriptors. That will work > much better as soon as we have more even classes, too. Hm, I didn't do it that way when I first coded this so there must've been something that I'm forgetting now. > > > ide_atapi_cmd_reply(s, used_len, max_len); > > } > > > > @@ -1661,6 +1749,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; > > Do we need to add some vmstate fields? Yes! I had it in mind till sometime back. Amit