| Message ID | 1412204151-18117-2-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
John Snow <jsnow@redhat.com> writes: > Currently, the D2H FIS packets AHCI generates simply parrot back > the LBA that the guest sent to us in the cmd_fis. However, some > commands (like READ NATIVE MAX) modify the LBA registers as a > return value, through which the AHCI D2H FIS is the only response > mechanism. Thus, the D2H response should use the current register > values, not the initial ones. > > This patch adjusts the LBA and drive select register responses for > PIO Setup and D2H FIS response packets. > > Additionally, the PIO and D2H FIS responses copy too many bytes > from the command FIS that it is being generated from. Specifically, > byte 11 which is the Features(15:8) field for Register Host to > Device FIS packets, is instead reserved for the PIO Setup FIS and > should always be 0. Ignorant q: is this based on observation or some specification? If specification: where can I find a copy?
On 10/27/2014 05:32 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> Currently, the D2H FIS packets AHCI generates simply parrot back >> the LBA that the guest sent to us in the cmd_fis. However, some >> commands (like READ NATIVE MAX) modify the LBA registers as a >> return value, through which the AHCI D2H FIS is the only response >> mechanism. Thus, the D2H response should use the current register >> values, not the initial ones. >> >> This patch adjusts the LBA and drive select register responses for >> PIO Setup and D2H FIS response packets. >> >> Additionally, the PIO and D2H FIS responses copy too many bytes >> from the command FIS that it is being generated from. Specifically, >> byte 11 which is the Features(15:8) field for Register Host to >> Device FIS packets, is instead reserved for the PIO Setup FIS and >> should always be 0. > > Ignorant q: is this based on observation or some specification? If > specification: where can I find a copy? > Not ignorant. I do all kinds of crazy things. It's based off an interpretation of the specification and an observation. Specification: The SATA specification defines the FIS responses to commands via the Register Device to Host FIS. See Sata 3.2 section 10.5.6 "Register Device to Host FIS" The fields of this packet are defined as things like: "LBA(7:0) - Contains the *new value* of the LBA low register of the Shadow Register Block." (emphasis mine.) Many other registers are defined similarly. All six LBA registers, Device, Error, Count, and so on. Implying that instead of returning back what the Host-To-Device FIS set, we are returning what it is currently (the "new value" after the command was run.) The observation: READ_NATIVE_MAX_ADDRESS is a non-data command, so it performs no PIO or DMA actions on the IDE device. It will only return, in the case of the AHCI controller, an FIS packet. If the FIS packet returns (for example) the LBA registers that the user sent, there is no mechanism by which the host can actually /receive/ the native max address. Therefore, I interpret the specification to imply that the response FIS after successful completion of a command should return the new, current values of the IDE registers, and not simply parrot back what the user sent. It's a synchronization mechanism between the device and the host. For further specification... READ_NATIVE_MAX_ADDRESS is deprecated in AC3, but we can look at ATA8-ACS revision 4a, which is pretty clear about the line response to this command: Section 7.33 READ NATIVE MAX ADDRESS - F8h, Non-data, subsection 7.33.4 "Normal Outputs": "See table 96. LBA contains the Native Max Address. Bits 47:28 of LBA shall be cleared to zero." Table 96 - "HPA Normal Output" Specifies the line output for this command. Error, Count, LBA, Device, Status. These line outputs are for sure the information that is bundled up into the D2H FIS. On this basis, then, is the reasoning behind this patch.
John Snow <jsnow@redhat.com> writes: > On 10/27/2014 05:32 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> Currently, the D2H FIS packets AHCI generates simply parrot back >>> the LBA that the guest sent to us in the cmd_fis. However, some >>> commands (like READ NATIVE MAX) modify the LBA registers as a >>> return value, through which the AHCI D2H FIS is the only response >>> mechanism. Thus, the D2H response should use the current register >>> values, not the initial ones. >>> >>> This patch adjusts the LBA and drive select register responses for >>> PIO Setup and D2H FIS response packets. >>> >>> Additionally, the PIO and D2H FIS responses copy too many bytes >>> from the command FIS that it is being generated from. Specifically, >>> byte 11 which is the Features(15:8) field for Register Host to >>> Device FIS packets, is instead reserved for the PIO Setup FIS and >>> should always be 0. >> >> Ignorant q: is this based on observation or some specification? If >> specification: where can I find a copy? >> > > Not ignorant. I do all kinds of crazy things. It's based off an > interpretation of the specification and an observation. > > Specification: The SATA specification defines the FIS responses to > commands via the Register Device to Host FIS. See Sata 3.2 section > 10.5.6 "Register Device to Host FIS" > > The fields of this packet are defined as things like: > "LBA(7:0) - Contains the *new value* of the LBA low register of the > Shadow Register Block." (emphasis mine.) Many other registers are > defined similarly. All six LBA registers, Device, Error, Count, and so > on. > > Implying that instead of returning back what the Host-To-Device FIS > set, we are returning what it is currently (the "new value" after the > command was run.) > > The observation: READ_NATIVE_MAX_ADDRESS is a non-data command, so it > performs no PIO or DMA actions on the IDE device. It will only return, > in the case of the AHCI controller, an FIS packet. If the FIS packet > returns (for example) the LBA registers that the user sent, there is > no mechanism by which the host can actually /receive/ the native max > address. > > Therefore, I interpret the specification to imply that the response > FIS after successful completion of a command should return the new, > current values of the IDE registers, and not simply parrot back what > the user sent. It's a synchronization mechanism between the device and > the host. > > For further specification... READ_NATIVE_MAX_ADDRESS is deprecated in > AC3, but we can look at ATA8-ACS revision 4a, which is pretty clear > about the line response to this command: > > Section 7.33 READ NATIVE MAX ADDRESS - F8h, Non-data, subsection > 7.33.4 "Normal Outputs": > "See table 96. LBA contains the Native Max Address. Bits 47:28 of LBA > shall be cleared to zero." > > Table 96 - "HPA Normal Output" Specifies the line output for this > command. Error, Count, LBA, Device, Status. These line outputs are for > sure the information that is bundled up into the D2H FIS. > > On this basis, then, is the reasoning behind this patch. Makes sense to me, thanks. Since you already got competent review, I won't review further, unless you ask for it.
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 8978643..0529d68 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -599,6 +599,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) uint8_t *pio_fis, *cmd_fis; uint64_t tbl_addr; dma_addr_t cmd_len = 0x80; + IDEState *s = &ad->port.ifs[0]; if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; @@ -628,21 +629,21 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) pio_fis[0] = 0x5f; pio_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0); - pio_fis[2] = ad->port.ifs[0].status; - pio_fis[3] = ad->port.ifs[0].error; - - pio_fis[4] = cmd_fis[4]; - pio_fis[5] = cmd_fis[5]; - pio_fis[6] = cmd_fis[6]; - pio_fis[7] = cmd_fis[7]; - pio_fis[8] = cmd_fis[8]; - pio_fis[9] = cmd_fis[9]; - pio_fis[10] = cmd_fis[10]; - pio_fis[11] = cmd_fis[11]; + pio_fis[2] = s->status; + pio_fis[3] = s->error; + + pio_fis[4] = s->sector; + pio_fis[5] = s->lcyl; + pio_fis[6] = s->hcyl; + pio_fis[7] = s->select; + pio_fis[8] = s->hob_sector; + pio_fis[9] = s->hob_lcyl; + pio_fis[10] = s->hob_hcyl; + pio_fis[11] = 0; pio_fis[12] = cmd_fis[12]; pio_fis[13] = cmd_fis[13]; pio_fis[14] = 0; - pio_fis[15] = ad->port.ifs[0].status; + pio_fis[15] = s->status; pio_fis[16] = len & 255; pio_fis[17] = len >> 8; pio_fis[18] = 0; @@ -669,6 +670,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) int i; dma_addr_t cmd_len = 0x80; int cmd_mapped = 0; + IDEState *s = &ad->port.ifs[0]; if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; @@ -686,17 +688,17 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) d2h_fis[0] = 0x34; d2h_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0); - d2h_fis[2] = ad->port.ifs[0].status; - d2h_fis[3] = ad->port.ifs[0].error; - - d2h_fis[4] = cmd_fis[4]; - d2h_fis[5] = cmd_fis[5]; - d2h_fis[6] = cmd_fis[6]; - d2h_fis[7] = cmd_fis[7]; - d2h_fis[8] = cmd_fis[8]; - d2h_fis[9] = cmd_fis[9]; - d2h_fis[10] = cmd_fis[10]; - d2h_fis[11] = cmd_fis[11]; + d2h_fis[2] = s->status; + d2h_fis[3] = s->error; + + d2h_fis[4] = s->sector; + d2h_fis[5] = s->lcyl; + d2h_fis[6] = s->hcyl; + d2h_fis[7] = s->select; + d2h_fis[8] = s->hob_sector; + d2h_fis[9] = s->hob_lcyl; + d2h_fis[10] = s->hob_hcyl; + d2h_fis[11] = 0; d2h_fis[12] = cmd_fis[12]; d2h_fis[13] = cmd_fis[13]; for (i = 14; i < 20; i++) {
Currently, the D2H FIS packets AHCI generates simply parrot back the LBA that the guest sent to us in the cmd_fis. However, some commands (like READ NATIVE MAX) modify the LBA registers as a return value, through which the AHCI D2H FIS is the only response mechanism. Thus, the D2H response should use the current register values, not the initial ones. This patch adjusts the LBA and drive select register responses for PIO Setup and D2H FIS response packets. Additionally, the PIO and D2H FIS responses copy too many bytes from the command FIS that it is being generated from. Specifically, byte 11 which is the Features(15:8) field for Register Host to Device FIS packets, is instead reserved for the PIO Setup FIS and should always be 0. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 48 +++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 23 deletions(-)