atapi: Implement 'media' subcommand for GESN
v5 cover atapi: Implement 'media' subcommand for GESN
The GET_EVENT_STATUS_NOTIFICATION ATAPI command is listed as a mandatory command in the spec but we don't really implement it any of its sub-commands.
The commit message for the last commit explains why implementing just the media subcommand is helpful and how it goes a long way in getting guests to behave as expected.
The difference from the RFC series sent earlier is:
- Split into more patches
- Add tray open/close notification (from Markus)
There certainly is much more work to be done for the other commands and also for state change handling (tray open / close / new media) overall for the block layer, but this is a good first step in being spec-compliant and at the same time making guests work.
v5:
- re-add initialisation of gesn_event_header->notification_class; it is a bug to not init it; add comment.
- remove max_len param from call to event_status_media()
- convert enums to upper case.
v4:
- gesn_event_header should point to buf
- compile fix for patch 3/5
- remove initialisation without effect of a variable.
v3:
- Add gesn_event_header struct, further removing a few constants used
- Set reserved bits to 0 for the media subcommand
- Remove the function handling NEA, move to generic code
- Re-do patch series to reflect above change
- Merge vmstate patches with patch introducing new fields
- Merge fixes for other comments by Kevin
v2:
- Update comments
- Use struct instead of enum for cdb packet
- Add a new subsection to vmstate for new fields for save/restore
v1:
- Split into more patches
- Add tray open/close notification (from Markus)
RFC:
- Orig. series
Amit Shah (5):
atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
atapi: Move GET_EVENT_STATUS_NOTIFICATION command handling to its own
function
atapi: GESN: Use structs for commonly-used field types
atapi: GESN: Standardise event response handling for future additions
atapi: GESN: implement 'media' subcommand
hw/ide/core.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++------ hw/ide/internal.h | 6 ++ 2 files changed, 174 insertions(+), 19 deletions(-)
Signature · 1 line
1.7.4.2
PATCH v5 1/5 atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
After a media change, the only commands allowed from the guest were REQUEST_SENSE and INQUIRY. The guest may also issue GET_EVENT_STATUS_NOTIFICATION commands to get media changed notification.
--- hw/ide/core.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-)
hw/ide/core.c +14 -6
@@ -1102,13 +1102,21 @@ static void ide_atapi_cmd(IDEState *s) printf("\n"); }#endif -/* If there's a UNIT_ATTENTION condition pending, only -REQUEST_SENSE and INQUIRY commands are allowed to complete. */ +/* +* If there's a UNIT_ATTENTION condition pending, only +* REQUEST_SENSE, INQUIRY, GET_CONFIGURATION and +* GET_EVENT_STATUS_NOTIFICATION commands are allowed to complete. +* MMC-5, section 4.1.6.1 lists only these commands being allowed +* to complete, with other commands getting a CHECK condition +* response unless a higher priority status, defined by the drive +* here, is pending. +*/ 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_GET_EVENT_STATUS_NOTIFICATION ) { +ide_atapi_cmd_check_status (s); +return ; }switch (s->io_buffer[0 ]) {case GPCMD_TEST_UNIT_READY :
Signature · 1 line
1.7.4.2
PATCH v5 2/5 atapi: Move GET_EVENT_STATUS_NOTIFICATION command handling to its own function
In reply to Amit ShahThis makes the code more readable.
Also, there's a block like:
if () {
...
} else {
...
}
Split that into
if () {
...
return;
}
...
--- hw/ide/core.c | 37 ++++++++++++++++++++++++------------- 1 files changed, 24 insertions(+), 13 deletions(-)
hw/ide/core.c +24 -13
@@ -1084,6 +1084,29 @@ static int ide_dvd_read_structure(IDEState *s, int format, } } +static void handle_get_event_status_notification (IDEState *s, +uint8_t *buf, +const uint8_t *packet) +{ +unsigned int max_len; + + max_len =ube16_to_cpu (packet +7 ); + +if (!(packet[1 ] &0x01 )) {/* asynchronous mode */ +/* Only polling is supported, asynchronous mode is not. */ +ide_atapi_cmd_error (s,SENSE_ILLEGAL_REQUEST , +ASC_INV_FIELD_IN_CMD_PACKET ); +return ; + } + +/* polling */ +/* We don't support any event class (yet). */ +cpu_to_ube16 (buf,0x00 );/* No event descriptor returned */ + buf[2 ] =0x80 ;/* No Event Available (NEA) */ + buf[3 ] =0x00 ;/* Empty supported event classes */ +ide_atapi_cmd_reply (s,4 , max_len); +} +static void ide_atapi_cmd (IDEState *s) {const uint8_t *packet;
@@ -1530,19 +1553,7 @@ static void ide_atapi_cmd(IDEState *s)break ; }case GPCMD_GET_EVENT_STATUS_NOTIFICATION : - max_len =ube16_to_cpu (packet +7 ); - -if (packet[1 ] &0x01 ) {/* polling */ -/* We don't support any event class (yet). */ -cpu_to_ube16 (buf,0x00 );/* No event descriptor returned */ - buf[2 ] =0x80 ;/* No Event Available (NEA) */ - buf[3 ] =0x00 ;/* Empty supported event classes */ -ide_atapi_cmd_reply (s,4 , max_len); - }else {/* asynchronous mode */ -/* Only polling is supported, asynchronous mode is not. */ -ide_atapi_cmd_error (s,SENSE_ILLEGAL_REQUEST , -ASC_INV_FIELD_IN_CMD_PACKET ); - } +handle_get_event_status_notification (s, buf, packet);break ;default : ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
Signature · 1 line
1.7.4.2
PATCH v5 3/5 atapi: GESN: Use structs for commonly-used field types
In reply to Amit ShahInstead of using magic numbers, use structs that are more descriptive of the fields being used.
--- hw/ide/core.c | 16 ++++++++++++++-- 1 files changed, 14 insertions(+), 2 deletions(-)
hw/ide/core.c +14 -2
@@ -1088,11 +1088,23 @@ static void handle_get_event_status_notification(IDEState *s, uint8_t *buf, const uint8_t *packet) { +struct { +uint8_t opcode; +uint8_t polled;/* lsb bit is polled; others are reserved */ +uint8_t reserved2[2 ]; +uint8_t class; +uint8_t reserved3[2 ]; +uint16_t len; +uint8_t control; + } __attribute__((packed)) *gesn_cdb; +unsigned int max_len; - max_len =ube16_to_cpu (packet +7 ); + gesn_cdb = (void *)packet; + max_len =be16_to_cpu (gesn_cdb->len); -if (!(packet[1 ] &0x01 )) {/* asynchronous mode */ +/* It is fine by the MMC spec to not support async mode operations */ +if (!(gesn_cdb->polled &0x01 )) {/* asynchronous mode */ /* Only polling is supported, asynchronous mode is not. */ ide_atapi_cmd_error (s,SENSE_ILLEGAL_REQUEST ,ASC_INV_FIELD_IN_CMD_PACKET );
Signature · 1 line
1.7.4.2
PATCH v5 4/5 atapi: GESN: Standardise event response handling for future additions
In reply to Amit ShahHandle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a generic way so that future additions to the code to handle other response types is easier.
--- hw/ide/core.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-)
hw/ide/core.c +19 -6
@@ -1098,9 +1098,17 @@ static void handle_get_event_status_notification(IDEState *s,uint8_t control; } __attribute__((packed)) *gesn_cdb; -unsigned int max_len; +struct { +uint16_t len; +uint8_t notification_class; +uint8_t supported_events; + } __attribute((packed)) *gesn_event_header; + +unsigned int max_len, used_len; gesn_cdb = (void *)packet; + gesn_event_header = (void *)buf; + max_len =be16_to_cpu (gesn_cdb->len);/* It is fine by the MMC spec to not support async mode operations */
@@ -1111,12 +1119,17 @@ static void handle_get_event_status_notification(IDEState *s,return ; } -/* polling */ +/* polling mode operation */ +/* We don't support any event class (yet). */ -cpu_to_ube16 (buf,0x00 );/* No event descriptor returned */ - buf[2 ] =0x80 ;/* No Event Available (NEA) */ - buf[3 ] =0x00 ;/* Empty supported event classes */ -ide_atapi_cmd_reply (s,4 , max_len); + gesn_event_header->supported_events =0 ; + + 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); }static void ide_atapi_cmd (IDEState *s)
Signature · 1 line
1.7.4.2
PATCH v5 5/5 atapi: GESN: implement 'media' subcommand
In reply to Amit ShahImplement 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.
--- hw/ide/core.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 115 insertions(+), 4 deletions(-)
hw/ide/core.c +109 -4
@@ -1084,6 +1084,48 @@ static int ide_dvd_read_structure(IDEState *s, int format, } } +static unsigned int event_status_media (IDEState *s, +uint8_t *buf) +{ +enum media_event_code { +MEC_NO_CHANGE =0 ,/* Status unchanged */ +MEC_EJECT_REQUESTED ,/* received a request from user to eject */ +MEC_NEW_MEDIA ,/* new media inserted and ready for access */ +MEC_MEDIA_REMOVAL ,/* only for media changers */ +MEC_MEDIA_CHANGED ,/* only for media changers */ +MEC_BG_FORMAT_COMPLETED ,/* MRW or DVD+RW b/g format completed */ +MEC_BG_FORMAT_RESTARTED ,/* MRW or DVD+RW b/g format restarted */ + }; +enum media_status { +MS_TRAY_OPEN =1 , +MS_MEDIA_PRESENT =2 , + }; +uint8_t event_code, media_status; + + media_status =0 ; +if (s->bs->tray_open) { + media_status =MS_TRAY_OPEN ; + }else if (bdrv_is_inserted (s->bs)) { + media_status =MS_MEDIA_PRESENT ; + } + +/* Event notification descriptor */ + event_code =MEC_NO_CHANGE ; +if (media_status !=MS_TRAY_OPEN && s->events.new_media) { + event_code =MEC_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 +1146,26 @@ static void handle_get_event_status_notification(IDEState *s,uint8_t supported_events; } __attribute((packed)) *gesn_event_header; +enum notification_class_request_type { +NCR_RESERVED1 =1 <<0 , +NCR_OPERATIONAL_CHANGE =1 <<1 , +NCR_POWER_MANAGEMENT =1 <<2 , +NCR_EXTERNAL_REQUEST =1 <<3 , +NCR_MEDIA =1 <<4 , +NCR_MULTI_HOST =1 <<5 , +NCR_DEVICE_BUSY =1 <<6 , +NCR_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 +1183,32 @@ 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 ; +/* +* These are the supported events. +* +* We currently only support requests of the 'media' type. +*/ + gesn_event_header->supported_events =NCR_MEDIA ; - gesn_event_header->notification_class =0x80 ;/* No event available */ - used_len =sizeof (*gesn_event_header); +/* +* We use |= below to set the class field; other bits in this byte +* are reserved now but this is useful to do if we have to use the +* reserved fields later. +*/ + gesn_event_header->notification_class =0 ; +/* +* 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 &NCR_MEDIA ) { + gesn_event_header->notification_class |=ENC_MEDIA ; + used_len =event_status_media (s, buf); + }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 +1738,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 +2883,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 +2956,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 */ } }
hw/ide/internal.h +6 -0
@@ -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;
Signature · 1 line
1.7.4.2
Quoted 8 lines · Amit Shah
The
GET_EVENT_STATUS_NOTIFICATIONATAPI command is listed as a mandatory command in the spec but we don't really implement it any of its sub-commands.The commit message for the last commit explains why implementing just the media subcommand is helpful and how it goes a long way in getting guests to behave as expected.
Quoted 9 lines
The difference from the RFC series sent earlier is:
- Split into more patches
- Add tray open/close notification (from Markus)There certainly is much more work to be done for the other commands and also for state change handling (tray open / close / new media) overall for the block layer, but this is a good first step in being spec-compliant and at the same time making guests work.
v5 looks good.
Jes
Quoted 56 lines · Amit Shah
The
GET_EVENT_STATUS_NOTIFICATIONATAPI command is listed as a mandatory command in the spec but we don't really implement it any of its sub-commands.The commit message for the last commit explains why implementing just the media subcommand is helpful and how it goes a long way in getting guests to behave as expected.
The difference from the RFC series sent earlier is:
- Split into more patches
- Add tray open/close notification (from Markus)There certainly is much more work to be done for the other commands and also for state change handling (tray open / close / new media) overall for the block layer, but this is a good first step in being spec-compliant and at the same time making guests work.
v5:
- re-add initialisation ofgesn_event_header->notification_class; it is a bug to not init it; add comment.
- removemax_lenparam from call toevent_status_media()
- convert enums to upper case.v4:
-gesn_event_headershould point to buf
- compile fix for patch 3/5
- remove initialisation without effect of a variable.v3:
- Addgesn_event_headerstruct, further removing a few constants used
- Set reserved bits to 0 for the media subcommand
- Remove the function handling NEA, move to generic code
- Re-do patch series to reflect above change
- Merge vmstate patches with patch introducing new fields
- Merge fixes for other comments by Kevinv2:
- Update comments
- Use struct instead of enum for cdb packet
- Add a new subsection to vmstate for new fields for save/restorev1:
- Split into more patches
- Add tray open/close notification (from Markus)RFC:
- Orig. seriesAmit Shah (5):
atapi: AllowGET_EVENT_STATUS_NOTIFICATIONafter media change
atapi: MoveGET_EVENT_STATUS_NOTIFICATIONcommand handling to its own
function
atapi: GESN: Use structs for commonly-used field types
atapi: GESN: Standardise event response handling for future additions
atapi: GESN: implement 'media' subcommand
Quoted 3 lines
hw/ide/core.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++------ hw/ide/internal.h | 6 ++ 2 files changed, 174 insertions(+), 19 deletions(-)
ACK
Please consider moving structs and enums to a common header file (mmc.h or atapi.h) and naming them to avoid void* casts in a followup.
Paolo
Kevin Wolf
AppliedAm 12.04.2011 18:06, schrieb Amit Shah:
Quoted 8 lines
The
GET_EVENT_STATUS_NOTIFICATIONATAPI command is listed as a mandatory command in the spec but we don't really implement it any of its sub-commands.The commit message for the last commit explains why implementing just the media subcommand is helpful and how it goes a long way in getting guests to behave as expected.
Quoted 8 lines
The difference from the RFC series sent earlier is:
- Split into more patches
- Add tray open/close notification (from Markus)There certainly is much more work to be done for the other commands and also for state change handling (tray open / close / new media) overall for the block layer, but this is a good first step in being spec-compliant and at the same time making guests work.
Thanks, applied all to the block branch.
Kevin