Michael: It's probably much too late to include this in the 2.12.1
roundup, isn't it?
I'd either push for you to include this fix OR to drop the other ATAPI
related fix...
--js
On 06/25/2018 05:11 PM, John Snow wrote:
> From: Paolo Bonzini <pbonz...@redhat.com>
>
> The "I" bit in PIO Setup and D2H FISes is exclusively a device concept
> and the irqstatus register in the controller does not matter. The SATA
> spec says when it should be one; for D2H FISes in practice it is always
> set, while the PIO Setup FIS has several subcases that are documented in
> the patch.
>
> Also, the PIO Setup FIS interrupt is actually generated _after_ data
> has been received.
>
> Someone should probably spend some time reading the SATA specification and
> figuring out the more obscure fields in the PIO Setup FIS, but this is enough
> to fix SeaBIOS booting from ATAPI CD-ROMs over an AHCI controller.
>
> Fixes: 956556e131e35f387ac482ad7b41151576fef057
> Reported-by: Gerd Hoffmann <kra...@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> Reviewed-by: John Snow <js...@redhat.com>
> Message-id: 20180622165159.19863-1-pbonz...@redhat.com
> [Minor edit to avoid ATAPI comment ambiguity. --js]
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> hw/ide/ahci.c | 37 +++++++++++++++++++++++++------------
> hw/ide/ahci_internal.h | 2 +-
> tests/libqos/ahci.c | 25 ++++++++++++++++---------
> tests/libqos/ahci.h | 2 +-
> 4 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 2ec24cad9f..d700ca973b 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -801,7 +801,7 @@ static void ahci_write_fis_sdb(AHCIState *s,
> NCQTransferState *ncq_tfs)
> }
> }
>
> -static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
> +static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len, bool pio_fis_i)
> {
> AHCIPortRegs *pr = &ad->port_regs;
> uint8_t *pio_fis;
> @@ -814,7 +814,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t
> len)
> pio_fis = &ad->res_fis[RES_FIS_PSFIS];
>
> pio_fis[0] = SATA_FIS_TYPE_PIO_SETUP;
> - pio_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
> + pio_fis[1] = (pio_fis_i ? (1 << 6) : 0);
> pio_fis[2] = s->status;
> pio_fis[3] = s->error;
>
> @@ -842,8 +842,6 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t
> len)
> if (pio_fis[2] & ERR_STAT) {
> ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
> }
> -
> - ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
> }
>
> static bool ahci_write_fis_d2h(AHCIDevice *ad)
> @@ -860,7 +858,7 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
> d2h_fis = &ad->res_fis[RES_FIS_RFIS];
>
> d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H;
> - d2h_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
> + d2h_fis[1] = (1 << 6); /* interrupt bit */
> d2h_fis[2] = s->status;
> d2h_fis[3] = s->error;
>
> @@ -1258,11 +1256,10 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
> trace_handle_reg_h2d_fis_dump(s, port, pretty_fis);
> g_free(pretty_fis);
> }
> - s->dev[port].done_atapi_packet = false;
> }
>
> ide_state->error = 0;
> -
> + s->dev[port].done_first_drq = false;
> /* Reset transferred byte counter */
> cmd->status = 0;
>
> @@ -1351,13 +1348,23 @@ static void ahci_pio_transfer(IDEDMA *dma)
> int is_write = opts & AHCI_CMD_WRITE;
> int is_atapi = opts & AHCI_CMD_ATAPI;
> int has_sglist = 0;
> + bool pio_fis_i;
>
> - /* PIO FIS gets written prior to transfer */
> - ahci_write_fis_pio(ad, size);
> + /* The PIO Setup FIS is received prior to transfer, but the interrupt
> + * is only triggered after data is received.
> + *
> + * The device only sets the 'I' bit in the PIO Setup FIS for device->host
> + * requests (see "DPIOI1" in the SATA spec), or for host->device DRQs
> after
> + * the first (see "DPIOO1"). The latter is consistent with the spec's
> + * description of the PACKET protocol, where the command part of ATAPI
> requests
> + * ("DPKT0") has the 'I' bit clear, while the data part of PIO ATAPI
> requests
> + * ("DPKT4a" and "DPKT7") has the 'I' bit set for both directions for
> all DRQs.
> + */
> + pio_fis_i = ad->done_first_drq || (!is_atapi && !is_write);
> + ahci_write_fis_pio(ad, size, pio_fis_i);
>
> - if (is_atapi && !ad->done_atapi_packet) {
> + if (is_atapi && !ad->done_first_drq) {
> /* already prepopulated iobuffer */
> - ad->done_atapi_packet = true;
> goto out;
> }
>
> @@ -1379,9 +1386,15 @@ static void ahci_pio_transfer(IDEDMA *dma)
>
> /* Update number of transferred bytes, destroy sglist */
> dma_buf_commit(s, size);
> +
> out:
> /* declare that we processed everything */
> s->data_ptr = s->data_end;
> +
> + ad->done_first_drq = true;
> + if (pio_fis_i) {
> + ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
> + }
> }
>
> static void ahci_start_dma(IDEDMA *dma, IDEState *s,
> @@ -1627,7 +1640,7 @@ static const VMStateDescription vmstate_ahci_device = {
> VMSTATE_UINT32(port_regs.scr_err, AHCIDevice),
> VMSTATE_UINT32(port_regs.scr_act, AHCIDevice),
> VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
> - VMSTATE_BOOL(done_atapi_packet, AHCIDevice),
> + VMSTATE_BOOL(done_first_drq, AHCIDevice),
> VMSTATE_INT32(busy_slot, AHCIDevice),
> VMSTATE_BOOL(init_d2h_sent, AHCIDevice),
> VMSTATE_STRUCT_ARRAY(ncq_tfs, AHCIDevice, AHCI_MAX_CMDS,
> diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
> index 2953243929..9b7fa8fc7d 100644
> --- a/hw/ide/ahci_internal.h
> +++ b/hw/ide/ahci_internal.h
> @@ -315,7 +315,7 @@ struct AHCIDevice {
> QEMUBH *check_bh;
> uint8_t *lst;
> uint8_t *res_fis;
> - bool done_atapi_packet;
> + bool done_first_drq;
> int32_t busy_slot;
> bool init_d2h_sent;
> AHCICmdHdr *cur_cmd;
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index 7264e085d0..42d3f76933 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -651,10 +651,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port,
> /* Command creation */
> if (opts->atapi) {
> uint16_t bcl = opts->set_bcl ? opts->bcl : ATAPI_SECTOR_SIZE;
> - cmd = ahci_atapi_command_create(op, bcl);
> - if (opts->atapi_dma) {
> - ahci_command_enable_atapi_dma(cmd);
> - }
> + cmd = ahci_atapi_command_create(op, bcl, opts->atapi_dma);
> } else {
> cmd = ahci_command_create(op);
> }
> @@ -874,7 +871,6 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> /* cmd->interrupts |= props->data ? AHCI_PX_IS_DPS : 0; */
> /* BUG: We expect the DMA Setup interrupt for DMA commands */
> /* cmd->interrupts |= props->dma ? AHCI_PX_IS_DSS : 0; */
> - cmd->interrupts |= props->pio ? AHCI_PX_IS_PSS : 0;
> cmd->interrupts |= props->ncq ? AHCI_PX_IS_SDBS : 0;
>
> command_header_init(cmd);
> @@ -883,19 +879,24 @@ AHCICommand *ahci_command_create(uint8_t command_name)
> return cmd;
> }
>
> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl)
> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl, bool
> dma)
> {
> AHCICommand *cmd = ahci_command_create(CMD_PACKET);
> cmd->atapi_cmd = g_malloc0(16);
> cmd->atapi_cmd[0] = scsi_cmd;
> stw_le_p(&cmd->fis.lba_lo[1], bcl);
> + if (dma) {
> + ahci_command_enable_atapi_dma(cmd);
> + } else {
> + cmd->interrupts |= bcl ? AHCI_PX_IS_PSS : 0;
> + }
> return cmd;
> }
>
> void ahci_atapi_test_ready(AHCIQState *ahci, uint8_t port,
> bool ready, uint8_t expected_sense)
> {
> - AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_TEST_UNIT_READY,
> 0);
> + AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_TEST_UNIT_READY,
> 0, false);
> ahci_command_set_size(cmd, 0);
> if (!ready) {
> cmd->interrupts |= AHCI_PX_IS_TFES;
> @@ -937,7 +938,7 @@ void ahci_atapi_get_sense(AHCIQState *ahci, uint8_t port,
>
> void ahci_atapi_eject(AHCIQState *ahci, uint8_t port)
> {
> - AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_START_STOP_UNIT,
> 0);
> + AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_START_STOP_UNIT,
> 0, false);
> ahci_command_set_size(cmd, 0);
>
> cmd->atapi_cmd[4] = 0x02; /* loej = true */
> @@ -949,7 +950,7 @@ void ahci_atapi_eject(AHCIQState *ahci, uint8_t port)
>
> void ahci_atapi_load(AHCIQState *ahci, uint8_t port)
> {
> - AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_START_STOP_UNIT,
> 0);
> + AHCICommand *cmd = ahci_atapi_command_create(CMD_ATAPI_START_STOP_UNIT,
> 0, false);
> ahci_command_set_size(cmd, 0);
>
> cmd->atapi_cmd[4] = 0x03; /* loej,start = true */
> @@ -1098,6 +1099,12 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t
> xbytes,
> } else if (cmd->props->atapi) {
> ahci_atapi_set_size(cmd, xbytes);
> } else {
> + /* For writes, the PIO Setup FIS interrupt only comes from DRQs
> + * after the first.
> + */
> + if (cmd->props->pio && sect_count > (cmd->props->read ? 0 : 1)) {
> + cmd->interrupts |= AHCI_PX_IS_PSS;
> + }
> cmd->fis.count = sect_count;
> }
> cmd->header.prdtl = size_to_prdtl(cmd->xbytes, cmd->prd_size);
> diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
> index 13f6d87b75..f05b3e5fce 100644
> --- a/tests/libqos/ahci.h
> +++ b/tests/libqos/ahci.h
> @@ -622,7 +622,7 @@ void ahci_atapi_load(AHCIQState *ahci, uint8_t port);
>
> /* Command: Fine-grained lifecycle */
> AHCICommand *ahci_command_create(uint8_t command_name);
> -AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl);
> +AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd, uint16_t bcl, bool
> dma);
> void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port);
> void ahci_command_issue(AHCIQState *ahci, AHCICommand *cmd);
> void ahci_command_issue_async(AHCIQState *ahci, AHCICommand *cmd);
>
--
—js
