hw/ide/ahci.c | 36 +++++++++++++++++++++++++----------- hw/ide/ahci_internal.h | 2 +- tests/libqos/ahci.c | 25 ++++++++++++++++--------- tests/libqos/ahci.h | 2 +- 4 files changed, 43 insertions(+), 22 deletions(-)
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>
---
hw/ide/ahci.c | 36 +++++++++++++++++++++++++-----------
hw/ide/ahci_internal.h | 2 +-
tests/libqos/ahci.c | 25 ++++++++++++++++---------
tests/libqos/ahci.h | 2 +-
4 files changed, 43 insertions(+), 22 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f7852be842..82b3f755c4 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;
@@ -1251,6 +1249,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
/* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
* table to ide_state->io_buffer */
+ s->dev[port].done_first_drq = false;
if (opts & AHCI_CMD_ATAPI) {
memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
if (trace_event_get_state_backends(TRACE_HANDLE_REG_H2D_FIS_DUMP)) {
@@ -1258,7 +1257,6 @@ 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;
@@ -1351,13 +1349,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 +1387,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 +1641,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.17.0
On 06/22/2018 12:51 PM, Paolo Bonzini wrote:
> 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>
> ---
> hw/ide/ahci.c | 36 +++++++++++++++++++++++++-----------
> hw/ide/ahci_internal.h | 2 +-
> tests/libqos/ahci.c | 25 ++++++++++++++++---------
> tests/libqos/ahci.h | 2 +-
> 4 files changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index f7852be842..82b3f755c4 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;
>
> @@ -1251,6 +1249,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>
> /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
> * table to ide_state->io_buffer */
> + s->dev[port].done_first_drq = false;
If you don't mind I'm going to shift this down so that it's beneath the
ATAPI section, just before the ide_exec_cmd call alongside all of the
other reset/init calls.
> if (opts & AHCI_CMD_ATAPI) {
> memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
> if (trace_event_get_state_backends(TRACE_HANDLE_REG_H2D_FIS_DUMP)) {
> @@ -1258,7 +1257,6 @@ 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;
> @@ -1351,13 +1349,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 +1387,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 +1641,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),
Clever, thanks.
> 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;
> + }
Why are we gating on the DRQ byte count limit?
(Oh, I guess it CAN'T be zero if we expect to transfer any data, so this
assumption is good to test if we expect to see even a single DRQ block.)
> 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);
>
Reviewed-by: John Snow <jsnow@redhat.com>
On 22/06/2018 21:07, John Snow wrote:
>> @@ -1251,6 +1249,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>>
>> /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
>> * table to ide_state->io_buffer */
>> + s->dev[port].done_first_drq = false;
>
> If you don't mind I'm going to shift this down so that it's beneath the
> ATAPI section, just before the ide_exec_cmd call alongside all of the
> other reset/init calls.
It's okay if the tests pass. :)
>> @@ -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;
>> + }
>
> Why are we gating on the DRQ byte count limit?
>
> (Oh, I guess it CAN'T be zero if we expect to transfer any data, so this
> assumption is good to test if we expect to see even a single DRQ block.)
This is ATAPI, so "having a PIO Setup FIS with I=1" is the same as
"having >1 PIO Setup FIS's" (data direction doesn't matter) and in turn
that is the same as "having some bytes to transfer while DMA is off".
Thanks,
Paolo
On 06/22/2018 05:06 PM, Paolo Bonzini wrote:
> On 22/06/2018 21:07, John Snow wrote:
>>> @@ -1251,6 +1249,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
>>>
>>> /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
>>> * table to ide_state->io_buffer */
>>> + s->dev[port].done_first_drq = false;
>>
>> If you don't mind I'm going to shift this down so that it's beneath the
>> ATAPI section, just before the ide_exec_cmd call alongside all of the
>> other reset/init calls.
>
> It's okay if the tests pass. :)
>
They do! Thanks for solving two of the mysteries I had for the AHCI
emulation.
>>> @@ -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;
>>> + }
>>
>> Why are we gating on the DRQ byte count limit?
>>
>> (Oh, I guess it CAN'T be zero if we expect to transfer any data, so this
>> assumption is good to test if we expect to see even a single DRQ block.)
>
> This is ATAPI, so "having a PIO Setup FIS with I=1" is the same as
> "having >1 PIO Setup FIS's" (data direction doesn't matter) and in turn
> that is the same as "having some bytes to transfer while DMA is off".
>
Right, but "byte count limit" being the same as "having some bytes to
transfer" took me an extra minute. It's okay in the end because BCL is
not allowed to be zero if there is any data to transfer.
> Thanks,
>
> Paolo
>
On 06/22/2018 12:51 PM, Paolo Bonzini wrote: > 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> Thanks, applied to my IDE tree: https://github.com/jnsnow/qemu/commits/ide https://github.com/jnsnow/qemu.git --js
© 2016 - 2024 Red Hat, Inc.