| Message ID | 1434765047-29333-5-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote: > @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, > ((uint64_t)ncq_fis->lba2 << 16) | > ((uint64_t)ncq_fis->lba1 << 8) | > (uint64_t)ncq_fis->lba0; > + ncq_tfs->tag = tag; > > - /* Note: We calculate the sector count, but don't currently rely on it. > - * The total size of the DMA buffer tells us the transfer size instead. */ > ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | > ncq_fis->sector_count_low; > + ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); > + size = ncq_tfs->sector_count * 512; ncq_tfs->sector_count is used with - 2 and - 1 below. What is the semantics of this field and why is it okay to use it without subtracting here?
On 06/22/2015 10:06 AM, Stefan Hajnoczi wrote: > On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote: >> @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState >> *s, int port, uint8_t *cmd_fis, ((uint64_t)ncq_fis->lba2 << 16) >> | ((uint64_t)ncq_fis->lba1 << 8) | (uint64_t)ncq_fis->lba0; + >> ncq_tfs->tag = tag; >> >> - /* Note: We calculate the sector count, but don't currently >> rely on it. - * The total size of the DMA buffer tells us the >> transfer size instead. */ ncq_tfs->sector_count = >> ((uint16_t)ncq_fis->sector_count_high << 8) | >> ncq_fis->sector_count_low; + ahci_populate_sglist(ad, >> &ncq_tfs->sglist, 0); + size = ncq_tfs->sector_count * 512; > > ncq_tfs->sector_count is used with - 2 and - 1 below. What is the > semantics of this field and why is it okay to use it without > subtracting here? > Just a bonkers patch order. Patch #7 fixes the debug prints, which are wrong. The field is not "off by one" in the spec, sector_count == 1 means 1 sector. (sector_count == 0 means 64K sectors, though, like it does in regular ATA.)
On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote: > - /* Note: We calculate the sector count, but don't currently rely on it. > - * The total size of the DMA buffer tells us the transfer size instead. */ > ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | > ncq_fis->sector_count_low; > + ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); > + size = ncq_tfs->sector_count * 512; I just saw that a later patch addresses the sector_count inconsistency, so I'm happy with this now.
On Mon, Jun 22, 2015 at 10:08:22AM -0400, John Snow wrote: > (sector_count == 0 means 64K sectors, though, like it does in regular > ATA.) I don't see a case to handle this.
On 06/23/2015 09:46 AM, Stefan Hajnoczi wrote: > On Mon, Jun 22, 2015 at 10:08:22AM -0400, John Snow wrote: >> (sector_count == 0 means 64K sectors, though, like it does in regular >> ATA.) > > I don't see a case to handle this. > I apologize: Corrected in [PATCH 09/16] ahci: correct ncq sector count, in the "part 2" series. Sorry for the confusing order, I played a lot of patch-reshuffling and due to the refactors to get NCQ migrating, sometimes it was literally just a sin of convenience to leave certain patches after the shuffle. I tried to leave each patch fixing just one very small issue at a time so they were easier to digest, instead of batching them together like "Fix (everything wrong about) sector count." Maybe that wasn't the right approach, you can let me know. Thank you, --John
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 375aa44..24c4832 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -983,6 +983,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, NCQFrame *ncq_fis = (NCQFrame*)cmd_fis; uint8_t tag = ncq_fis->tag >> 3; NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag]; + size_t size; if (ncq_tfs->used) { /* error - already in use */ @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ((uint64_t)ncq_fis->lba2 << 16) | ((uint64_t)ncq_fis->lba1 << 8) | (uint64_t)ncq_fis->lba0; + ncq_tfs->tag = tag; - /* Note: We calculate the sector count, but don't currently rely on it. - * The total size of the DMA buffer tells us the transfer size instead. */ ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | ncq_fis->sector_count_low; + ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); + size = ncq_tfs->sector_count * 512; + + if (ncq_tfs->sglist.size < size) { + error_report("ahci: PRDT length for NCQ command (0x%zx) " + "is smaller than the requested size (0x%zx)", + ncq_tfs->sglist.size, size); + qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW); + return; + } DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", " "drive max %"PRId64"\n", ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2, ide_state->nb_sectors - 1); - ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); - ncq_tfs->tag = tag; - switch(ncq_fis->command) { case READ_FPDMA_QUEUED: DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", " @@ -1051,6 +1060,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, "error: tried to process non-NCQ command as NCQ\n"); } qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); } }
Don't attempt the NCQ transfer if the PRDT we were given is not big enough to perform the entire transfer. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)