[v4,5/5] atapi: GESN: implement 'media' subcommand

Submitted by Amit Shah on April 12, 2011, 2:09 p.m.

Details

Message ID 1632463ee09a9df5690475f0b507757c0876fce9.1302617257.git.amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah April 12, 2011, 2:09 p.m.
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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/ide/internal.h |    6 +++
 2 files changed, 110 insertions(+), 5 deletions(-)

Comments

Kevin Wolf April 12, 2011, 2:54 p.m.
Am 12.04.2011 16:09, 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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/ide/internal.h |    6 +++
>  2 files changed, 110 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index dafc049..209d8e6 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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;

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
Amit Shah April 12, 2011, 3:03 p.m.
On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote:
> Am 12.04.2011 16:09, 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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  hw/ide/internal.h |    6 +++
> >  2 files changed, 110 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index dafc049..209d8e6 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -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;
> 
> 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
Jes Sorensen April 12, 2011, 3:03 p.m.
On 04/12/11 16:09, Amit Shah wrote:
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index dafc049..209d8e6 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -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,
> +    };

Would be nice if you used upper-case with the enums since they are
effectively just defines, to avoid confusing them with variables.

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

Shouldn't you verify that you don't exceed max_len in this?

> @@ -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,
> +    };

More lower-case enums :(

Otherwise the series looks good.

Jes
Amit Shah April 12, 2011, 3:05 p.m.
On (Tue) 12 Apr 2011 [20:33:39], Amit Shah wrote:
> On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote:
> > Am 12.04.2011 16:09, 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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  hw/ide/internal.h |    6 +++
> > >  2 files changed, 110 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > > index dafc049..209d8e6 100644
> > > --- a/hw/ide/core.c
> > > +++ b/hw/ide/core.c
> > > @@ -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;
> > 
> > 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.

Oh; that reminds me:  that's why I had the notification_class = 0
initialisation up there.



		Amit
Kevin Wolf April 12, 2011, 3:11 p.m.
Am 12.04.2011 17:03, schrieb Amit Shah:
> On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote:
>> Am 12.04.2011 16:09, 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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  hw/ide/internal.h |    6 +++
>>>  2 files changed, 110 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index dafc049..209d8e6 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -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;
>>
>> 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?

|= 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
Jes Sorensen April 12, 2011, 3:13 p.m.
On 04/12/11 17:13, Kevin Wolf wrote:
> Am 12.04.2011 17:03, schrieb Jes Sorensen:
>> Shouldn't you verify that you don't exceed max_len in 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

That works fine too - I just didn't like having a length and then having
it ignored completely.

Cheers,
Jes
Kevin Wolf April 12, 2011, 3:13 p.m.
Am 12.04.2011 17:03, schrieb Jes Sorensen:
> On 04/12/11 16:09, Amit Shah wrote:
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index dafc049..209d8e6 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -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,
>> +    };
> 
> Would be nice if you used upper-case with the enums since they are
> effectively just defines, to avoid confusing them with variables.
> 
>> +    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 */
> 
> Shouldn't you verify that you don't exceed max_len in 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
Amit Shah April 12, 2011, 3:16 p.m.
On (Tue) 12 Apr 2011 [17:11:22], Kevin Wolf wrote:
> Am 12.04.2011 17:03, schrieb Amit Shah:
> > On (Tue) 12 Apr 2011 [16:54:53], Kevin Wolf wrote:
> >> Am 12.04.2011 16:09, 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     |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>>  hw/ide/internal.h |    6 +++
> >>>  2 files changed, 110 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>> index dafc049..209d8e6 100644
> >>> --- a/hw/ide/core.c
> >>> +++ b/hw/ide/core.c
> >>> @@ -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;
> >>
> >> 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?
> 
> |= 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

Patch hide | download patch | download mbox

diff --git a/hw/ide/core.c b/hw/ide/core.c
index dafc049..209d8e6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -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 */
         }
     }
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;