| Message ID | 1412204151-18117-6-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/02/2014 12:55 AM, John Snow wrote: > + /* If this is true, you're leaking memory. */ ... or qsg is uninitialized, which would work because qemu_sglist_init initializes all fields. This is the only comment I have on the series. :) Paolo > + assert(qsg->sg == NULL); > + > qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry)); > > @@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) > s->io_buffer_size = s->sg.size; > > DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size); > - return s->io_buffer_size != 0; > + return s->io_buffer_size / 512 != 0; > }
On 10/27/2014 06:06 AM, Paolo Bonzini wrote: > > > On 10/02/2014 12:55 AM, John Snow wrote: >> + /* If this is true, you're leaking memory. */ > > ... or qsg is uninitialized, which would work because qemu_sglist_init > initializes all fields. > > This is the only comment I have on the series. :) > > Paolo > >> + assert(qsg->sg == NULL); >> + >> qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry)); > >> >> @@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) >> s->io_buffer_size = s->sg.size; >> >> DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size); >> - return s->io_buffer_size != 0; >> + return s->io_buffer_size / 512 != 0; >> } oh, yeah :\ I really did want to guard against re-initialization, but if it's expected that this structure may have completely anything in it at init time, I don't really have a way to do that, do I. I guess I'll just delete the assertion, unless you have a very simple idea to help guard against double-inits. --j
diff --git a/dma-helpers.c b/dma-helpers.c index 7f86e18..f51d6ee 100644 --- a/dma-helpers.c +++ b/dma-helpers.c @@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len) void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint, AddressSpace *as) { + /* If this is true, you're leaking memory. */ + assert(qsg->sg == NULL); + qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry)); qsg->nsg = 0; qsg->nalloc = alloc_hint; diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 16cd248..67c1e36 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) s->io_buffer_size = s->sg.size; DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size); - return s->io_buffer_size != 0; + return s->io_buffer_size / 512 != 0; } /** diff --git a/hw/ide/core.c b/hw/ide/core.c index 7c1929e..82d01e8 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -732,6 +732,7 @@ void ide_dma_cb(void *opaque, int ret) /* The PRDs were too short. Reset the Active bit, but don't raise an * interrupt. */ s->status = READY_STAT | SEEK_STAT; + dma_buf_commit(s, false); goto eot; } diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 2397f35..3f643c2 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write) if (bm->cur_prd_len == 0) { /* end of table (with a fail safe of one page) */ if (bm->cur_prd_last || - (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) - return s->io_buffer_size != 0; + (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) { + return (s->io_buffer_size / 512) != 0; + } pci_dma_read(pci_dev, bm->cur_addr, &prd, 8); bm->cur_addr += 8; prd.addr = le32_to_cpu(prd.addr);
This impacts both BMDMA and AHCI HBA interfaces for IDE. Currently, we confuse the difference between a PRD having "0 bytes" and a PRD having "0 complete sectors." This leads to, in the BMDMA case, leaked memory for short PRDTs, and infinite loops in the AHCI case. the "prepare_buf" callback is reworked to return 0 if it could not allocate a full sector's worth of buffer space, instead of returning non-zero if it allocated any number of bytes. ide_dma_cb adds a call to commit_buf in order to delete the short PRDT that it will not attempt to use to finish the DMA operation. This patch corrects both occurrences and adds an assertion to prevent future regression. This assertion is tested in the existing ide-test, and is covered in a forthcoming AHCI test. Signed-off-by: John Snow <jsnow@redhat.com> --- dma-helpers.c | 3 +++ hw/ide/ahci.c | 2 +- hw/ide/core.c | 1 + hw/ide/pci.c | 5 +++-- 4 files changed, 8 insertions(+), 3 deletions(-)