| Message ID | 1435018875-22527-15-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: > @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) > 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[12] = s->nsector & 0xFF; > + pio_fis[13] = (s->nsector >> 8) & 0xFF; hw/ide/core.c decreases s->nsector until it reaches 0 and the request ends. Will the values reported back to the guest be correct if we use s->nsector?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote: > On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: >> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice >> *ad, uint16_t len) 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[12] = s->nsector & 0xFF; >> + pio_fis[13] = (s->nsector >> 8) & 0xFF; > > hw/ide/core.c decreases s->nsector until it reaches 0 and the > request ends. > > Will the values reported back to the guest be correct if we use > s->nsector? > See the commit message for justification of this one. Ultimately, it doesn't matter what gets put in here (for data transfer commands) -- but getting RID of the cmd_fis mapping is a strong positive. The field is supposed to contain the contents of the sector count register at the conclusion of the command. The spec says that for data transfer commands, this value is useless/uninteresting (it can be zero-filled, or filled with garbage, etc) However, for some special purpose commands, the sector count register is required to hold a value that has some special meaning. I don't think we currently support any of those, but for the future -- this part of the FIS generation code will work correctly regardless. So this patch is mainly about getting rid of the cmd_fis mapping and putting something nominally correct in its place. - --js -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVjYxgAAoJEH3vgQaq/DkOYiMQAIUetgDGGd8QvntbT147pifE f8vLqV1TDZL2cmorplUT8K5BFPEOiSFn3GIa2K4sVmfZy9L7r/Q8SUn/WtV0AeLD /OkU7l25JIz62cSKpZZpfJcKsw9iBjRoeltxM1AfiS+GpUIMRxxORUEsgswEJQzn CPLkOOE+ME1Bh9qq6/kYj0+gPLwC2UjoW2xny1A5pizwdf4VlJl9jyM5DWMdtryW Qcd/djxK2CAXsvpMYNQnnjF7JDp+nkGaXct+hTQEUZRWbUv02+KOt9StBfBh+Dq1 njYq6pgUfvvbjOzccHTzgeaUlCCI6mP+ng0ksjG2HW9LI8QyV9whtPB/Rc2ORfhz R1zTw5JW6fsAVnbfkxBC7OFJBZB4WfmJWEdIPUmrmLgpxaBDi9jOK+bqYXeTTpXv bgdFJN2BAWXW1F4UCSjDaVUAL1FYG5vjxw+MbbWGcdHTFPT6z3OZVxe1AST7SS3j xuqIVMaxBeskQgUJiwOBFSFkTMTpWPIfRLY1MLk9yqXuwQpg7NtQncbho3wCj1cC nxpjDjWRAB3a2odjCocA9RN6uBNeKq2WbmFzU6bODgjOHRFUPfP/s3qz43PPQGxa +VQZaQahOnvVIGWnerXMyl+LT/0tLhsBUZDeRoNbR3MMMprw3yxozP1BM1bQ4bWx brUOjknDPIwWfjfIpUID =SaS/ -----END PGP SIGNATURE-----
On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote: > On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote: > > On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: > >> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice > >> *ad, uint16_t len) 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[12] = s->nsector & 0xFF; > >> + pio_fis[13] = (s->nsector >> 8) & 0xFF; > > > > hw/ide/core.c decreases s->nsector until it reaches 0 and the > > request ends. > > > > Will the values reported back to the guest be correct if we use > > s->nsector? > > > > See the commit message for justification of this one. Ultimately, it > doesn't matter what gets put in here (for data transfer commands) -- > but getting RID of the cmd_fis mapping is a strong positive. Getting rid of cmd_fis mapping is good. Putting s->nsector into the undefined fields makes the code confusing. It is clearer to zero the bytes with a comment saying the value does not matter according to the spec.
On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote: > On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote: >> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote: >>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: >>>> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice >>>> *ad, uint16_t len) 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[12] = s->nsector & 0xFF; >>>> + pio_fis[13] = (s->nsector >> 8) & 0xFF; >>> >>> hw/ide/core.c decreases s->nsector until it reaches 0 and the >>> request ends. >>> >>> Will the values reported back to the guest be correct if we use >>> s->nsector? >>> >> >> See the commit message for justification of this one. Ultimately, it >> doesn't matter what gets put in here (for data transfer commands) -- >> but getting RID of the cmd_fis mapping is a strong positive. > > Getting rid of cmd_fis mapping is good. > > Putting s->nsector into the undefined fields makes the code confusing. > > It is clearer to zero the bytes with a comment saying the value does not > matter according to the spec. > Well, it's not that it doesn't matter /ever/, it's more that for standard IO routines it doesn't matter. (See the normative output spec in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it carries a diagnostic signature.) What's really the case is that the FIS always dutifully copies out what the SATA registers are (or should be.) There are still a handful of commands that, if we choose to support them, copying the nsector register would be the "correct thing" to do, so I decided to copy that field here to serve as documentation and support future command additions. I would argue that if this field ever does the /wrong thing/, it would be a fix in the S/ATA layer, and not a change to the FIS generator here. I am inclined to leave it as-is, since for the current cases, nsector is going to empty to zero anyway. I believe the behavior presented here is correct. --js
On Mon, Jun 29, 2015 at 11:07:18AM -0400, John Snow wrote: > On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote: > > On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote: > >> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote: > >>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote: > >>>> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice > >>>> *ad, uint16_t len) 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[12] = s->nsector & 0xFF; > >>>> + pio_fis[13] = (s->nsector >> 8) & 0xFF; > >>> > >>> hw/ide/core.c decreases s->nsector until it reaches 0 and the > >>> request ends. > >>> > >>> Will the values reported back to the guest be correct if we use > >>> s->nsector? > >>> > >> > >> See the commit message for justification of this one. Ultimately, it > >> doesn't matter what gets put in here (for data transfer commands) -- > >> but getting RID of the cmd_fis mapping is a strong positive. > > > > Getting rid of cmd_fis mapping is good. > > > > Putting s->nsector into the undefined fields makes the code confusing. > > > > It is clearer to zero the bytes with a comment saying the value does not > > matter according to the spec. > > > > Well, it's not that it doesn't matter /ever/, it's more that for > standard IO routines it doesn't matter. (See the normative output spec > in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it > carries a diagnostic signature.) > > What's really the case is that the FIS always dutifully copies out what > the SATA registers are (or should be.) > > There are still a handful of commands that, if we choose to support > them, copying the nsector register would be the "correct thing" to do, > so I decided to copy that field here to serve as documentation and > support future command additions. > > I would argue that if this field ever does the /wrong thing/, it would > be a fix in the S/ATA layer, and not a change to the FIS generator here. > > I am inclined to leave it as-is, since for the current cases, nsector is > going to empty to zero anyway. I believe the behavior presented here is > correct. I'm trying to understand the guest-visible change in behavior. Guests might take different code paths from before. For example, I think that after this patch the CHECK POWER MODE command works correctly for the first time with AHCI. It sets ->nsector to 0xff. Anyway, I'm happy with assigning s->nsector.
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index eeb48fb..d344701 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -700,35 +700,13 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished) static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) { AHCIPortRegs *pr = &ad->port_regs; - uint8_t *pio_fis, *cmd_fis; - uint64_t tbl_addr; - dma_addr_t cmd_len = 0x80; + uint8_t *pio_fis; IDEState *s = &ad->port.ifs[0]; if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; } - /* map cmd_fis */ - tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr); - cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len, - DMA_DIRECTION_TO_DEVICE); - - if (cmd_fis == NULL) { - DPRINTF(ad->port_no, "dma_memory_map failed in ahci_write_fis_pio"); - ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR); - return; - } - - if (cmd_len != 0x80) { - DPRINTF(ad->port_no, - "dma_memory_map mapped too few bytes in ahci_write_fis_pio"); - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); - ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR); - return; - } - pio_fis = &ad->res_fis[RES_FIS_PSFIS]; pio_fis[0] = SATA_FIS_TYPE_PIO_SETUP; @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) 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[12] = s->nsector & 0xFF; + pio_fis[13] = (s->nsector >> 8) & 0xFF; pio_fis[14] = 0; pio_fis[15] = s->status; pio_fis[16] = len & 255; @@ -762,9 +740,6 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) } ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS); - - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); } static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) @@ -772,22 +747,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) AHCIPortRegs *pr = &ad->port_regs; uint8_t *d2h_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; } - if (!cmd_fis) { - /* map cmd_fis */ - uint64_t tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr); - cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len, - DMA_DIRECTION_TO_DEVICE); - cmd_mapped = 1; - } - d2h_fis = &ad->res_fis[RES_FIS_RFIS]; d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H; @@ -803,8 +768,8 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) 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]; + d2h_fis[12] = s->nsector & 0xFF; + d2h_fis[13] = (s->nsector >> 8) & 0xFF; for (i = 14; i < 20; i++) { d2h_fis[i] = 0; } @@ -818,11 +783,6 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) } ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS); - - if (cmd_mapped) { - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); - } } static int prdt_tbl_entry_size(const AHCI_SG *tbl)
The Register D2H FIS should copy the current values of the registers instead of just parroting back the same values the guest sent back to it. In this case, the SECTOR COUNT variables are actually not generally meaningful in terms of standard commands (See ATA8-AC3 Section 9.2 Normal Outputs), so it actually probably doesn't matter what we put in here. Meanwhile, we do need to use the Register update FIS from the NCQ pathways (in error cases), so getting rid of references to cur_cmd here is a win for AHCI concurrency. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 50 +++++--------------------------------------------- 1 file changed, 5 insertions(+), 45 deletions(-)