Il 13/09/2014 06:34, John Snow ha scritto:
> 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 <js...@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(-)
>
> diff --git a/dma-helpers.c b/dma-helpers.c
> index ba965a3..3f9766d 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 8e6a352..42a77c4 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1132,7 +1132,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 b2980e9..1685f6d 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -726,6 +726,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);
>
Reviewed-by: Paolo Bonzini <pbonz...@redhat.com>
The changes I suggested in patch 2 shouldn't be a hurdle here.
Paolo
