* [Qemu-devel] [PATCH 1/5] ahci: add is_ncq predicate helper
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
@ 2014-11-03 23:56 ` John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 2/5] ahci: Fix FIS decomposition John Snow
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2014-11-03 23:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mst, armbru, stefanha, pbonzini, John Snow
A small helper to determine which S/ATA commands
are destined to be routed to the NCQ pathways.
This references SATA 3.2 section 13.6,
Native Command Queueing. See sections 13.6.4,
13.6.5, 13.6.6, 13.6.7 and 13.6.8 for all
SATA commands considered to be part of the
NCQ feature set. This is summarized in a small
list in section 13.6.3.1 and again in 13.6.3.2.
Not all of these NCQ commands are currently supported,
so the error pathways are adjusted slightly to be more
informative in the case they are encountered.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 28 ++++++++++++++++++++++++----
hw/ide/ahci.h | 3 +++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f49be9f..722ac4a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -854,6 +854,21 @@ static void ncq_cb(void *opaque, int ret)
ncq_tfs->used = 0;
}
+static int is_ncq(uint8_t ata_cmd)
+{
+ /* Based on SATA 3.2 section 13.6.3.2 */
+ switch (ata_cmd) {
+ case READ_FPDMA_QUEUED:
+ case WRITE_FPDMA_QUEUED:
+ case NCQ_NON_DATA:
+ case RECEIVE_FPDMA_QUEUED:
+ case SEND_FPDMA_QUEUED:
+ return 1;
+ default:
+ return 0;
+ }
+}
+
static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
int slot)
{
@@ -919,9 +934,15 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
ncq_cb, ncq_tfs);
break;
default:
- DPRINTF(port, "error: tried to process non-NCQ command as NCQ\n");
+ if (is_ncq(cmd_fis[2])) {
+ DPRINTF(port,
+ "error: unsupported NCQ command (0x%02x) received\n",
+ cmd_fis[2]);
+ } else {
+ DPRINTF(port,
+ "error: tried to process non-NCQ command as NCQ\n");
+ }
qemu_sglist_destroy(&ncq_tfs->sglist);
- break;
}
}
@@ -1013,8 +1034,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
/* Check for NCQ command */
- if ((cmd_fis[2] == READ_FPDMA_QUEUED) ||
- (cmd_fis[2] == WRITE_FPDMA_QUEUED)) {
+ if (is_ncq(cmd_fis[2])) {
process_ncq_command(s, port, cmd_fis, slot);
goto out;
}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index b123237..e0d2eb8 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -186,6 +186,9 @@
#define READ_FPDMA_QUEUED 0x60
#define WRITE_FPDMA_QUEUED 0x61
+#define NCQ_NON_DATA 0x63
+#define RECEIVE_FPDMA_QUEUED 0x65
+#define SEND_FPDMA_QUEUED 0x64
#define RES_FIS_DSFIS 0x00
#define RES_FIS_PSFIS 0x20
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 2/5] ahci: Fix FIS decomposition
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 1/5] ahci: add is_ncq predicate helper John Snow
@ 2014-11-03 23:56 ` John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 3/5] ahci: Reorder error cases in handle_cmd John Snow
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2014-11-03 23:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mst, armbru, stefanha, pbonzini, John Snow
This patch introduces a few changes to how FIS packets are
deciphered in the AHCI virtual device. The summary of
changes can be grouped into two pieces:
[A] Changes to how we apply a preliminary sieve to FISes,
[B] Changes in how we internalize a decomposed FIS.
== Changes to how we apply a preliminary sieve to FISes ==
(1) Packets may now either update the Control register or
the Command register, but not both. This is according
to the SATA 3.2 specification which states:
"...the device either initiates processing of the command
indicated in the Command register or initiates processing
of the control request indicated [...] depending on the
state of the C bit in the FIS."
See SATA 3.2 section 10.5.5.4, "Reception" in the 10.5.5
"Register Host to Device FIS" section.
This change accounts for the first two regions of change
within the diff. All other changes belong to the following
changes.
== Changes in how we internalize a decomposed FIS ==
(2) Instead of trying to extract the sector number out of the
FIS from bytes 4-10 and setting it with ide_set_sector,
we set the appropriate IDEState registers and trust that
ide_get_sector can retrieve the correct sector later.
By "constructing" the sector for use with ide_set_sector,
we are duplicating the mechanisms of ide_get_sector.
This change makes the FIS decomposition more obvious.
SATA 3.2 as a specification does not make the legacy
register mapping with respect to the D2H FIS obvious.
However, SATA 3.2 section 10.5.5.1 "Register Host to
Device FIS layout" describes all of the "cmd_fis"
bytes:
0 - FIS Type (0x27)
1 - Port Multiplier Port and Command Update flag
2 - ATA Command
3 - Features_Low
4 - LBA 7:0
5 - LBA 15:8
6 - LBA 23:16
7 - Device, AKA "Drive Select."
8 - LBA 31:24
9 - LBA 39:32
10 - LBA 47:40
11 - Features_High
12 - Count Low
13 - Count High
14 - ICC
15 - Control
16-19 - Auxiliary (for NCQ, defined per-command)
Most of these registers map to existing IDEState registers
in obvious ways, especially features, select, hob_features,
and nsector (count). ICC is reserved in older specifications
but is not supported in our implementation, and remains
unused here. The Control register is not valid for a command
that is trying to update the command register and is to be
considered reserved at this point.
What is not obvious is the LBA register mappings, but SATA 1.0
can help inform of us legacy device support, see SATA 1.0 section
8.5.2 "Register - Host to Device."
LBA 7:0 - Sector Number (sector)
LBA 15:8 - Cyl Low (lcyl)
LBA 23:16 - Cyl High (hcyl)
LBA 31:24 - Sector Num Exp. (hob_sector)
LBA 39:32 - Cyl Low Exp. (hob_lcyl)
LBA 47:40 - Cyl High Exp. (hob_hcyl)
These mappings help guide which registers the FIS should be decomposed
into/towards for CHS, LBA28 and LBA48 commands.
As a note: The prior confusion that can be seen in the documentation
arises from the fact that CHS and LBA28 commands use the low nybble
of the drive select register to store LBA 27:24, whereas LNA48 commands
use the hob_sector, hob_lcyl and hob_hcyl registers as explained above.
The decomposition as it stands now will correctly decompose CHS, LBA28
and LBA48 commands into their appropriate registers where the core
IDE/ATAPI layers can deal with them correctly.
See the below point for more information.
(3) We save cmd_fis[7] as ide_state->select, which informs
decisions about if we are using LBA or CHS.
This corrects a bug in AHCI wherein we attempt to set and/or
retrieve the sector number by using ide_set_sector and
ide_get_sector, which depend on the select register to
determine if we are using LBA or CHS.
Without this adjustment, LBA48 read/writes are currently
broken. Thanks to Eniac Zheng @ HP for pointing this out.
(4) Save cmd_fis[11] as ide_state->hob_feature, as defined in SATA 3.2.
(5) For several ATA commands, the sector count register set to 0
is a magic number that means 256 sectors. For LBA48 commands,
this means 65,536 sectors. We drop the magic sector correction
here, and trust the ide core layer to handle the conversion
appropriately, in ide_cmd_lba48_transform(). As it stands,
the current AHCI code is only compliant with LBA28 commands.
By simply removing the magic, it will work with LBA28 and LBA48.
(6) We expand FIS decomposition to include both ATAPI and IDE devices.
We leave the logic of determining if the fields are valid or not
to the respective layers.
This change intends to make it clearer that AHCI is only a
composition mechanism for the FIS packets: the meanings of
the registers is best left to the implementation layers for
those devices.
(7) Forcefully setting the feature, hcyl and lcyl registers for ATAPI
commands is removed.
- The hcyl and lcyl magic present here is valid at boot only,
and should not be overridden for every PACKET command.
- The feature register is defined as valid for the PACKET command,
so we should not suppress it. The ATAPI layer does not even
currently depend on or require 0x01 as mandatory.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 64 ++++++++++++++++++++++++-----------------------------------
1 file changed, 26 insertions(+), 38 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 722ac4a..d162959 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1018,7 +1018,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
break;
}
- switch (s->dev[port].port_state) {
+ if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
+ switch (s->dev[port].port_state) {
case STATE_RUN:
if (cmd_fis[15] & ATA_SRST) {
s->dev[port].port_state = STATE_RESET;
@@ -1029,9 +1030,10 @@ static int handle_cmd(AHCIState *s, int port, int slot)
ahci_reset_port(s, port);
}
break;
+ }
}
- if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
+ else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
/* Check for NCQ command */
if (is_ncq(cmd_fis[2])) {
@@ -1039,50 +1041,36 @@ static int handle_cmd(AHCIState *s, int port, int slot)
goto out;
}
- /* Decompose the FIS */
- ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* Decompose the FIS:
+ * AHCI does not interpret FIS packets, it only forwards them.
+ * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
+ * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
+ *
+ * ATA4 describes sector number for LBA28/CHS commands.
+ * ATA6 describes sector number for LBA48 commands.
+ * ATA8 deprecates CHS fully, describing only LBA28/48.
+ *
+ * We dutifully convert the FIS into IDE registers, and allow the
+ * core layer to interpret them as needed. */
ide_state->feature = cmd_fis[3];
- if (!ide_state->nsector) {
- ide_state->nsector = 256;
- }
-
- if (ide_state->drive_kind != IDE_CD) {
- /*
- * We set the sector depending on the sector defined in the FIS.
- * Unfortunately, the spec isn't exactly obvious on this one.
- *
- * Apparently LBA48 commands set fis bytes 10,9,8,6,5,4 to the
- * 48 bit sector number. ATA_CMD_READ_DMA_EXT is an example for
- * such a command.
- *
- * Non-LBA48 commands however use 7[lower 4 bits],6,5,4 to define a
- * 28-bit sector number. ATA_CMD_READ_DMA is an example for such
- * a command.
- *
- * Since the spec doesn't explicitly state what each field should
- * do, I simply assume non-used fields as reserved and OR everything
- * together, independent of the command.
- */
- ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
- | ((uint64_t)cmd_fis[9] << 32)
- /* This is used for LBA48 commands */
- | ((uint64_t)cmd_fis[8] << 24)
- /* This is used for non-LBA48 commands */
- | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
- | ((uint64_t)cmd_fis[6] << 16)
- | ((uint64_t)cmd_fis[5] << 8)
- | cmd_fis[4]);
- }
+ ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
+ ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
+ ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
+ ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
+ ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
+ ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
+ ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
+ ide_state->hob_feature = cmd_fis[11];
+ ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
+ /* 15: Only valid when UPDATE_COMMAND not set. */
/* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
* table to ide_state->io_buffer
*/
if (opts & AHCI_CMD_ATAPI) {
memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
- ide_state->lcyl = 0x14;
- ide_state->hcyl = 0xeb;
debug_print_fis(ide_state->io_buffer, 0x10);
- ide_state->feature = IDE_FEATURE_DMA;
s->dev[port].done_atapi_packet = false;
/* XXX send PIO setup FIS */
}
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 3/5] ahci: Reorder error cases in handle_cmd
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 1/5] ahci: add is_ncq predicate helper John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 2/5] ahci: Fix FIS decomposition John Snow
@ 2014-11-03 23:56 ` John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 4/5] ahci: Check cmd_fis[1] more explicitly John Snow
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2014-11-03 23:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mst, armbru, stefanha, pbonzini, John Snow
Error checking in ahci's handle_cmd is re-ordered so that we
initialize as few things as possible before we've done our
sanity checking. This simplifies returning from this call
in case of an error.
A check to make sure the DMA memory map succeeds with the
correct size is also added, and the debug print of the
command fis is cleaned up with its size corrected.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d162959..3671be1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -961,38 +961,37 @@ static int handle_cmd(AHCIState *s, int port, int slot)
return -1;
}
- cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
-
if (!s->dev[port].lst) {
DPRINTF(port, "error: lst not given but cmd handled");
return -1;
}
-
+ cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot];
/* remember current slot handle for later */
s->dev[port].cur_cmd = cmd;
+ /* The device we are working for */
+ ide_state = &s->dev[port].port.ifs[0];
+ if (!ide_state->blk) {
+ DPRINTF(port, "error: guest accessed unused port");
+ return -1;
+ }
+
opts = le32_to_cpu(cmd->opts);
tbl_addr = le64_to_cpu(cmd->tbl_addr);
-
cmd_len = 0x80;
cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
DMA_DIRECTION_FROM_DEVICE);
-
if (!cmd_fis) {
DPRINTF(port, "error: guest passed us an invalid cmd fis\n");
return -1;
- }
-
- /* The device we are working for */
- ide_state = &s->dev[port].port.ifs[0];
-
- if (!ide_state->blk) {
- DPRINTF(port, "error: guest accessed unused port");
+ } else if (cmd_len != 0x80) {
+ ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_HBUS_ERR);
+ DPRINTF(port, "error: dma_memory_map failed: "
+ "(len(%02"PRIx64") != 0x80)\n",
+ cmd_len);
goto out;
}
-
- debug_print_fis(cmd_fis, 0x90);
- //debug_print_fis(cmd_fis, (opts & AHCI_CMD_HDR_CMD_FIS_LEN) * 4);
+ debug_print_fis(cmd_fis, 0x80);
switch (cmd_fis[0]) {
case SATA_FIS_TYPE_REGISTER_H2D:
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 4/5] ahci: Check cmd_fis[1] more explicitly
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
` (2 preceding siblings ...)
2014-11-03 23:56 ` [Qemu-devel] [PATCH 3/5] ahci: Reorder error cases in handle_cmd John Snow
@ 2014-11-03 23:56 ` John Snow
2014-11-03 23:56 ` [Qemu-devel] [PATCH 5/5] ahci: factor out FIS decomposition from handle_cmd John Snow
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2014-11-03 23:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mst, armbru, stefanha, pbonzini, John Snow
Instead of checking for a known byte, inspect the
fields of this byte explicitly to produce more meaningful
error messages and improve the readability of this section.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3671be1..536b455 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1004,17 +1004,18 @@ static int handle_cmd(AHCIState *s, int port, int slot)
break;
}
- switch (cmd_fis[1]) {
- case SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER:
- break;
- case 0:
- break;
- default:
- DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
- "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
- cmd_fis[2]);
- goto out;
- break;
+ if (cmd_fis[1] & 0x0F) {
+ DPRINTF(port, "Port Multiplier not supported."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ goto out;
+ }
+
+ if (cmd_fis[1] & 0x70) {
+ DPRINTF(port, "Reserved flags set in H2D Register FIS."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ goto out;
}
if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH 5/5] ahci: factor out FIS decomposition from handle_cmd
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
` (3 preceding siblings ...)
2014-11-03 23:56 ` [Qemu-devel] [PATCH 4/5] ahci: Check cmd_fis[1] more explicitly John Snow
@ 2014-11-03 23:56 ` John Snow
2014-11-04 15:48 ` [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements Paolo Bonzini
2014-11-13 10:08 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: John Snow @ 2014-11-03 23:56 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mst, armbru, stefanha, pbonzini, John Snow
In order to make handle_cmd more readable at the macro level,
the details of how to decompose particular types of FIS packets
are left to helper functions.
In our case, the only type of FIS packet we currently expect to
see is a Register H2D FIS packet, but the gory details of its
decomposition are of no particular interest in handle_cmd.
This patch keeps the receipt of FIS packets and the decomposition
thereof separated to two different functions.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 169 ++++++++++++++++++++++++++++++----------------------------
1 file changed, 86 insertions(+), 83 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 536b455..4f4bda0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -946,10 +946,94 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
}
}
+static void handle_reg_h2d_fis(AHCIState *s, int port,
+ int slot, uint8_t *cmd_fis)
+{
+ IDEState *ide_state = &s->dev[port].port.ifs[0];
+ AHCICmdHdr *cmd = s->dev[port].cur_cmd;
+ uint32_t opts = le32_to_cpu(cmd->opts);
+
+ if (cmd_fis[1] & 0x0F) {
+ DPRINTF(port, "Port Multiplier not supported."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ return;
+ }
+
+ if (cmd_fis[1] & 0x70) {
+ DPRINTF(port, "Reserved flags set in H2D Register FIS."
+ " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
+ cmd_fis[0], cmd_fis[1], cmd_fis[2]);
+ return;
+ }
+
+ if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
+ switch (s->dev[port].port_state) {
+ case STATE_RUN:
+ if (cmd_fis[15] & ATA_SRST) {
+ s->dev[port].port_state = STATE_RESET;
+ }
+ break;
+ case STATE_RESET:
+ if (!(cmd_fis[15] & ATA_SRST)) {
+ ahci_reset_port(s, port);
+ }
+ break;
+ }
+ return;
+ }
+
+ /* Check for NCQ command */
+ if (is_ncq(cmd_fis[2])) {
+ process_ncq_command(s, port, cmd_fis, slot);
+ return;
+ }
+
+ /* Decompose the FIS:
+ * AHCI does not interpret FIS packets, it only forwards them.
+ * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
+ * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
+ *
+ * ATA4 describes sector number for LBA28/CHS commands.
+ * ATA6 describes sector number for LBA48 commands.
+ * ATA8 deprecates CHS fully, describing only LBA28/48.
+ *
+ * We dutifully convert the FIS into IDE registers, and allow the
+ * core layer to interpret them as needed. */
+ ide_state->feature = cmd_fis[3];
+ ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
+ ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
+ ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
+ ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
+ ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
+ ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
+ ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
+ ide_state->hob_feature = cmd_fis[11];
+ ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
+ /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
+ /* 15: Only valid when UPDATE_COMMAND not set. */
+
+ /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
+ * table to ide_state->io_buffer */
+ if (opts & AHCI_CMD_ATAPI) {
+ memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
+ debug_print_fis(ide_state->io_buffer, 0x10);
+ s->dev[port].done_atapi_packet = false;
+ /* XXX send PIO setup FIS */
+ }
+
+ ide_state->error = 0;
+
+ /* Reset transferred byte counter */
+ cmd->status = 0;
+
+ /* We're ready to process the command in FIS byte 2. */
+ ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
+}
+
static int handle_cmd(AHCIState *s, int port, int slot)
{
IDEState *ide_state;
- uint32_t opts;
uint64_t tbl_addr;
AHCICmdHdr *cmd;
uint8_t *cmd_fis;
@@ -976,7 +1060,6 @@ static int handle_cmd(AHCIState *s, int port, int slot)
return -1;
}
- opts = le32_to_cpu(cmd->opts);
tbl_addr = le64_to_cpu(cmd->tbl_addr);
cmd_len = 0x80;
cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len,
@@ -995,93 +1078,13 @@ static int handle_cmd(AHCIState *s, int port, int slot)
switch (cmd_fis[0]) {
case SATA_FIS_TYPE_REGISTER_H2D:
+ handle_reg_h2d_fis(s, port, slot, cmd_fis);
break;
default:
DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x "
"cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1],
cmd_fis[2]);
- goto out;
- break;
- }
-
- if (cmd_fis[1] & 0x0F) {
- DPRINTF(port, "Port Multiplier not supported."
- " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
- cmd_fis[0], cmd_fis[1], cmd_fis[2]);
- goto out;
- }
-
- if (cmd_fis[1] & 0x70) {
- DPRINTF(port, "Reserved flags set in H2D Register FIS."
- " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n",
- cmd_fis[0], cmd_fis[1], cmd_fis[2]);
- goto out;
- }
-
- if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) {
- switch (s->dev[port].port_state) {
- case STATE_RUN:
- if (cmd_fis[15] & ATA_SRST) {
- s->dev[port].port_state = STATE_RESET;
- }
break;
- case STATE_RESET:
- if (!(cmd_fis[15] & ATA_SRST)) {
- ahci_reset_port(s, port);
- }
- break;
- }
- }
-
- else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
-
- /* Check for NCQ command */
- if (is_ncq(cmd_fis[2])) {
- process_ncq_command(s, port, cmd_fis, slot);
- goto out;
- }
-
- /* Decompose the FIS:
- * AHCI does not interpret FIS packets, it only forwards them.
- * SATA 1.0 describes how to decode LBA28 and CHS FIS packets.
- * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets.
- *
- * ATA4 describes sector number for LBA28/CHS commands.
- * ATA6 describes sector number for LBA48 commands.
- * ATA8 deprecates CHS fully, describing only LBA28/48.
- *
- * We dutifully convert the FIS into IDE registers, and allow the
- * core layer to interpret them as needed. */
- ide_state->feature = cmd_fis[3];
- ide_state->sector = cmd_fis[4]; /* LBA 7:0 */
- ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */
- ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */
- ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */
- ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */
- ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */
- ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */
- ide_state->hob_feature = cmd_fis[11];
- ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]);
- /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */
- /* 15: Only valid when UPDATE_COMMAND not set. */
-
- /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
- * table to ide_state->io_buffer
- */
- if (opts & AHCI_CMD_ATAPI) {
- memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10);
- debug_print_fis(ide_state->io_buffer, 0x10);
- s->dev[port].done_atapi_packet = false;
- /* XXX send PIO setup FIS */
- }
-
- ide_state->error = 0;
-
- /* Reset transferred byte counter */
- cmd->status = 0;
-
- /* We're ready to process the command in FIS byte 2. */
- ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
}
out:
--
1.9.3
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
` (4 preceding siblings ...)
2014-11-03 23:56 ` [Qemu-devel] [PATCH 5/5] ahci: factor out FIS decomposition from handle_cmd John Snow
@ 2014-11-04 15:48 ` Paolo Bonzini
2014-11-13 10:08 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2014-11-04 15:48 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: kwolf, armbru, stefanha, mst
On 04/11/2014 00:56, John Snow wrote:
> This patch series may require, and is based on another
> AHCI series pending on-list:
> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg04143.html
>
> This series aims to improve the way that handle_cmd
> reads and behaves in interpreting the FIS packets
> received by the AHCI HBA. This series used to be
> part of a larger RFC, but has since been pared
> down to a more focused series.
>
> Previous RFC:
> http://lists.gnu.org/archive/html/qemu-devel/2014-09/msg02694.html
>
> Patch 1 is a subset of the previously posted
> "AHCI: Rename NCQFIS structure fields" patch.
> This subset is just aimed at improving the readability
> of the FIS decomposition in handle_cmd for now.
>
> Patch 2 is nearly the same as "AHCI: Fix FIS decomposition"
> which has yet to be reviewed.
>
> Patches 3, 4, and 5 have been inspected previously, and should
> be identical.
> "ide/ahci: Reorder error cases in handle_cmd"
> "ahci: Check cmd_fis[1] more explicitly"
> "ahci: factor out FIS decomposition"
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements
2014-11-03 23:56 [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements John Snow
` (5 preceding siblings ...)
2014-11-04 15:48 ` [Qemu-devel] [PATCH 0/5] ahci: fis decomposition improvements Paolo Bonzini
@ 2014-11-13 10:08 ` Stefan Hajnoczi
6 siblings, 0 replies; 8+ messages in thread
From: Stefan Hajnoczi @ 2014-11-13 10:08 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, mst, armbru, qemu-devel, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1627 bytes --]
On Mon, Nov 03, 2014 at 06:56:14PM -0500, John Snow wrote:
> This patch series may require, and is based on another
> AHCI series pending on-list:
> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg04143.html
>
> This series aims to improve the way that handle_cmd
> reads and behaves in interpreting the FIS packets
> received by the AHCI HBA. This series used to be
> part of a larger RFC, but has since been pared
> down to a more focused series.
>
> Previous RFC:
> http://lists.gnu.org/archive/html/qemu-devel/2014-09/msg02694.html
>
> Patch 1 is a subset of the previously posted
> "AHCI: Rename NCQFIS structure fields" patch.
> This subset is just aimed at improving the readability
> of the FIS decomposition in handle_cmd for now.
>
> Patch 2 is nearly the same as "AHCI: Fix FIS decomposition"
> which has yet to be reviewed.
>
> Patches 3, 4, and 5 have been inspected previously, and should
> be identical.
> "ide/ahci: Reorder error cases in handle_cmd"
> "ahci: Check cmd_fis[1] more explicitly"
> "ahci: factor out FIS decomposition"
>
> John Snow (5):
> ahci: add is_ncq predicate helper
> AHCI: Fix FIS decomposition
> ahci: Reorder error cases in handle_cmd
> ahci: Check cmd_fis[1] more explicitly
> ahci: factor out FIS decomposition from handle_cmd
>
> hw/ide/ahci.c | 233 ++++++++++++++++++++++++++++++----------------------------
> hw/ide/ahci.h | 3 +
> 2 files changed, 125 insertions(+), 111 deletions(-)
>
> --
> 1.9.3
>
>
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread