Re: [Qemu-devel] [RFC] ide: fix bmdma underflow code

[John Snow]

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 address@hidden

--js