From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z9fVA-0003RS-Ff for qemu-devel@nongnu.org; Mon, 29 Jun 2015 16:18:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z9fV9-0003Qt-4S for qemu-devel@nongnu.org; Mon, 29 Jun 2015 16:18:00 -0400 Message-ID: <5591A7F0.1060107@redhat.com> Date: Mon, 29 Jun 2015 16:17:52 -0400 From: John Snow MIME-Version: 1.0 References: <1435608966-12135-1-git-send-email-jsnow@redhat.com> In-Reply-To: <1435608966-12135-1-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC] ide: fix bmdma underflow code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel Cc: qemu-block@nongnu.org, stefanha@redhat.com On 06/29/2015 04:16 PM, John Snow wrote: > This RFC requires my ahci-ncq-s2 patchset and all of its dependencies. > > Fix the BMDMA underflow code to acknowledge the new 'limit' parameter, > without breaking or modifying the existing ide-tests. > > This approach uses s->io_buffer_size as a return value to mean the > number of bytes witnessed in the PRDs, but the return code is the number > of actual bytes prepared to the sglist. > > With io_buffer_size retaining the same value as it would have prior to > the patch, the existing pathways for detecting underflow for BMDMA > continue to work and set register values as expected. > > --- > hw/ide/ahci.c | 6 +++--- > hw/ide/core.c | 9 ++++----- > hw/ide/internal.h | 2 +- > hw/ide/macio.c | 2 +- > hw/ide/pci.c | 24 ++++++++++++++++-------- > 5 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index aac5597..d891506 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -49,7 +49,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot); > static void ahci_reset_port(AHCIState *s, int port); > static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis); > static void ahci_init_d2h(AHCIDevice *ad); > -static int ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write); > +static int ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit); > static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes); > static bool ahci_map_clb_address(AHCIDevice *ad); > static bool ahci_map_fis_address(AHCIDevice *ad); > @@ -1263,7 +1263,7 @@ static void ahci_start_transfer(IDEDMA *dma) > goto out; > } > > - if (ahci_dma_prepare_buf(dma, size, is_write)) { > + if (ahci_dma_prepare_buf(dma, size)) { > has_sglist = 1; > } > > @@ -1330,7 +1330,7 @@ static void ahci_restart(IDEDMA *dma) > * Not currently invoked by PIO R/W chains, > * which invoke ahci_populate_sglist via ahci_start_transfer. > */ > -static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) > +static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit) > { > AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); > IDEState *s = &ad->port.ifs[0]; > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 8c271cc..6bcf07c 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -716,8 +716,8 @@ static void ide_dma_cb(void *opaque, int ret) > > sector_num = ide_get_sector(s); > if (n > 0) { > - assert(s->io_buffer_size == s->sg.size); > - dma_buf_commit(s, s->io_buffer_size); > + assert(s->nsector * 512 == s->sg.size); > + dma_buf_commit(s, s->sg.size); > sector_num += n; > ide_set_sector(s, sector_num); > s->nsector -= n; > @@ -734,8 +734,7 @@ static void ide_dma_cb(void *opaque, int ret) > n = s->nsector; > s->io_buffer_index = 0; > s->io_buffer_size = n * 512; > - if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size, > - ide_cmd_is_read(s)) < 512) { > + if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) { > /* The PRDs were too short. Reset the Active bit, but don't raise an > * interrupt. */ > s->status = READY_STAT | SEEK_STAT; > @@ -2327,7 +2326,7 @@ static void ide_nop(IDEDMA *dma) > { > } > > -static int32_t ide_nop_int32(IDEDMA *dma, int64_t l, int x) > +static int32_t ide_nop_int32(IDEDMA *dma, int64_t l) > { > return 0; > } > diff --git a/hw/ide/internal.h b/hw/ide/internal.h > index adb968c..efc2a90 100644 > --- a/hw/ide/internal.h > +++ b/hw/ide/internal.h > @@ -324,7 +324,7 @@ typedef void EndTransferFunc(IDEState *); > typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *); > typedef void DMAVoidFunc(IDEDMA *); > typedef int DMAIntFunc(IDEDMA *, int); > -typedef int32_t DMAInt32Func(IDEDMA *, int64_t len, int is_write); > +typedef int32_t DMAInt32Func(IDEDMA *, int64_t len); > typedef void DMAu32Func(IDEDMA *, uint32_t); > typedef void DMAStopFunc(IDEDMA *, bool); > typedef void DMARestartFunc(void *, int, RunState); > diff --git a/hw/ide/macio.c b/hw/ide/macio.c > index 1bd1580..ec14e5b 100644 > --- a/hw/ide/macio.c > +++ b/hw/ide/macio.c > @@ -499,7 +499,7 @@ static int ide_nop_int(IDEDMA *dma, int x) > return 0; > } > > -static int32_t ide_nop_int32(IDEDMA *dma, int64_t l, int x) > +static int32_t ide_nop_int32(IDEDMA *dma, int64_t l) > { > return 0; > } > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index a295baa..afdbda8 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -53,13 +53,14 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s, > } > > /** > - * Return the number of bytes successfully prepared. > - * -1 on error. > - * BUG?: Does not currently heed the 'limit' parameter because > - * it is not clear what the correct behavior here is, > - * see tests/ide-test.c > + * Prepare an sglist based on available PRDs. > + * @limit: How many bytes to prepare total. > + * > + * Returns the number of bytes prepared, -1 on error. > + * IDEState.io_buffer_size will contain the number of bytes described > + * by the PRDs, whether or not we added them to the sglist. > */ > -static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) > +static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit) > { > BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma); > IDEState *s = bmdma_active_if(bm); > @@ -78,7 +79,7 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) > /* 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; > + return s->sg.size; > } > pci_dma_read(pci_dev, bm->cur_addr, &prd, 8); > bm->cur_addr += 8; > @@ -93,7 +94,14 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write) > } > l = bm->cur_prd_len; > if (l > 0) { > - qemu_sglist_add(&s->sg, bm->cur_prd_addr, l); > + uint64_t sg_len; > + > + /* Don't add extra bytes to the SGList; consume any remaining > + * PRDs from the guest, but ignore them. */ > + sg_len = MIN(limit - s->sg.size, bm->cur_prd_len); > + if (sg_len) { > + qemu_sglist_add(&s->sg, bm->cur_prd_addr, sg_len); > + } > > /* Note: We limit the max transfer to be 2GiB. > * This should accommodate the largest ATA transaction > Whoops, fat-fingered the email address here. subbing qemu@ for qemu-devel@. --js