[Qemu-devel] [PATCH 0/2] ahci: fix atapi PIO (for 2.4?)

[Qemu-devel] [PATCH 0/2] ahci: fix atapi PIO (for 2.4?)

[John Snow]

ATAPI transfers using the AHCI HBA will work if you ask the HBA to perform
a DMA transfer, but if you ask for PIO, it will not.

All AHCI transfers are actually DMA anyway, but the AHCI HBA will need some
assistance from the core layer to help it manage its covert-ops DMA when
performing PIO routines deep in the bowels of the ATAPI code.

This is strictly a bugfix and /maybe/ it's 2.4-rc1 material, but the CDROM
tests for AHCI are not ready for prime-time (we don't have ANY cdrom tests
yet, really!) so I am submitting just the fix now, and my say-so that yes,
this fixes OVMF booting and pleases my hacky, off-list CDROM qtests.
________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-atapi-pio
https://github.com/jnsnow/qemu/tree/ahci-atapi-pio

This version is tagged ahci-atapi-pio-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-atapi-pio-v1

John Snow (2):
  atapi: abort transfers with 0 byte limits
  ide: unify io_buffer_offset increments

 hw/ide/ahci.c     | 22 +++++++---------------
 hw/ide/atapi.c    | 10 +++++++++-
 hw/ide/core.c     |  7 +++----
 hw/ide/internal.h |  2 ++
 4 files changed, 21 insertions(+), 20 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[John Snow]

We're supposed to abort on transfers like this, unless we fill
Word 125 of our IDENTIFY data with a default transfer size, which
we don't currently do.

This is an ATA error, not a SCSI/ATAPI one.
See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.

If we don't do this, QEMU will loop forever trying to transfer
zero bytes, which isn't particularly useful.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c    | 10 +++++++++-
 hw/ide/core.c     |  2 +-
 hw/ide/internal.h |  1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 950e311..1efdefc 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -213,8 +213,16 @@ void ide_atapi_cmd_reply_end(IDEState *s)
 #ifdef DEBUG_IDE_ATAPI
             printf("byte_count_limit=%d\n", byte_count_limit);
 #endif
-            if (byte_count_limit == 0xffff)
+            if (byte_count_limit == 0x00) {
+                /* This command aborts at the /ATA/ level, not the ATAPI level.
+                 * See ATA8 ACS3 section 7.17.6.49 and 7.21.5 */
+                ide_abort_command(s);
+                return;
+            } else if (byte_count_limit == 0xffff) {
+                /* ATA8 ACS3 7.21.5 */
                 byte_count_limit--;
+            }
+
             size = s->packet_transfer_size;
             if (size > byte_count_limit) {
                 /* byte count limit must be even if this case */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 122e955..a3a8365 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -457,7 +457,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
     return &iocb->common;
 }
 
-static inline void ide_abort_command(IDEState *s)
+void ide_abort_command(IDEState *s)
 {
     ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 30fdcbc..40e1aa4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -537,6 +537,7 @@ void ide_set_sector(IDEState *s, int64_t sector_num);
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
 void ide_dma_error(IDEState *s);
+void ide_abort_command(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
 void ide_atapi_cmd_error(IDEState *s, int sense_key, int asc);
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] ide: unify io_buffer_offset increments

[John Snow]

This variable was originally added to keep track of offsets in AHCI
rather exclusively, but it was added to IDEState. AHCI fakes all PIO
transfers using DMA and a scatter-gather list. When the core or atapi
layers invoke HBA-specific mechanisms for transfers, they do not always
know that it is being backed by DMA or a sglist, so this offset is not
always updated by all the code everywhere.

If we modify it in dma_buf_commit, however, any HBA that needs to use
this offset to manage operating on only part of a sglist will have
access to it.

As a result, this will fix ATAPI PIO transfers performed through the AHCI
HBA, which were previously not modifying this value appropriately. This
should fix AHCI/ATAPI with OVMF, which tries to use these PIO transfers.

Reported-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 22 +++++++---------------
 hw/ide/core.c     |  5 ++---
 hw/ide/internal.h |  1 +
 3 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bb6a92f..15427f4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -50,7 +50,6 @@ 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, int32_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);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
@@ -1285,7 +1284,7 @@ out:
     s->data_ptr = s->data_end;
 
     /* Update number of transferred bytes, destroy sglist */
-    ahci_commit_buf(dma, size);
+    dma_buf_commit(s, size);
 
     s->end_transfer_func(s);
 
@@ -1327,9 +1326,8 @@ static void ahci_restart(IDEDMA *dma)
 }
 
 /**
- * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
- * Not currently invoked by PIO R/W chains,
- * which invoke ahci_populate_sglist via ahci_start_transfer.
+ * Called in DMA and PIO R/W chains to read the PRDT.
+ * Not shared with NCQ pathways.
  */
 static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
 {
@@ -1348,21 +1346,16 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit)
 }
 
 /**
- * Destroys the scatter-gather list,
- * and updates the command header with a bytes-read value.
- * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
- * and ahci_start_transfer (PIO R/W),
- * and called via callback from ide_dma_cb for DMA R/W paths.
+ * Updates the command header with a bytes-read value.
+ * Called via dma_buf_commit, for both DMA and PIO paths.
+ * sglist destruction is handled within dma_buf_commit.
  */
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    IDEState *s = &ad->port.ifs[0];
 
     tx_bytes += le32_to_cpu(ad->cur_cmd->status);
     ad->cur_cmd->status = cpu_to_le32(tx_bytes);
-
-    qemu_sglist_destroy(&s->sg);
 }
 
 static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
@@ -1383,10 +1376,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
     }
 
     /* free sglist, update byte count */
-    ahci_commit_buf(dma, l);
+    dma_buf_commit(s, l);
 
     s->io_buffer_index += l;
-    s->io_buffer_offset += l;
 
     DPRINTF(ad->port_no, "len=%#x\n", l);
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index a3a8365..c249a37 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -591,7 +591,6 @@ static void ide_sector_read_cb(void *opaque, int ret)
     s->nsector -= n;
     /* Allow the guest to read the io_buffer */
     ide_transfer_start(s, s->io_buffer, n * BDRV_SECTOR_SIZE, ide_sector_read);
-    s->io_buffer_offset += 512 * n;
     ide_set_irq(s->bus);
 }
 
@@ -635,11 +634,12 @@ static void ide_sector_read(IDEState *s)
                                  ide_sector_read_cb, s);
 }
 
-static void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 {
     if (s->bus->dma->ops->commit_buf) {
         s->bus->dma->ops->commit_buf(s->bus->dma, tx_bytes);
     }
+    s->io_buffer_offset += tx_bytes;
     qemu_sglist_destroy(&s->sg);
 }
 
@@ -842,7 +842,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
         n = s->req_nb_sectors;
     }
     s->nsector -= n;
-    s->io_buffer_offset += 512 * n;
 
     ide_set_sector(s, ide_get_sector(s) + n);
     if (s->nsector == 0) {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 40e1aa4..05e93ff 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -536,6 +536,7 @@ int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *cb);
+void dma_buf_commit(IDEState *s, uint32_t tx_bytes);
 void ide_dma_error(IDEState *s);
 void ide_abort_command(IDEState *s);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[Kevin Wolf]

Am 11.07.2015 um 03:05 hat John Snow geschrieben:
> We're supposed to abort on transfers like this, unless we fill
> Word 125 of our IDENTIFY data with a default transfer size, which
> we don't currently do.
> 
> This is an ATA error, not a SCSI/ATAPI one.
> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
> 
> If we don't do this, QEMU will loop forever trying to transfer
> zero bytes, which isn't particularly useful.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Of course, one could argue that for a clean separation between the ATA
core and SCSI/ATAPI, this check should really be done in cmd_packet and
ide_abort_command() could stay static. And in fact, I do think it's
worthwhile to try getting a better separation in the mid term.

However, for a 2.4 fix, this patch might be the right thing to do.

The interesting part is that the SCSI command is already partially
handled before the error is returned. The required assumption here is
that the guest doesn't actually see any intermediate state between
issuing the command and getting the abort, so it doesn't notice that we
already started the command successfully. I _think_ that assumption
holds true.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[John Snow]

On 07/13/2015 06:18 AM, Kevin Wolf wrote:
> Am 11.07.2015 um 03:05 hat John Snow geschrieben:
>> We're supposed to abort on transfers like this, unless we fill
>> Word 125 of our IDENTIFY data with a default transfer size, which
>> we don't currently do.
>>
>> This is an ATA error, not a SCSI/ATAPI one.
>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
>>
>> If we don't do this, QEMU will loop forever trying to transfer
>> zero bytes, which isn't particularly useful.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Of course, one could argue that for a clean separation between the ATA
> core and SCSI/ATAPI, this check should really be done in cmd_packet and
> ide_abort_command() could stay static. And in fact, I do think it's
> worthwhile to try getting a better separation in the mid term.
> 
> However, for a 2.4 fix, this patch might be the right thing to do.
> 
> The interesting part is that the SCSI command is already partially
> handled before the error is returned. The required assumption here is
> that the guest doesn't actually see any intermediate state between
> issuing the command and getting the abort, so it doesn't notice that we
> already started the command successfully. I _think_ that assumption
> holds true.
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

Yes, in theory it should be handled in cmd_packet, but it needs to know
in advance all of the commands that obey the byte count limit and that
information didn't seem easy to extract from the specification.

This is indeed a hack, but it's effective for now.

--js

Re: [Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[John Snow]

On 07/13/2015 06:18 AM, Kevin Wolf wrote:
> Am 11.07.2015 um 03:05 hat John Snow geschrieben:
>> We're supposed to abort on transfers like this, unless we fill
>> Word 125 of our IDENTIFY data with a default transfer size, which
>> we don't currently do.
>>
>> This is an ATA error, not a SCSI/ATAPI one.
>> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
>>
>> If we don't do this, QEMU will loop forever trying to transfer
>> zero bytes, which isn't particularly useful.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Of course, one could argue that for a clean separation between the ATA
> core and SCSI/ATAPI, this check should really be done in cmd_packet and
> ide_abort_command() could stay static. And in fact, I do think it's
> worthwhile to try getting a better separation in the mid term.
> 
> However, for a 2.4 fix, this patch might be the right thing to do.
> 
> The interesting part is that the SCSI command is already partially
> handled before the error is returned. The required assumption here is
> that the guest doesn't actually see any intermediate state between
> issuing the command and getting the abort, so it doesn't notice that we
> already started the command successfully. I _think_ that assumption
> holds true.
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

Ping --

Do you still feel it's necessary we perform the abort in the IDE layer?
This requires us to peek into the CDB and determine if the CDB will (or
will not) transfer any data.

Is that sane? Is that worse than exporting ide_abort_command into the
ATAPI layer?

Pretty much either way, the ATA bits need to know about the SCSI packet,
or the ATAPI bits need to invoke core ATA functionality.

It's sort of gross, since the spec unfortunately allows for this field
to be zero if the command does not transfer data. :(

--js

Re: [Qemu-devel] [PATCH 1/2] atapi: abort transfers with 0 byte limits

[Kevin Wolf]

Am 25.08.2015 um 02:25 hat John Snow geschrieben:
> 
> 
> On 07/13/2015 06:18 AM, Kevin Wolf wrote:
> > Am 11.07.2015 um 03:05 hat John Snow geschrieben:
> >> We're supposed to abort on transfers like this, unless we fill
> >> Word 125 of our IDENTIFY data with a default transfer size, which
> >> we don't currently do.
> >>
> >> This is an ATA error, not a SCSI/ATAPI one.
> >> See ATA8-ACS3 sections 7.17.6.49 or 7.21.5.
> >>
> >> If we don't do this, QEMU will loop forever trying to transfer
> >> zero bytes, which isn't particularly useful.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> > 
> > Of course, one could argue that for a clean separation between the ATA
> > core and SCSI/ATAPI, this check should really be done in cmd_packet and
> > ide_abort_command() could stay static. And in fact, I do think it's
> > worthwhile to try getting a better separation in the mid term.
> > 
> > However, for a 2.4 fix, this patch might be the right thing to do.
> > 
> > The interesting part is that the SCSI command is already partially
> > handled before the error is returned. The required assumption here is
> > that the guest doesn't actually see any intermediate state between
> > issuing the command and getting the abort, so it doesn't notice that we
> > already started the command successfully. I _think_ that assumption
> > holds true.
> > 
> > Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> > 
> 
> Ping --
> 
> Do you still feel it's necessary we perform the abort in the IDE layer?

I can hardly say that it's necessary, but it might be desirable.

> This requires us to peek into the CDB and determine if the CDB will (or
> will not) transfer any data.
> 
> Is that sane? Is that worse than exporting ide_abort_command into the
> ATAPI layer?

No, core.c looking at the CDB is definitely not sane.

> Pretty much either way, the ATA bits need to know about the SCSI packet,
> or the ATAPI bits need to invoke core ATA functionality.
> 
> It's sort of gross, since the spec unfortunately allows for this field
> to be zero if the command does not transfer data. :(

The only reasonable way to do this that I can think of offhand would be
to return this information from ide_atapi_cmd().

Of course, the current structure of the code doesn't make this easy. But
in theory it can be done if (and only if) no asynchronous operation is
involved and the command is completed while ide_atapi_cmd() is running.
As I said in my review above, we already make that assumption; if it
weren't true, our current behaviour would be wrong.

In order to actually implement things this way, we would need to first
restructure the commands to move completion to ide_atapi_cmd() instead
of duplicating it in each command; I already mentioned this as a
possible improvement of the code in our private email thread.

Kevin