atapi: Implement 'media' subcommand for GESN
v4 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.
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 | 181 +++++++++++++++++++++++++++++++++++++++++++++++------ hw/ide/internal.h | 6 ++ 2 files changed, 168 insertions(+), 19 deletions(-)
Signature · 1 line
1.7.4.2
PATCH v4 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 v4 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 v4 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 v4 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 v4 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 | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)
hw/ide/core.c +104 -5
@@ -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 */ } }
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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Am 12.04.2011 16:09, schrieb Amit Shah:
Quoted 113 lines
Implement the 'media' sub-command of the
GET_EVENT_STATUS_NOTIFICATIONcommand. 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_STATUSioctl (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 <[email protected]>--- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)hw/ide/core.c +67
-5@@ -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.
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1200
Quoted 10 lines
hw/ide/core.c +9
-0+*/ + 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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Quoted 117 lines · Kevin Wolf
Am 12.04.2011 16:09, schrieb Amit Shah:
Quoted 116 lines
Implement the 'media' sub-command of the
GET_EVENT_STATUS_NOTIFICATIONcommand. 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_STATUSioctl (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 <[email protected]>--- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)hw/ide/core.c +69
-5@@ -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;
Quoted 10 lines
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1200
Quoted 7 lines
hw/ide/core.c +7
-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 & 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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Quoted 129 lines · Amit Shah
Quoted 117 lines · Kevin Wolf
Am 12.04.2011 16:09, schrieb Amit Shah:
Quoted 116 lines
Implement the 'media' sub-command of the
GET_EVENT_STATUS_NOTIFICATIONcommand. 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_STATUSioctl (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 <[email protected]>--- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)hw/ide/core.c +69
-5@@ -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;Quoted 10 lines
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1200
Quoted 7 lines
hw/ide/core.c +7
-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 & media) { + gesn_event_header->notification_class |= enc_media;Am I missing where this field is initialized or should it be = instead of |= ?
Quoted 2 lines
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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
In reply to Amit ShahAm 12.04.2011 17:03, schrieb Amit Shah:
Quoted 129 lines · Kevin Wolf
Quoted 117 lines
Am 12.04.2011 16:09, schrieb Amit Shah:
Quoted 116 lines
Implement the 'media' sub-command of the
GET_EVENT_STATUS_NOTIFICATIONcommand. 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_STATUSioctl (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 <[email protected]>--- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)hw/ide/core.c +69
-5@@ -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;Quoted 10 lines
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1200
Quoted 7 lines
hw/ide/core.c +7
-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 & media) { + gesn_event_header->notification_class |= enc_media;Am I missing where this field is initialized or should it be = instead of |= ?
Quoted 3 lines
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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Quoted 130 lines · Kevin Wolf
Am 12.04.2011 17:03, schrieb Amit Shah:
Quoted 129 lines · Kevin Wolf
Quoted 127 lines
Am 12.04.2011 16:09, schrieb Amit Shah:
Quoted 113 lines
Implement the 'media' sub-command of the
GET_EVENT_STATUS_NOTIFICATIONcommand. 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_STATUSioctl (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 <[email protected]>--- hw/ide/core.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/ide/internal.h | 6 +++ 2 files changed, 110 insertions(+), 5 deletions(-)hw/ide/core.c +67
-5@@ -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. hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1200
Quoted 10 lines
hw/ide/core.c +9
-0+*/ + 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 |= ?
Quoted 7 lines
Quoted 3 lines
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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
In reply to Amit ShahQuoted 15 lines · Amit Shah
hw/ide/core.c +7
-0@@ -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 */
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1103
Quoted 10 lines
hw/ide/core.c +10
-0+ 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.
Quoted 14 lines
hw/ide/core.c +14
-0+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;
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1127
Quoted 10 lines
hw/ide/core.c +10
-0+ } + + 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?
Quoted 14 lines
hw/ide/core.c +10
-0@@ -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 , + };
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1169
Quoted 10 lines
hw/ide/core.c +10
-0+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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Am 12.04.2011 17:03, schrieb Jes Sorensen:
Quoted 48 lines · Amit Shah
Quoted 15 lines
hw/ide/core.c +7
-0@@ -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 */ hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1103
Quoted 10 lines
hw/ide/core.c +10
-0+ 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.
Quoted 18 lines
hw/ide/core.c +18
-0+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;
Quoted 8 lines
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1127
Quoted 6 lines
hw/ide/core.c +6
-0+ +/* 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_lenin 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
Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand
Quoted 7 lines · Kevin Wolf
Am 12.04.2011 17:03, schrieb Jes Sorensen:
Quoted 1 line
Shouldn't you verify that you don't exceed
max_lenin this?Not necessary (the buffer is always 2048 bytes), but it looks like the
max_lenparameter 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