[Qemu-devel] [PULL 0/1] Ide patches

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit 14e60aaece20a1cfc059a69f6491b0899f9257a8:

  hw/net/e1000: Don't use *_to_cpup() (2016-06-27 16:39:56 +0100)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 0d0437aac6dc94bdc1601968bbb9e623e34bd3e7:

  macio: Use blk_drain instead of blk_drain_all (2016-06-27 14:28:31 -0400)

----------------------------------------------------------------

Just the one little patch.

----------------------------------------------------------------

Fam Zheng (1):
  macio: Use blk_drain instead of blk_drain_all

 hw/ide/macio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.4.11

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 27 June 2016 at 20:24, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 14e60aaece20a1cfc059a69f6491b0899f9257a8:
>
>   hw/net/e1000: Don't use *_to_cpup() (2016-06-27 16:39:56 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 0d0437aac6dc94bdc1601968bbb9e623e34bd3e7:
>
>   macio: Use blk_drain instead of blk_drain_all (2016-06-27 14:28:31 -0400)
>
> ----------------------------------------------------------------
>
> Just the one little patch.
>
> ----------------------------------------------------------------
>
> Fam Zheng (1):
>   macio: Use blk_drain instead of blk_drain_all
>
>  hw/ide/macio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

-- PMM

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2016-07-27 18:18:21 +0100)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 87ac25fd1fed05a30a93d27dbeb2a4c4b83ec95f:

  ide: fix halted IO segfault at reset (2016-07-28 17:34:19 -0400)

----------------------------------------------------------------

----------------------------------------------------------------

John Snow (1):
  ide: fix halted IO segfault at reset

 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 28 July 2016 at 23:50, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 21a21b853a1bb606358af61e738abfb9aecbd720:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2016-07-27 18:18:21 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 87ac25fd1fed05a30a93d27dbeb2a4c4b83ec95f:
>
>   ide: fix halted IO segfault at reset (2016-07-28 17:34:19 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> John Snow (1):
>   ide: fix halted IO segfault at reset
>
>  hw/ide/core.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- PMM

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit ab861f3915e8667927cf18ad97f71cae7ccf8818:

  Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into staging (2016-08-09 10:44:27 +0100)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 7f951b2d7765f68ae1e563c2fed44071ca774790:

  atapi: fix halted DMA reset (2016-08-09 11:47:23 -0400)

----------------------------------------------------------------

----------------------------------------------------------------

John Snow (1):
  atapi: fix halted DMA reset

 hw/ide/atapi.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.7.4

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 9 August 2016 at 16:50, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit ab861f3915e8667927cf18ad97f71cae7ccf8818:
>
>   Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into staging (2016-08-09 10:44:27 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 7f951b2d7765f68ae1e563c2fed44071ca774790:
>
>   atapi: fix halted DMA reset (2016-08-09 11:47:23 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> John Snow (1):
>   atapi: fix halted DMA reset
>
>  hw/ide/atapi.c | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- PMM

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit 61eedf7aec0e2395aabd628cc055096909a3ea15:

  tests/prom-env: Ease time-out problems on slow hosts (2017-02-10 15:44:53 +0000)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 98cb5dccb192b0082626080890dac413473573c6:

  ahci: advertise HOST_CAP_64 (2017-02-10 11:47:11 -0500)

----------------------------------------------------------------

----------------------------------------------------------------

Ladi Prosek (1):
  ahci: advertise HOST_CAP_64

 hw/ide/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.9.3

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 10 February 2017 at 16:48, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 61eedf7aec0e2395aabd628cc055096909a3ea15:
>
>   tests/prom-env: Ease time-out problems on slow hosts (2017-02-10 15:44:53 +0000)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 98cb5dccb192b0082626080890dac413473573c6:
>
>   ahci: advertise HOST_CAP_64 (2017-02-10 11:47:11 -0500)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

gpg: Signature made Fri 10 Feb 2017 16:47:54 GMT
gpg:                using RSA key 0x7DEF8106AAFC390E
gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>"
gpg: Note: This key has expired!
Primary key fingerprint: FAEB 9711 A12C F475 812F  18F2 88A9 064D 1835 61EB
     Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76  CBD0 7DEF 8106 AAFC 390E

...?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

On 02/10/2017 12:49 PM, Peter Maydell wrote:
> On 10 February 2017 at 16:48, John Snow <jsnow@redhat.com> wrote:
>> The following changes since commit 61eedf7aec0e2395aabd628cc055096909a3ea15:
>>
>>   tests/prom-env: Ease time-out problems on slow hosts (2017-02-10 15:44:53 +0000)
>>
>> are available in the git repository at:
>>
>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>
>> for you to fetch changes up to 98cb5dccb192b0082626080890dac413473573c6:
>>
>>   ahci: advertise HOST_CAP_64 (2017-02-10 11:47:11 -0500)
>>
>> ----------------------------------------------------------------
>>
>> ----------------------------------------------------------------
> 
> gpg: Signature made Fri 10 Feb 2017 16:47:54 GMT
> gpg:                using RSA key 0x7DEF8106AAFC390E
> gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>"
> gpg: Note: This key has expired!
> Primary key fingerprint: FAEB 9711 A12C F475 812F  18F2 88A9 064D 1835 61EB
>      Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76  CBD0 7DEF 8106 AAFC 390E
> 
> ...?
> 
> thanks
> -- PMM
> 

[Discussed on IRC, but for the benefit of list archives;]

My key expired after the holidays, but I've redone the expiry and
re-pushed to public keyservers. Please refresh from the public keys list
and let me know if you have further problems.

Thanks,
--John

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 10 February 2017 at 18:46, John Snow <jsnow@redhat.com> wrote:
>
>
> On 02/10/2017 12:49 PM, Peter Maydell wrote:
>> On 10 February 2017 at 16:48, John Snow <jsnow@redhat.com> wrote:
>>> The following changes since commit 61eedf7aec0e2395aabd628cc055096909a3ea15:
>>>
>>>   tests/prom-env: Ease time-out problems on slow hosts (2017-02-10 15:44:53 +0000)
>>>
>>> are available in the git repository at:
>>>
>>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>>
>>> for you to fetch changes up to 98cb5dccb192b0082626080890dac413473573c6:
>>>
>>>   ahci: advertise HOST_CAP_64 (2017-02-10 11:47:11 -0500)
>>>
>>> ----------------------------------------------------------------
>>>
>>> ----------------------------------------------------------------
>>
>> gpg: Signature made Fri 10 Feb 2017 16:47:54 GMT
>> gpg:                using RSA key 0x7DEF8106AAFC390E
>> gpg: Good signature from "John Snow (John Huston) <jsnow@redhat.com>"
>> gpg: Note: This key has expired!
>> Primary key fingerprint: FAEB 9711 A12C F475 812F  18F2 88A9 064D 1835 61EB
>>      Subkey fingerprint: F9B7 ABDB BCAC DF95 BE76  CBD0 7DEF 8106 AAFC 390E
>>
>> ...?
>>
>> thanks
>> -- PMM
>>
>
> [Discussed on IRC, but for the benefit of list archives;]
>
> My key expired after the holidays, but I've redone the expiry and
> re-pushed to public keyservers. Please refresh from the public keys list
> and let me know if you have further problems.

Yep, I refreshed the key and have now applied this to master.

thanks
-- PMM

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit 0833df03f4206a6cf416fbb3d380fa95c8e61fba:

  Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180206a' into staging (2018-02-07 12:07:23 +0000)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 29e1d473ccb0703044541959df443c175ea5f5da:

  ide-test: test trim requests (2018-02-07 11:25:22 -0500)

----------------------------------------------------------------

----------------------------------------------------------------

Anton Nefedov (1):
  ide-test: test trim requests

 tests/ide-test.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

-- 
2.14.3

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 7 February 2018 at 16:33, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 0833df03f4206a6cf416fbb3d380fa95c8e61fba:
>
>   Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20180206a' into staging (2018-02-07 12:07:23 +0000)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 29e1d473ccb0703044541959df443c175ea5f5da:
>
>   ide-test: test trim requests (2018-02-07 11:25:22 -0500)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> Anton Nefedov (1):
>   ide-test: test trim requests
>
>  tests/ide-test.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>

Applied, thanks.

-- PMM

[Qemu-devel] [PULL 0/1] Ide patches

[John Snow]

The following changes since commit 35e238c9330669882487f9929e0aa97900431853:

  Merge remote-tracking branch 'remotes/kraxel/tags/audio-20180625-pull-request' into staging (2018-06-25 15:25:26 +0100)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to ae79c2db150e17757ee1be080481be675a15ccea:

  ahci: fix FIS I bit and PIO Setup FIS interrupt (2018-06-25 16:50:48 -0400)

----------------------------------------------------------------
Pull request

----------------------------------------------------------------

Paolo Bonzini (1):
  ahci: fix FIS I bit and PIO Setup FIS interrupt

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

-- 
2.14.4

[Qemu-devel] [PULL 1/1] ahci: fix FIS I bit and PIO Setup FIS interrupt

[John Snow]

From: Paolo Bonzini <pbonzini@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 <kraxel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20180622165159.19863-1-pbonzini@redhat.com
[Minor edit to avoid ATAPI comment ambiguity. --js]
Signed-off-by: John Snow <jsnow@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);
-- 
2.14.4

Re: [Qemu-devel] [PULL 1/1] ahci: fix FIS I bit and PIO Setup FIS interrupt

[John Snow]

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 <pbonzini@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 <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Message-id: 20180622165159.19863-1-pbonzini@redhat.com
> [Minor edit to avoid ATAPI comment ambiguity. --js]
> Signed-off-by: John Snow <jsnow@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

Re: [Qemu-devel] [PULL 0/1] Ide patches

[Peter Maydell]

On 25 June 2018 at 22:11, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 35e238c9330669882487f9929e0aa97900431853:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/audio-20180625-pull-request' into staging (2018-06-25 15:25:26 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to ae79c2db150e17757ee1be080481be675a15ccea:
>
>   ahci: fix FIS I bit and PIO Setup FIS interrupt (2018-06-25 16:50:48 -0400)
>
> ----------------------------------------------------------------
> Pull request
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM