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

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 qemu-devel@.

--js

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

[John Snow]

On 07/01/2015 01:10 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 1, 2015 at 4:49 PM, John Snow <jsnow@redhat.com> wrote:
>> On 07/01/2015 06:18 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 29, 2015 at 04:16:06PM -0400, John Snow wrote:
>>>> 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);
>>>
>>> The short PRDT case:
>>>
>>
>> ...is handled "first," lower down in this callback. The first call to
>> ide_dma_cb occurs before we've prepared any bytes or set io_buffer_size.
>>
>> We'll cruise past these post-hoc checks because we didn't transfer
>> anything. We'll hit the explicit "too short" check during that first
>> call and will cease processing there.
>>
>> If the (n > 0) conditionals fire off here, it's because we've already
>> transferred data and we KNOW the PRDTs weren't too short for at least
>> one iteration of the DMA loop*
> 
> The case I described with nsector = 2 and PRDT having only 1 sector
> isn't handled before we hit the assertion failure:
> 
> qemu-system-x86_64: hw/ide/core.c:719: ide_dma_cb: Assertion
> `s->nsector * 512 == s->sg.size' failed.
> 

GOTCHA, it's because the short PRDT check doesn't check to see if it's
enough to satisfy the entire command, just at least one sector.

I forgot. You're right.

>> (*note that at present, AHCI only ever makes one DMA callback loop,
>> because we fire off the entire transfer in one shot, though the loop
>> is tolerant to multiple loops. Maybe other HBAs utilize that, but AHCI
>> doesn't. Regardless, with this patch no qtests or iotests fail, so
>> everything seems peachy.)
> 
> I have sent an ide-test.c patch with the test case that triggers the
> assertion failure.  It's a slightly different code path from the other
> short PRDT test case so it's useful to add it.
> 
>> Though I'm now realizing that what I really meant to write was
>>
>> assert(n * 512 == s->sg.size);
> 
> Looks good.
>