[Qemu-devel] [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Qemu-devel] [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Amit Shah]

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(-)

-- 
1.7.4.2

[Qemu-devel] [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change

[Amit Shah]

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.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c11d457..327f703 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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:
-- 
1.7.4.2

[Qemu-devel] [PATCH 2/5] ide: Move GET_EVENT_STATUS_NOTIFICATION command handling to its own function

[Amit Shah]

This makes the code more readable.

Also, there's a block like:

if () {
  ...
} else {
  ...
}

Split that into

if () {
  ...
  return;
}
...

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |   37 ++++++++++++++++++++++++-------------
 1 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 327f703..c4a5a13 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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,
-- 
1.7.4.2

[Qemu-devel] [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function

[Amit Shah]

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.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |   42 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index c4a5a13..730587e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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)
-- 
1.7.4.2

[Qemu-devel] [PATCH 4/5] atapi: GESN: Add enums for commonly-used field types

[Amit Shah]

Instead of using magic numbers, use enums that are more descriptive of
the fields being used.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 730587e..cdc2c56 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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);
-- 
1.7.4.2

[Qemu-devel] [PATCH 5/5] atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command

[Amit Shah]

Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION
command.  This helps us report tray open, tray closed, no media, media
present states to the guest.

Newer Linux kernels (2.6.38+) rely on this command to revalidate discs
after media change.

This patch also sends out tray open/closed status to the guest driver
when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus).
Without such notification, the guest and qemu's tray open/close status
was frequently out of sync, causing installers like Anaconda detecting
no disc instead of tray open, confusing them terribly.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c     |   93 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |    6 +++
 2 files changed, 97 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index cdc2c56..627b2cf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1084,6 +1084,57 @@ static int ide_dvd_read_structure(IDEState *s, int format,
     }
 }
 
+static unsigned int event_status_media(IDEState *s,
+                                       uint8_t *buf,
+                                       unsigned int max_len,
+                                       unsigned int event_class,
+                                       unsigned int supported_events)
+{
+    enum media_event_code {
+        no_change = 0,       /* Status unchanged */
+        eject_requested,     /* received a request from user to eject */
+        new_media,           /* new media inserted and ready for access */
+        media_removal,       /* only for media changers */
+        media_changed,       /* only for media changers */
+        bg_format_completed, /* MRW or DVD+RW b/g format completed */
+        bg_format_restarted, /* MRW or DVD+RW b/g format restarted */
+    };
+    enum media_status {
+        tray_open = 1,
+        media_present = 2,
+    };
+    uint8_t event_code, media_status;
+
+    if (max_len < 6) {
+        /* We're going to use 6 bytes; don't report anything if we have less */
+        return 0;
+    }
+
+    /* 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);
 }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index d533fb6..ba7e9a8 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -373,6 +373,11 @@ typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
 typedef void DMARestartFunc(void *, int, int);
 
+struct unreported_events {
+    bool eject_request;
+    bool new_media;
+};
+
 /* NOTE: IDEState represents in fact one drive */
 struct IDEState {
     IDEBus *bus;
@@ -408,6 +413,7 @@ struct IDEState {
     BlockDriverState *bs;
     char version[9];
     /* ATAPI specific */
+    struct unreported_events events;
     uint8_t sense_key;
     uint8_t asc;
     uint8_t cdrom_changed;
-- 
1.7.4.2

[Qemu-devel] Re: [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Paolo Bonzini]

On 04/08/2011 09:15 AM, Amit Shah wrote:
> 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(-)
>

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

[Qemu-devel] Re: [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change

[Amit Shah]

On (Fri) 08 Apr 2011 [12:45:15], Amit Shah wrote:
> 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.

This isn't true -- the messages go only if TEST_UNIT_READY is also
added to the list.  Removed this statement from local copy.

> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c11d457..327f703 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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 0/5] atapi: Implement 'media' subcommand for GESN

[Markus Armbruster]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

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.


[-- Attachment #2: Type: text/plain, Size: 703 bytes --]

#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;
}

Re: [Qemu-devel] [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> 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.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/ide/core.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c11d457..327f703 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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.

>      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:

Re: [Qemu-devel] [PATCH 1/5] atapi: Allow GET_EVENT_STATUS_NOTIFICATION after media change

[Kevin Wolf]

Am 08.04.2011 12:54, schrieb Markus Armbruster:
> Amit Shah <amit.shah@redhat.com> writes:
> 
>> 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.
>>
>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> ---
>>  hw/ide/core.c |    9 +++++----
>>  1 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index c11d457..327f703 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -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

[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function

[Kevin Wolf]

Am 08.04.2011 09:15, schrieb Amit Shah:
> 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 4/5] atapi: GESN: Add enums for commonly-used field types

[Kevin Wolf]

Am 08.04.2011 09:15, schrieb Amit Shah:
> Instead of using magic numbers, use enums that are more descriptive of
> the fields being used.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/ide/core.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 730587e..cdc2c56 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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 5/5] atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command

[Kevin Wolf]

Am 08.04.2011 09:15, schrieb Amit Shah:
> Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION
> command.  This helps us report tray open, tray closed, no media, media
> present states to the guest.
> 
> Newer Linux kernels (2.6.38+) rely on this command to revalidate discs
> after media change.
> 
> This patch also sends out tray open/closed status to the guest driver
> when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus).
> Without such notification, the guest and qemu's tray open/close status
> was frequently out of sync, causing installers like Anaconda detecting
> no disc instead of tray open, confusing them terribly.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/ide/core.c     |   93 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/ide/internal.h |    6 +++
>  2 files changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index cdc2c56..627b2cf 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1084,6 +1084,57 @@ static int ide_dvd_read_structure(IDEState *s, int format,
>      }
>  }
>  
> +static unsigned int event_status_media(IDEState *s,
> +                                       uint8_t *buf,
> +                                       unsigned int max_len,
> +                                       unsigned int event_class,
> +                                       unsigned int supported_events)
> +{
> +    enum media_event_code {
> +        no_change = 0,       /* Status unchanged */
> +        eject_requested,     /* received a request from user to eject */
> +        new_media,           /* new media inserted and ready for access */
> +        media_removal,       /* only for media changers */
> +        media_changed,       /* only for media changers */
> +        bg_format_completed, /* MRW or DVD+RW b/g format completed */
> +        bg_format_restarted, /* MRW or DVD+RW b/g format restarted */
> +    };
> +    enum media_status {
> +        tray_open = 1,
> +        media_present = 2,
> +    };
> +    uint8_t event_code, media_status;
> +
> +    if (max_len < 6) {
> +        /* We're going to use 6 bytes; don't report anything if we have less */
> +        return 0;
> +    }

Why? Shouldn't we report the first few bytes?

> +
> +    /* 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.

> +
> +    media_status = 0;
> +    if (s->bs->tray_open) {
> +        media_status = tray_open;
> +    } else if (bdrv_is_inserted(s->bs)) {
> +        media_status = media_present;
> +    }
> +
> +    /* Event notification descriptor */
> +    event_code = no_change;
> +    if (media_status != tray_open && s->events.new_media) {
> +        event_code = new_media;
> +        s->events.new_media = false;
> +    }
> +
> +    buf[4] = event_code;
> +    buf[5] = media_status;
> +
> +    return 6; /* We wrote to just 2 extra bytes from the header */

After media_state, there are two more fields for start/end slot (even
though they are reserved because we don't have a multiple slot device)

So I think we should return 8 and possible zero bytes 6 and 7.

> +}
> +
>  static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len)
>  {
>      unsigned int used_len;
> @@ -1125,7 +1176,28 @@ static void handle_get_event_status_notification(IDEState *s,
>          allocation_length_lsb = 8,
>          control = 9,
>      };
> +    enum notification_class_request_type {
> +        reserved1 = 1 << 0,
> +        operational_change = 1 << 1,
> +        power_management = 1 << 2,
> +        external_request = 1 << 3,
> +        media = 1 << 4,
> +        multi_host = 1 << 5,
> +        device_busy = 1 << 6,
> +        reserved2 = 1 << 7,
> +    };
> +    enum event_notification_class_field {
> +        enc_no_events = 0,
> +        enc_operational_change,
> +        enc_power_management,
> +        enc_external_request,
> +        enc_media,
> +        enc_multiple_hosts,
> +        enc_device_busy,
> +        enc_reserved,
> +    };
>      unsigned int max_len, used_len;
> +    unsigned int supported_events;
>  
>      max_len = ube16_to_cpu(packet + allocation_length_msb);
>  
> @@ -1139,8 +1211,24 @@ static void handle_get_event_status_notification(IDEState *s,
>  
>      /* polling mode operation */
>  
> -    /* We don't support any event class (yet). */
> -    used_len = event_status_nea(buf, max_len);
> +    /*
> +     * These are the supported events.
> +     *
> +     * We currently only support requests of the 'media' type.
> +     */
> +    supported_events = media;
> +
> +    /*
> +     * Responses to requests are to be based on request priority.  The
> +     * notification_class_request_type enum above specifies the
> +     * priority: upper elements are higher prio than lower ones.
> +     */
> +    if (packet[request] & media) {
> +        used_len = event_status_media(s, buf, max_len, enc_media,
> +                                      supported_events);
> +    } else {
> +        used_len = event_status_nea(buf, max_len);
> +    }

Once you put the event header here, there's no need for event_status_nea
any more because it just mean not adding any descriptors. That will work
much better as soon as we have more even classes, too.

>      ide_atapi_cmd_reply(s, used_len, max_len);
>  }
>  
> @@ -1661,6 +1749,7 @@ static void cdrom_change_cb(void *opaque, int reason)
>      s->sense_key = SENSE_UNIT_ATTENTION;
>      s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
>      s->cdrom_changed = 1;
> +    s->events.new_media = true;
>      ide_set_irq(s->bus);
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index d533fb6..ba7e9a8 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -373,6 +373,11 @@ typedef int DMAFunc(IDEDMA *);
>  typedef int DMAIntFunc(IDEDMA *, int);
>  typedef void DMARestartFunc(void *, int, int);
>  
> +struct unreported_events {
> +    bool eject_request;
> +    bool new_media;
> +};
> +
>  /* NOTE: IDEState represents in fact one drive */
>  struct IDEState {
>      IDEBus *bus;
> @@ -408,6 +413,7 @@ struct IDEState {
>      BlockDriverState *bs;
>      char version[9];
>      /* ATAPI specific */
> +    struct unreported_events events;
>      uint8_t sense_key;
>      uint8_t asc;
>      uint8_t cdrom_changed;

Do we need to add some vmstate fields?

Kevin

[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function

[Amit Shah]

On (Fri) 08 Apr 2011 [15:31:49], Kevin Wolf wrote:
> Am 08.04.2011 09:15, schrieb Amit Shah:
> > 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);

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?

> 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.

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 4/5] atapi: GESN: Add enums for commonly-used field types

[Amit Shah]

On (Fri) 08 Apr 2011 [16:21:38], Kevin Wolf wrote:
> Am 08.04.2011 09:15, schrieb Amit Shah:
> > Instead of using magic numbers, use enums that are more descriptive of
> > the fields being used.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/ide/core.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index 730587e..cdc2c56 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -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.

Yes, indeed.  It was faster for me to prototype it this way, but I
will convert it to a struct.

		Amit

[Qemu-devel] Re: [PATCH 5/5] atapi: Implement 'media' subcommand of GET_EVENT_STATUS_NOTIFICATION command

[Amit Shah]

On (Fri) 08 Apr 2011 [18:17:38], Kevin Wolf wrote:
> Am 08.04.2011 09:15, schrieb Amit Shah:
> > Implement the 'media' sub-command of the GET_EVENT_STATUS_NOTIFICATION
> > command.  This helps us report tray open, tray closed, no media, media
> > present states to the guest.
> > 
> > Newer Linux kernels (2.6.38+) rely on this command to revalidate discs
> > after media change.
> > 
> > This patch also sends out tray open/closed status to the guest driver
> > when requested e.g. via the CDROM_DRIVE_STATUS ioctl (thanks Markus).
> > Without such notification, the guest and qemu's tray open/close status
> > was frequently out of sync, causing installers like Anaconda detecting
> > no disc instead of tray open, confusing them terribly.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/ide/core.c     |   93 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/ide/internal.h |    6 +++
> >  2 files changed, 97 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index cdc2c56..627b2cf 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -1084,6 +1084,57 @@ static int ide_dvd_read_structure(IDEState *s, int format,
> >      }
> >  }
> >  
> > +static unsigned int event_status_media(IDEState *s,
> > +                                       uint8_t *buf,
> > +                                       unsigned int max_len,
> > +                                       unsigned int event_class,
> > +                                       unsigned int supported_events)
> > +{
> > +    enum media_event_code {
> > +        no_change = 0,       /* Status unchanged */
> > +        eject_requested,     /* received a request from user to eject */
> > +        new_media,           /* new media inserted and ready for access */
> > +        media_removal,       /* only for media changers */
> > +        media_changed,       /* only for media changers */
> > +        bg_format_completed, /* MRW or DVD+RW b/g format completed */
> > +        bg_format_restarted, /* MRW or DVD+RW b/g format restarted */
> > +    };
> > +    enum media_status {
> > +        tray_open = 1,
> > +        media_present = 2,
> > +    };
> > +    uint8_t event_code, media_status;
> > +
> > +    if (max_len < 6) {
> > +        /* We're going to use 6 bytes; don't report anything if we have less */
> > +        return 0;
> > +    }
> 
> Why? Shouldn't we report the first few bytes?

This shouldn't be necessary at all now, isn't it?

> > +
> > +    /* Event notification header */
> > +    cpu_to_ube16(buf, max_len);
> > +    buf[2] = event_class;
> > +    buf[3] = supported_events;
> 
> This should be done in handle_get_event_status_notification as it's not
> specific for media events.

Yes, I moved it here just before the submission.  With the used_len
stuff not being needed anymore, this can be put back there and code
won't look too ugly.

> > +    media_status = 0;
> > +    if (s->bs->tray_open) {
> > +        media_status = tray_open;
> > +    } else if (bdrv_is_inserted(s->bs)) {
> > +        media_status = media_present;
> > +    }
> > +
> > +    /* Event notification descriptor */
> > +    event_code = no_change;
> > +    if (media_status != tray_open && s->events.new_media) {
> > +        event_code = new_media;
> > +        s->events.new_media = false;
> > +    }
> > +
> > +    buf[4] = event_code;
> > +    buf[5] = media_status;
> > +
> > +    return 6; /* We wrote to just 2 extra bytes from the header */
> 
> After media_state, there are two more fields for start/end slot (even
> though they are reserved because we don't have a multiple slot device)

Yes, they're reserved, so we shouldn't change them.  Any change might
trigger bad response from guests.

> So I think we should return 8 and possible zero bytes 6 and 7.
> 
> > +}
> > +
> >  static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len)
> >  {
> >      unsigned int used_len;
> > @@ -1125,7 +1176,28 @@ static void handle_get_event_status_notification(IDEState *s,
> >          allocation_length_lsb = 8,
> >          control = 9,
> >      };
> > +    enum notification_class_request_type {
> > +        reserved1 = 1 << 0,
> > +        operational_change = 1 << 1,
> > +        power_management = 1 << 2,
> > +        external_request = 1 << 3,
> > +        media = 1 << 4,
> > +        multi_host = 1 << 5,
> > +        device_busy = 1 << 6,
> > +        reserved2 = 1 << 7,
> > +    };
> > +    enum event_notification_class_field {
> > +        enc_no_events = 0,
> > +        enc_operational_change,
> > +        enc_power_management,
> > +        enc_external_request,
> > +        enc_media,
> > +        enc_multiple_hosts,
> > +        enc_device_busy,
> > +        enc_reserved,
> > +    };
> >      unsigned int max_len, used_len;
> > +    unsigned int supported_events;
> >  
> >      max_len = ube16_to_cpu(packet + allocation_length_msb);
> >  
> > @@ -1139,8 +1211,24 @@ static void handle_get_event_status_notification(IDEState *s,
> >  
> >      /* polling mode operation */
> >  
> > -    /* We don't support any event class (yet). */
> > -    used_len = event_status_nea(buf, max_len);
> > +    /*
> > +     * These are the supported events.
> > +     *
> > +     * We currently only support requests of the 'media' type.
> > +     */
> > +    supported_events = media;
> > +
> > +    /*
> > +     * Responses to requests are to be based on request priority.  The
> > +     * notification_class_request_type enum above specifies the
> > +     * priority: upper elements are higher prio than lower ones.
> > +     */
> > +    if (packet[request] & media) {
> > +        used_len = event_status_media(s, buf, max_len, enc_media,
> > +                                      supported_events);
> > +    } else {
> > +        used_len = event_status_nea(buf, max_len);
> > +    }
> 
> Once you put the event header here, there's no need for event_status_nea
> any more because it just mean not adding any descriptors. That will work
> much better as soon as we have more even classes, too.

Hm, I didn't do it that way when I first coded this so there must've
been something that I'm forgetting now.

> 
> >      ide_atapi_cmd_reply(s, used_len, max_len);
> >  }
> >  
> > @@ -1661,6 +1749,7 @@ static void cdrom_change_cb(void *opaque, int reason)
> >      s->sense_key = SENSE_UNIT_ATTENTION;
> >      s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> >      s->cdrom_changed = 1;
> > +    s->events.new_media = true;
> >      ide_set_irq(s->bus);
> >  }
> >  
> > diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> > index d533fb6..ba7e9a8 100644
> > --- a/hw/ide/internal.h
> > +++ b/hw/ide/internal.h
> > @@ -373,6 +373,11 @@ typedef int DMAFunc(IDEDMA *);
> >  typedef int DMAIntFunc(IDEDMA *, int);
> >  typedef void DMARestartFunc(void *, int, int);
> >  
> > +struct unreported_events {
> > +    bool eject_request;
> > +    bool new_media;
> > +};
> > +
> >  /* NOTE: IDEState represents in fact one drive */
> >  struct IDEState {
> >      IDEBus *bus;
> > @@ -408,6 +413,7 @@ struct IDEState {
> >      BlockDriverState *bs;
> >      char version[9];
> >      /* ATAPI specific */
> > +    struct unreported_events events;
> >      uint8_t sense_key;
> >      uint8_t asc;
> >      uint8_t cdrom_changed;
> 
> Do we need to add some vmstate fields?

Yes!  I had it in mind till sometime back.

		Amit

Re: [Qemu-devel] [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Amit Shah]

On (Fri) 08 Apr 2011 [11:39:26], Markus Armbruster wrote:
> Results of quick test run now, patch review to follow.
> 
> Test uses a simple program to try ioctl CDROM_DRIVE_STATUS (attached).

...

> 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.

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

[Qemu-devel] Re: [PATCH 3/5] atapi: GESN: Spin off No Event Available handling into own function

[Kevin Wolf]

Am 09.04.2011 12:36, schrieb Amit Shah:
> On (Fri) 08 Apr 2011 [15:31:49], Kevin Wolf wrote:
>> Am 08.04.2011 09:15, schrieb Amit Shah:
>>> 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);
> 
> 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).

>> 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.
> 
> OK - but in some cases we just do a ide_set_irq() instead of
> ide_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

Re: [Qemu-devel] [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Fri) 08 Apr 2011 [11:39:26], Markus Armbruster wrote:
>> Results of quick test run now, patch review to follow.
>> 
>> Test uses a simple program to try ioctl CDROM_DRIVE_STATUS (attached).
>
> ...
>
>> 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.
>
> 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.

Re: [Qemu-devel] [PATCH 0/5] atapi: Implement 'media' subcommand for GESN

[Amit Shah]

On (Mon) 11 Apr 2011 [15:46:33], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Fri) 08 Apr 2011 [11:39:26], Markus Armbruster wrote:
> >> Results of quick test run now, patch review to follow.
> >> 
> >> Test uses a simple program to try ioctl CDROM_DRIVE_STATUS (attached).
> >
> > ...
> >
> >> 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.
> >
> > 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.

OK - thanks for testing!

		Amit