Re: [Qemu-devel] [PATCH] atapi: Indicate 'medium not ready' to 'medium ready' transition via error

[Kevin Wolf <kwolf@redhat.com>]

Am 15.04.2011 11:33, schrieb Amit Shah:
> MMC-5 Table F.1 lists errors that can be thrown for the TEST_UNIT_READY
> command.  Going from medium not ready to medium ready states is
> communicated by throwing an error.
> 
> This adds the missing 'tray opened' event that we fail to report to
> guests.  After doing this, older Linux guests properly revalidate a disc
> on the change command.  HSM violation errors, which caused Linux guests
> to do a soft-reset of the link, also go away:
> 
> ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
> sr 1:0:0:0: CDB: Test Unit Ready: 00 00 00 00 00 00
> ata2.00: cmd a0/00:00:00:00:00/00:00:00:00:00/a0 tag 0
>          res 01/60:00:00:00:00/00:00:00:00:00/a0 Emask 0x3 (HSM violation)
> ata2.00: status: { ERR }
> ata2: soft resetting link
> ata2.00: configured for MWDMA2
> ata2: EH complete
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/ide/core.c |   32 ++++++++++++++++++++++++++------
>  1 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f028ddb..5a72420 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1253,9 +1253,16 @@ static void ide_atapi_cmd(IDEState *s)
>          if (bdrv_is_inserted(s->bs) && !s->cdrom_changed) {
>              ide_atapi_cmd_ok(s);
>          } else {
> -            s->cdrom_changed = 0;
> -            ide_atapi_cmd_error(s, SENSE_NOT_READY,
> -                                ASC_MEDIUM_NOT_PRESENT);
> +            int sense, asc;
> +
> +            sense = SENSE_NOT_READY;
> +            asc = ASC_MEDIUM_NOT_PRESENT;
> +            if (bdrv_is_inserted(s->bs) && s->cdrom_changed) {
> +                s->cdrom_changed = 0;
> +                sense = SENSE_UNIT_ATTENTION;
> +                asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> +            }
> +            ide_atapi_cmd_error(s, sense, asc);
>          }
>          break;
>      case GPCMD_MODE_SENSE_6:
> @@ -1734,11 +1741,24 @@ static void cdrom_change_cb(void *opaque, int reason)
>      bdrv_get_geometry(s->bs, &nb_sectors);
>      s->nb_sectors = nb_sectors;
>  
> -    s->sense_key = SENSE_UNIT_ATTENTION;
> -    s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
> +    /*
> +     * This function combines three actions a physical cdrom would do to
> +     * change a disc in a drive:
> +     *
> +     * 1. eject the tray,
> +     * 2. change the disc,
> +     * 3. close the tray.
> +     *
> +     * Guests expect responses from us in the same order.  So first,
> +     * just mark the disc changed, but provide an 'ejected' event to
> +     * the guest.  Later, when the guest invokes a TEST_UNIT_READY
> +     * command, we will provide with a disc change (UNIT_ATTENTION)
> +     * event.
> +     */
>      s->cdrom_changed = 1;
>      s->events.new_media = true;
> -    ide_set_irq(s->bus);
> +
> +    ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
>  }

ide_atapi_cmd_error expands to:

    s->error = sense_key << 4;
    s->status = READY_STAT | ERR_STAT;
    s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO |
ATAPI_INT_REASON_CD;
    s->sense_key = sense_key;
    s->asc = asc;
    ide_set_irq(s->bus);

Are you sure that you can modify the ATA error register here, even
though we're not responding to a guest initiated command? I think
possibly we're even in the middle of processing an independent command,
which would falsely appear to have failed.

I'm not even sure that raising an IRQ is correct in cdrom_change_cb.

Kevin