[Qemu-devel] [PATCH] ide: remove hardcoded 2GiB transactional limit

[Qemu-devel] [PATCH] ide: remove hardcoded 2GiB transactional limit

[John Snow]

Not that you can request a >2GiB transaction, but that's why checking
for it makes no sense anymore.

With the newer 'limit' parameter to prepare_buf, we no longer need a
static limit. The maximum limit is still 2GiB, but the limit parameter
is set to the current transaction size, which cannot surpass 32MiB
(512 * 65536). If the PRDT surpasses the transactional size, then,
we'll just carry out the normative underflow handling pathways instead
of needing an extra, strange pathway that worries about hitting some
logistical cap for the largest sglist we can support -- we'll never
even attempt to build one that big anymore.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 30 ++++++++++++++----------------
 hw/ide/internal.h |  2 +-
 hw/ide/pci.c      |  7 -------
 3 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 21f76ed..f547ebb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -804,8 +804,21 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl)
     return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
 }
 
+/**
+ * Fetch entries in a guest-provided PRDT and convert it into a QEMU SGlist.
+ * @ad: The AHCIDevice for whom we are building the SGList.
+ * @sglist: The SGList target to add PRD entries to.
+ * @cmd: The AHCI Command Header that describes where the PRDT is.
+ * @limit: The remaining size of the S/ATA transaction, in bytes.
+ * @offset: The number of bytes already transferred, in bytes.
+ *
+ * The AHCI PRDT can describe up to 256GiB. S/ATA only support transactions of
+ * up to 32MiB as of ATA8-ACS3 rev 1b, assuming a 512 byte sector size. We stop
+ * building the sglist from the PRDT as soon as we hit @limit bytes,
+ * which is <= INT32_MAX/2GiB.
+ */
 static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
-                                AHCICmdHdr *cmd, int64_t limit, int32_t offset)
+                                AHCICmdHdr *cmd, int64_t limit, uint64_t offset)
 {
     uint16_t opts = le16_to_cpu(cmd->opts);
     uint16_t prdtl = le16_to_cpu(cmd->prdtl);
@@ -823,14 +836,6 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     IDEBus *bus = &ad->port;
     BusState *qbus = BUS(bus);
 
-    /*
-     * Note: AHCI PRDT can describe up to 256GiB. SATA/ATA only support
-     * transactions of up to 32MiB as of ATA8-ACS3 rev 1b, assuming a
-     * 512 byte sector size. We limit the PRDT in this implementation to
-     * a reasonably large 2GiB, which can accommodate the maximum transfer
-     * request for sector sizes up to 32K.
-     */
-
     if (!prdtl) {
         DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);
         return -1;
@@ -880,13 +885,6 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
                             MIN(prdt_tbl_entry_size(&tbl[i]),
                                 limit - sglist->size));
-            if (sglist->size > INT32_MAX) {
-                error_report("AHCI Physical Region Descriptor Table describes "
-                             "more than 2 GiB.");
-                qemu_sglist_destroy(sglist);
-                r = -1;
-                goto out;
-            }
         }
     }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 05e93ff..e4629b0 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -397,7 +397,7 @@ struct IDEState {
     struct iovec iov;
     QEMUIOVector qiov;
     /* ATA DMA state */
-    int32_t io_buffer_offset;
+    uint64_t io_buffer_offset;
     int32_t io_buffer_size;
     QEMUSGList sg;
     /* PIO transfer handling */
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index d31ff88..9c54b37 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -103,13 +103,6 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int32_t limit)
                 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
-             * for LBA48 (65,536 sectors) and 32K sector sizes. */
-            if (s->sg.size > INT32_MAX) {
-                error_report("IDE: sglist describes more than 2GiB.");
-                break;
-            }
             bm->cur_prd_addr += l;
             bm->cur_prd_len -= l;
             s->io_buffer_size += l;
-- 
2.4.3

Re: [Qemu-devel] [PATCH] ide: remove hardcoded 2GiB transactional limit

[Stefan Hajnoczi]

On Mon, Oct 26, 2015 at 07:38:02PM -0400, John Snow wrote:
> Not that you can request a >2GiB transaction, but that's why checking
> for it makes no sense anymore.
> 
> With the newer 'limit' parameter to prepare_buf, we no longer need a
> static limit. The maximum limit is still 2GiB, but the limit parameter
> is set to the current transaction size, which cannot surpass 32MiB
> (512 * 65536). If the PRDT surpasses the transactional size, then,
> we'll just carry out the normative underflow handling pathways instead
> of needing an extra, strange pathway that worries about hitting some
> logistical cap for the largest sglist we can support -- we'll never
> even attempt to build one that big anymore.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c     | 30 ++++++++++++++----------------
>  hw/ide/internal.h |  2 +-
>  hw/ide/pci.c      |  7 -------
>  3 files changed, 15 insertions(+), 24 deletions(-)

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH] ide: remove hardcoded 2GiB transactional limit

[John Snow]

On 10/27/2015 12:50 PM, Stefan Hajnoczi wrote:
> On Mon, Oct 26, 2015 at 07:38:02PM -0400, John Snow wrote:
>> Not that you can request a >2GiB transaction, but that's why checking
>> for it makes no sense anymore.
>>
>> With the newer 'limit' parameter to prepare_buf, we no longer need a
>> static limit. The maximum limit is still 2GiB, but the limit parameter
>> is set to the current transaction size, which cannot surpass 32MiB
>> (512 * 65536). If the PRDT surpasses the transactional size, then,
>> we'll just carry out the normative underflow handling pathways instead
>> of needing an extra, strange pathway that worries about hitting some
>> logistical cap for the largest sglist we can support -- we'll never
>> even attempt to build one that big anymore.
>>
>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/ide/ahci.c     | 30 ++++++++++++++----------------
>>  hw/ide/internal.h |  2 +-
>>  hw/ide/pci.c      |  7 -------
>>  3 files changed, 15 insertions(+), 24 deletions(-)
> 
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js