atapi: Implement 'media' subcommand for GESN
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.
Please apply!
Amit Shah (5):
atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
ide: Move GET_EVENT_STATUS_NOTIFICATION command handling to its own
function
atapi: GESN: Spin off No Event Available handling into own function
atapi: GESN: Add enums for commonly-used field types
atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION
command
hw/ide/core.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++----- hw/ide/internal.h | 6 ++ 2 files changed, 161 insertions(+), 17 deletions(-)
Signature · 1 line
1.7.4.2
PATCH 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.
After this, the HSM violation messages from Linux guests aren't seen.
--- hw/ide/core.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
hw/ide/core.c +5 -4
@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)/* If there's a UNIT_ATTENTION condition pending, only REQUEST_SENSE and INQUIRY commands are allowed to complete. */ 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
[Qemu-devel] Re: [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
Quoted 6 lines · Amit Shah
After a media change, the only commands allowed from the guest were
REQUEST_SENSEand INQUIRY. The guest may also issueGET_EVENT_STATUS_NOTIFICATIONcommands to get media changed notification.After this, the HSM violation messages from Linux guests aren't seen.
This isn't true -- the messages go only if TEST_UNIT_READY is also added to the list. Removed this statement from local copy.
hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1106
Quoted 7 lines
hw/ide/core.c +0
-0@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)/* If there's a UNIT_ATTENTION condition pending, only REQUEST_SENSE and INQUIRY commands are allowed to complete. */
And updated this comment locally as well.
Amit
Re: [Qemu-devel] [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
In reply to Amit ShahQuoted 17 lines · Amit Shah
After a media change, the only commands allowed from the guest were
REQUEST_SENSEand INQUIRY. The guest may also issueGET_EVENT_STATUS_NOTIFICATIONcommands to get media changed notification.After this, the HSM violation messages from Linux guests aren't seen.
Signed-off-by Amit Shah <[email protected]>--- hw/ide/core.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)hw/ide/core.c +0
-0
hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1106
Quoted 2 lines
hw/ide/core.c +0
-0@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)/* If there's a UNIT_ATTENTION condition pending, only REQUEST_SENSE and INQUIRY commands are allowed to complete. */
Comment is now stale. I doubt it's terribly useful for anyone capable of reading C, but as long as it's there, it better be accurate.
Quoted 13 lines
hw/ide/core.c +5
-4if (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 :
Re: [Qemu-devel] [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change
Am 08.04.2011 12:54, schrieb Markus Armbruster:
Quoted 14 lines
Quoted 12 lines · Amit Shah
After a media change, the only commands allowed from the guest were
REQUEST_SENSEand INQUIRY. The guest may also issueGET_EVENT_STATUS_NOTIFICATIONcommands to get media changed notification.After this, the HSM violation messages from Linux guests aren't seen.
Signed-off-by Amit Shah <[email protected]>--- hw/ide/core.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
Quoted 10 lines
hw/ide/core.c static void ide_atapi_cmd(IDEState *s) · line 1106
Quoted 7 lines
hw/ide/core.c +0
-0@@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)/* If there's a UNIT_ATTENTION condition pending, only REQUEST_SENSE and INQUIRY commands are allowed to complete. */ Comment is now stale. I doubt it's terribly useful for anyone capable of reading C, but as long as it's there, it better be accurate.
You can make it useful by pointing to the right section in MMC (and listing the commands that should be allowed according to the standard as long as it differs from what we implement today).
Kevin
PATCH 2/5 ide: 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;
@@ -1523,19 +1546,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 3/5 atapi: GESN: Spin off No Event Available handling into own function
In reply to Amit ShahHandle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its own function.
Also ensure the buffer the driver sent us is big enough to fill in all the data we have -- else just fill in as much as the buffer can hold.
--- hw/ide/core.c | 42 ++++++++++++++++++++++++++++++++++++------ 1 files changed, 36 insertions(+), 6 deletions(-)
hw/ide/core.c +36 -6
@@ -1084,14 +1084,45 @@ static int ide_dvd_read_structure(IDEState *s, int format, } } +static unsigned int event_status_nea (uint8_t *buf,unsigned int max_len) +{ +unsigned int used_len; + +/* +* Ensure we don't write on memory we don't have. +* max_len of 0 should not produce an error as well. +*/ + used_len =0 ; + +/* No event descriptor returned */ +if (max_len >0 ) { + buf[0 ] =0 ; + used_len++; + } +if (max_len >1 ) { + buf[1 ] =0 ; + used_len++; + } +if (max_len >2 ) { + buf[2 ] =0x80 ;/* No Event Available (NEA) */ + used_len++; + } +if (max_len >3 ) { + buf[3 ] =0x00 ;/* Empty supported event classes */ + used_len++; + } +return used_len; +} +static void handle_get_event_status_notification (IDEState *s,uint8_t *buf,const uint8_t *packet) { -unsigned int max_len; +unsigned int max_len, used_len; max_len =ube16_to_cpu (packet +7 ); +/* It is fine by the MMC spec to not support async mode operations */ if (!(packet[1 ] &0x01 )) {/* asynchronous mode */ /* Only polling is supported, asynchronous mode is not. */ ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST,
@@ -1099,12 +1130,11 @@ 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); + used_len =event_status_nea (buf, max_len); +ide_atapi_cmd_reply (s, used_len, max_len); }static void ide_atapi_cmd (IDEState *s)
Signature · 1 line
1.7.4.2
[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 5 lines
Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its own function.
Also ensure the buffer the driver sent us is big enough to fill in all the data we have -- else just fill in as much as the buffer can hold.
This is unnecessary and in fact none of the IDE code does this. s->io_buffer isn't guest memory, but an internal buffer that is allocated like this:
s->io_buffer = qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4);
So that's more than enough for storing four bytes. ide_atapi_cmd_reply() takes care of making only max_size bytes visible to the guest.
Kevin
[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function
Quoted 4 lines · Kevin Wolf
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 3 lines
Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its own function.
Quoted 8 lines
Quoted 2 lines
Also ensure the buffer the driver sent us is big enough to fill in all the data we have -- else just fill in as much as the buffer can hold.
This is unnecessary and in fact none of the IDE code does this.
s->io_bufferisn't guest memory, but an internal buffer that is allocated like this:
s->io_buffer=qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4);
OK - so all the code paths will be much easier then.
But by my reading of (the kernel) code, it looks as if the kernel allocates the memory and passes it on. What am I missing?
Quoted 2 lines
So that's more than enough for storing four bytes.
ide_atapi_cmd_reply()takes care of making onlymax_sizebytes visible to the guest.
OK - but in some cases we just do a ide_set_irq() instead of ide_atapi_cmd_reply() so what happens in that case?
Amit
[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function
Am 09.04.2011 12:36, schrieb Amit Shah:
Quoted 12 lines · Kevin Wolf
Quoted 11 lines
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 5 lines
Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its own function.
Also ensure the buffer the driver sent us is big enough to fill in all the data we have -- else just fill in as much as the buffer can hold.
This is unnecessary and in fact none of the IDE code does this.
s->io_bufferisn't guest memory, but an internal buffer that is allocated like this:
Quoted 6 lines
Quoted 1 line
s->io_buffer=qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4);OK - so all the code paths will be much easier then.
But by my reading of (the kernel) code, it looks as if the kernel allocates the memory and passes it on. What am I missing?
That the data is copied. All command work on the internal s->io_buffer. Once the command is completed, the response can be transferred to the guest, either using DMA (see bmdma_rw_buf) or PIO (ide_data_readl).
Quoted 5 lines
Quoted 2 lines
So that's more than enough for storing four bytes.
ide_atapi_cmd_reply()takes care of making onlymax_sizebytes visible to the guest.OK - but in some cases we just do a
ide_set_irq()instead ofide_atapi_cmd_reply()so what happens in that case?
I can't find any ATAPI command that calls ide_set_irq directly. Anyway, the important thing is that s->io_buffer_size is set correctly.
Kevin
PATCH 4/5 atapi: GESN: Add enums for commonly-used field types
In reply to Amit ShahInstead of using magic numbers, use enums that are more descriptive of the fields being used.
--- hw/ide/core.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)
hw/ide/core.c +9 -2
@@ -1118,12 +1118,19 @@ static void handle_get_event_status_notification(IDEState *s,uint8_t *buf, const uint8_t *packet) { + enum cdb { + polled =1 , + request =4 , + allocation_length_msb =7 , + allocation_length_lsb =8 , + control = 9, + };unsigned int max_len, used_len; - max_len =ube16_to_cpu (packet +7 ); + max_len =ube16_to_cpu (packet + allocation_length_msb);/* It is fine by the MMC spec to not support async mode operations */ -if (!(packet[1 ] &0x01 )) {/* asynchronous mode */ +if (!(packet[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
[Qemu-devel] Re: [PATCH 4/5] atapi: GESN: Add enums for commonly-used field types
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 13 lines
Instead of using magic numbers, use enums that are more descriptive of the fields being used.
Signed-off-by Amit Shah <[email protected]>--- hw/ide/core.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)hw/ide/core.c +0
-0
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1127
Quoted 10 lines
hw/ide/core.c +7
-0@@ -1118,12 +1118,19 @@ static void handle_get_event_status_notification(IDEState *s,uint8_t *buf, const uint8_t *packet) { + enum cdb { + polled =1 , + request =4 , + allocation_length_msb =7 , + allocation_length_lsb =8 , + control = 9, + };
Wouldn't it be nicer to make this a struct and just cast packet to a pointer to this struct? At first I didn't realize that this should be field offsets and I find something like packet + allocation_length_msb rather confusing.
Kevin
[Qemu-devel] Re: [PATCH 4/5] atapi: GESN: Add enums for commonly-used field types
Quoted 21 lines · Kevin Wolf
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 20 lines
Instead of using magic numbers, use enums that are more descriptive of the fields being used.
Signed-off-by Amit Shah <[email protected]>--- hw/ide/core.c | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-)hw/ide/core.c +4
-0@@ -1118,12 +1118,19 @@ static void handle_get_event_status_notification(IDEState *s,uint8_t *buf, const uint8_t *packet) { + enum cdb { + polled =1 , + request =4 , + allocation_length_msb = 7,
Quoted 8 lines
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1127
Quoted 3 lines
hw/ide/core.c +3
-0+ allocation_length_lsb =8 , + control = 9, + };Wouldn't it be nicer to make this a struct and just cast packet to a pointer to this struct? At first I didn't realize that this should be field offsets and I find something like packet +
allocation_length_msbrather confusing.
Yes, indeed. It was faster for me to prototype it this way, but I will convert it to a struct.
Amit
PATCH 5/5 atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command
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 | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++- hw/ide/internal.h | 6 +++ 2 files changed, 97 insertions(+), 2 deletions(-)
hw/ide/core.c +91 -2
@@ -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 ; + } + +/* Event notification header */ +cpu_to_ube16 (buf, max_len); + buf[2 ] = event_class; + buf[3 ] = supported_events; + + 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) {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); + }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); }
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
[Qemu-devel] Re: [PATCH 5/5] atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 42 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 | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++- hw/ide/internal.h | 6 +++ 2 files changed, 97 insertions(+), 2 deletions(-)hw/ide/core.c +15
-0@@ -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 */ + };
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1111
Quoted 10 lines
hw/ide/core.c +10
-0+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?
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1116
Quoted 5 lines
hw/ide/core.c +5
-0+ +/* 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.
Quoted 9 lines
hw/ide/core.c +9
-0+ + 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 */
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1135
Quoted 10 lines
hw/ide/core.c +10
-0+ 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)
So I think we should return 8 and possible zero bytes 6 and 7.
Quoted 48 lines
hw/ide/core.c +31
-2+} +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; + +/*
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1231
Quoted 10 lines
hw/ide/core.c +10
-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 (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.
Quoted 28 lines
hw/ide/core.c +1
-0ide_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); }hw/ide/internal.h +5
-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;
hw/ide/internal.h struct IDEState { · line 419
Quoted 7 lines
hw/ide/internal.h +1
-0@@ -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?
Kevin
[Qemu-devel] Re: [PATCH 5/5] atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command
Quoted 47 lines · Kevin Wolf
Am 08.04.2011 09:15, schrieb Amit Shah:
Quoted 46 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 | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++- hw/ide/internal.h | 6 +++ 2 files changed, 97 insertions(+), 2 deletions(-)hw/ide/core.c +19
-0@@ -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 , + };
Quoted 8 lines
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1111
Quoted 6 lines
hw/ide/core.c +6
-0+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?
Quoted 8 lines
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1116
Quoted 5 lines
hw/ide/core.c +5
-0+ +/* 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_notificationas 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.
Quoted 13 lines
Quoted 13 lines
hw/ide/core.c +13
-0+ 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; + }
Quoted 8 lines
hw/ide/core.c static int ide_dvd_read_structure(IDEState *s, int format, · line 1135
Quoted 5 lines
hw/ide/core.c +5
-0+ + 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.
Quoted 56 lines
So I think we should return 8 and possible zero bytes 6 and 7.
Quoted 54 lines
hw/ide/core.c +37
-2+} +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,
Quoted 8 lines
hw/ide/core.c static void handle_get_event_status_notification(IDEState *s, · line 1231
Quoted 4 lines
hw/ide/core.c +4
-0+ 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_neaany 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.
Quoted 30 lines
Quoted 30 lines
hw/ide/core.c +1
-0ide_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); }hw/ide/internal.h +5
-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;
Quoted 8 lines
hw/ide/internal.h struct IDEState { · line 419
Quoted 6 lines
hw/ide/internal.h +1
-0char 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
Quoted 28 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.
Please apply!
Amit Shah (5):
atapi: AllowGET_EVENT_STATUS_NOTIFICATIONafter media change
ide: MoveGET_EVENT_STATUS_NOTIFICATIONcommand handling to its own
function
atapi: GESN: Spin off No Event Available handling into own function
atapi: GESN: Add enums for commonly-used field types
atapi: Implement 'media' subcommand ofGET_EVENT_STATUS_NOTIFICATION
command
Quoted 4 lines
hw/ide/core.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++----- hw/ide/internal.h | 6 ++ 2 files changed, 161 insertions(+), 17 deletions(-)
ACK patches 1-3.
For 4 and 5, I don't think lowercase enums are used in QEMU. I'm downloading the spec now so I can do a more complete review as well.
Paolo
Results of quick test run now, patch review to follow.
Test uses a simple program to try ioctl CDROM_DRIVE_STATUS (attached).
Test run in the host:
[start with tray closed & empty]
$ ./drive-status
CDS_NO_DISC
$ eject /dev/sr0
[tray opens]
$ ./drive-status
CDS_TRAY_OPEN
$ eject -t /dev/sr0
[tray closes]
$ ./drive-status
CDS_NO_DISC
[open tray, insert media, close tray]
$ ./drive-status
CDS_DISC_OK
$ eject /dev/sr0
[tray opens]
$ ./drive-status
CDS_TRAY_OPEN
$ eject -t /dev/sr0
[tray closes]
$ ./drive-status
CDS_DRIVE_NOT_READY
[ok, too impatient]
$ ./drive-status
CDS_DISC_OK
Test in guest without your patches:
[start with empty drive]
# ./drive-status
CDS_NO_DISC
# eject /dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should be CDS_TRAY_OPEN]
# eject -t /dev/sr0
# ./drive-status
CDS_NO_DISC
[insert media with monitor command change]
# ./drive-status
CDS_DISC_OK
# eject /dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should be CDS_TRAY_OPEN]
# eject -t /dev/sr0
# ./drive-status
CDS_DISC_OK
With the patches, it behaves as expected. Except something (guest kernel?) closes the tray right after eject if there's a medium in the open tray.
Anaconda is *much* happier with the patches. Before, it could get into a confused state where it can't find install medium because the tray is stuck open. Now, it recovers from "can't find" on first retry, by closing the tray.
#include <errno.h>
#include <error.h>
#include <fcntl.h>
#include <limits.h>
#include <stdio.h>
#include <linux/cdrom.h>
#include <sys/ioctl.h>
char *
cdsstr(int cds)
{
static char *ststr[5] = {
"CDS_NO_INFO",
"CDS_NO_DISC",
"CDS_TRAY_OPEN",
"CDS_DRIVE_NOT_READY",
"CDS_DISC_OK",
};
if ((unsigned)cds >= 5)
return NULL;
return ststr[cds];
}int
main(void)
{
int fd, cds;
char *s;
fd = open("/dev/sr0", O_RDONLY | O_NONBLOCK);
if (fd < 0)
error(1, errno, "open");
cds = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
if (cds < 0)
error(1, errno, "ioctl");
s = cdsstr(cds);
if (s)
printf("%s\n", s);
else
printf("%d\n", cds);
return 0;
}Quoted 3 lines · Markus Armbruster
Results of quick test run now, patch review to follow.
Test uses a simple program to try ioctl
CDROM_DRIVE_STATUS(attached).
...
Quoted 23 lines
Test in guest without your patches:
[start with empty drive]
# ./drive-status
CDS_NO_DISC
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_NO_DISC
[insert media with monitor command change]
# ./drive-status
CDS_DISC_OK
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_DISC_OK
Quoted 3 lines
With the patches, it behaves as expected. Except something (guest kernel?) closes the tray right after eject if there's a medium in the open tray.
Can you try with the two patches I sent on Saturday:
atapi: Drives can be locked without media present
atapi: Report correct errors on guest eject request
Thanks,
Amit
Quoted 30 lines · Amit Shah
Quoted 3 lines · Markus Armbruster
Results of quick test run now, patch review to follow.
Test uses a simple program to try ioctl
CDROM_DRIVE_STATUS(attached)....
Quoted 23 lines
Test in guest without your patches:
[start with empty drive]
# ./drive-status
CDS_NO_DISC
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_NO_DISC
[insert media with monitor command change]
# ./drive-status
CDS_DISC_OK
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_DISC_OK
Quoted 8 lines
Quoted 3 lines
With the patches, it behaves as expected. Except something (guest kernel?) closes the tray right after eject if there's a medium in the open tray.
Can you try with the two patches I sent on Saturday:
atapi: Drives can be locked without media present
atapi: Report correct errors on guest eject request
My test cases show no further improvement.
Quoted 36 lines · Markus Armbruster
Quoted 34 lines · Amit Shah
Quoted 3 lines · Markus Armbruster
Results of quick test run now, patch review to follow.
Test uses a simple program to try ioctl
CDROM_DRIVE_STATUS(attached)....
Quoted 26 lines
Test in guest without your patches:
[start with empty drive]
# ./drive-status
CDS_NO_DISC
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_NO_DISC
[insert media with monitor command change]
# ./drive-status
CDS_DISC_OK
# eject/dev/sr0
# ./drive-status
CDS_NO_DISC
[incorrect, should beCDS_TRAY_OPEN]
# eject -t/dev/sr0
# ./drive-status
CDS_DISC_OKWith the patches, it behaves as expected. Except something (guest kernel?) closes the tray right after eject if there's a medium in the open tray.
Quoted 6 lines
Quoted 4 lines
Can you try with the two patches I sent on Saturday:
atapi: Drives can be locked without media present
atapi: Report correct errors on guest eject requestMy test cases show no further improvement.
OK - thanks for testing!
Amit