From a395f3fa2f26c94dac03b37e3dfb1074bfe2ddea Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 10 Nov 2014 19:41:40 -0500 Subject: [PATCH 01/11] ahci: Fix byte count regression for ATAPI/PIO This patch fixes a regression caused by commit 659142ecf71a0da240ab0ff7cf929ee25c32b9bc. The problem occurs when we wish to return early from the ahci_start_transfer function, but are now updating the transferred byte count in the AHCI command header via ahci_commit_buf. This will cause problems in the Windows 8 installer. Don't update the byte count in the command header for the transmission of ATAPI packets: These commands will distort the final byte count of the actual data payload. The call to ahci_commit_buf remains in the "out" portion of the call in order to clean up the sglist. The byte count is maintained by forcing size to be 0. Signed-off-by: John Snow Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 61dbed1b97..1f3f951b50 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1089,6 +1089,7 @@ static void ahci_start_transfer(IDEDMA *dma) if (is_atapi && !ad->done_atapi_packet) { /* already prepopulated iobuffer */ ad->done_atapi_packet = true; + size = 0; goto out; } From 36334faf35ccc48d61ca3431a5c0787b125dd306 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 31 Oct 2014 16:03:37 -0400 Subject: [PATCH 02/11] ide: repair PIO transfers for cases where nsector > 1 Currently, for emulated PIO transfers through the AHCI device, any attempt made to request more than a single sector's worth of data will result in the same sector being transferred over and over. For example, if we request 8 sectors via PIO READ SECTORS, the AHCI device will give us the same sector eight times. This patch adds offset tracking into the PIO pathways so that we can fulfill these requests appropriately. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1414785819-26209-2-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 2 +- hw/ide/core.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 1f3f951b50..dbd6773f8e 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1093,7 +1093,7 @@ static void ahci_start_transfer(IDEDMA *dma) goto out; } - if (!ahci_populate_sglist(ad, &s->sg, 0)) { + if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) { has_sglist = 1; } diff --git a/hw/ide/core.c b/hw/ide/core.c index d316ccf961..dab21f06c3 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -592,6 +592,7 @@ static void ide_sector_read_cb(void *opaque, int ret) ide_set_sector(s, ide_get_sector(s) + n); s->nsector -= n; + s->io_buffer_offset += 512 * n; } void ide_sector_read(IDEState *s) @@ -832,6 +833,8 @@ static void ide_sector_write_cb(void *opaque, int ret) n = s->req_nb_sectors; } s->nsector -= n; + s->io_buffer_offset += 512 * n; + if (s->nsector == 0) { /* no more sectors to write */ ide_transfer_stop(s); @@ -1824,6 +1827,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) s->status = READY_STAT | BUSY_STAT; s->error = 0; + s->io_buffer_offset = 0; complete = ide_cmd_table[val].handler(s, val); if (complete) { From bef1301acb74d177b42890116e4eeaf26047b9e3 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 31 Oct 2014 16:03:38 -0400 Subject: [PATCH 03/11] ahci: unify sglist preparation The intent of this patch is to further unify the creation and deletion of the sglist used for all AHCI transfers, including emulated PIO, ATAPI R/W, and native DMA R/W. By replacing ahci_start_transfer's call to ahci_populate_sglist with ahci_dma_prepare_buf, we reduce the number of direct calls where we manipulate the scatter-gather list in the AHCI code. To make this switch, the constant "0" passed as an offset in ahci_dma_prepare_buf is adjusted to use io_buffer_offset. For DMA pathways, this has no effect: io_buffer_offset is always updated to 0 at the beginning of a DMA transfer loop regardless. DMA pathways through ide_dma_cb() update the io_buffer_offset accordingly, and for circumstances where we might make several trips through this loop, this may actually correct a design flaw. For PIO pathways, the newly updated ahci_dma_prepare_buf will now prepare the sglist at the correct offset. It will also set io_buffer_size, but this is not used in the cmd_read_pio or cmd_write_pio pathways. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1414785819-26209-3-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index dbd6773f8e..28aa1055dd 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1093,7 +1093,7 @@ static void ahci_start_transfer(IDEDMA *dma) goto out; } - if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) { + if (ahci_dma_prepare_buf(dma, is_write)) { has_sglist = 1; } @@ -1145,7 +1145,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; - ahci_populate_sglist(ad, &s->sg, 0); + ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset); s->io_buffer_size = s->sg.size; DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size); From 3251bdcf1c67427d964517053c3d185b46e618e8 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 31 Oct 2014 16:03:39 -0400 Subject: [PATCH 04/11] ide: Correct handling of malformed/short PRDTs This impacts both BMDMA and AHCI HBA interfaces for IDE. Currently, we confuse the difference between a PRDT having "0 bytes" and a PRDT having "0 complete sectors." When we receive an incomplete sector, inconsistent error checking leads to an infinite loop wherein the call succeeds, but it didn't give us enough bytes -- leading us to re-call the DMA chain over and over again. This leads to, in the BMDMA case, leaked memory for short PRDTs, and infinite loops and resource usage in the AHCI case. The .prepare_buf() callback is reworked to return the number of bytes that it successfully prepared. 0 is a valid, non-error answer that means the table was empty and described no bytes. -1 indicates an error. Our current implementation uses the io_buffer in IDEState to ultimately describe the size of a prepared scatter-gather list. Even though the AHCI PRDT/SGList can be as large as 256GiB, the AHCI command header limits transactions to just 4GiB. ATA8-ACS3, however, defines the largest transaction to be an LBA48 command that transfers 65,536 sectors. With a 512 byte sector size, this is just 32MiB. Since our current state structures use the int type to describe the size of the buffer, and this state is migrated as int32, we are limited to describing 2GiB buffer sizes unless we change the migration protocol. For this reason, this patch begins to unify the assertions in the IDE pathways that the scatter-gather list provided by either the AHCI PRDT or the PCI BMDMA PRDs can only describe, at a maximum, 2GiB. This should be resilient enough unless we need a sector size that exceeds 32KiB. Further, the likelihood of any guest operating system actually attempting to transfer this much data in a single operation is very slim. To this end, the IDEState variables have been updated to more explicitly clarify our maximum supported size. Callers to the prepare_buf callback have been reworked to understand the new return code, and all versions of the prepare_buf callback have been adjusted accordingly. Lastly, the ahci_populate_sglist helper, relied upon by the AHCI implementation of .prepare_buf() as well as the PCI implementation of the callback have had overflow assertions added to help make clear the reasonings behind the various type changes. [Added %d -> %"PRId64" fix John sent because off_pos changed from int to int64_t. --Stefan] Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1414785819-26209-4-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 33 ++++++++++++++++++++++++++------- hw/ide/core.c | 10 ++++++++-- hw/ide/internal.h | 13 +++++++------ hw/ide/macio.c | 7 ++++++- hw/ide/pci.c | 27 +++++++++++++++++++++------ 5 files changed, 68 insertions(+), 22 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 28aa1055dd..9647d94d9b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -730,7 +730,8 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl) return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1; } -static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset) +static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, + int32_t offset) { AHCICmdHdr *cmd = ad->cur_cmd; uint32_t opts = le32_to_cpu(cmd->opts); @@ -741,13 +742,21 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset) uint8_t *prdt; int i; int r = 0; - int sum = 0; + uint64_t sum = 0; int off_idx = -1; - int off_pos = -1; + int64_t off_pos = -1; int tbl_entry_size; 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 (!sglist_alloc_hint) { DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts); return -1; @@ -782,7 +791,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset) } if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) { DPRINTF(ad->port_no, "%s: Incorrect offset! " - "off_idx: %d, off_pos: %d\n", + "off_idx: %d, off_pos: %"PRId64"\n", __func__, off_idx, off_pos); r = -1; goto out; @@ -797,6 +806,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset) /* flags_size is zero-based */ qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr), prdt_tbl_entry_size(&tbl[i])); + if (sglist->size > INT32_MAX) { + error_report("AHCI Physical Region Descriptor Table describes " + "more than 2 GiB.\n"); + qemu_sglist_destroy(sglist); + r = -1; + goto out; + } } } @@ -1140,16 +1156,19 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s, * Not currently invoked by PIO R/W chains, * which invoke ahci_populate_sglist via ahci_start_transfer. */ -static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write) +static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; - ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset); + if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset) == -1) { + DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n"); + return -1; + } s->io_buffer_size = s->sg.size; DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size); - return s->io_buffer_size != 0; + return s->io_buffer_size; } /** diff --git a/hw/ide/core.c b/hw/ide/core.c index dab21f06c3..00e21cf7ef 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -731,10 +731,11 @@ 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, ide_cmd_is_read(s)) == 0) { + if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) { /* The PRDs were too short. Reset the Active bit, but don't raise an * interrupt. */ s->status = READY_STAT | SEEK_STAT; + dma_buf_commit(s, 0); goto eot; } @@ -2313,12 +2314,17 @@ static int ide_nop_int(IDEDMA *dma, int x) return 0; } +static int32_t ide_nop_int32(IDEDMA *dma, int x) +{ + return 0; +} + static void ide_nop_restart(void *opaque, int x, RunState y) { } static const IDEDMAOps ide_dma_nop_ops = { - .prepare_buf = ide_nop_int, + .prepare_buf = ide_nop_int32, .rw_buf = ide_nop_int, .set_unit = ide_nop_int, .restart_cb = ide_nop_restart, diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 907493d0f8..8a3eca40d2 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -322,6 +322,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 *, int); typedef void DMAu32Func(IDEDMA *, uint32_t); typedef void DMAStopFunc(IDEDMA *, bool); typedef void DMARestartFunc(void *, int, RunState); @@ -385,7 +386,7 @@ struct IDEState { uint8_t cdrom_changed; int packet_transfer_size; int elementary_transfer_size; - int io_buffer_index; + int32_t io_buffer_index; int lba; int cd_sector_size; int atapi_dma; /* true if dma is requested for the packet cmd */ @@ -394,8 +395,8 @@ struct IDEState { struct iovec iov; QEMUIOVector qiov; /* ATA DMA state */ - int io_buffer_offset; - int io_buffer_size; + int32_t io_buffer_offset; + int32_t io_buffer_size; QEMUSGList sg; /* PIO transfer handling */ int req_nb_sectors; /* number of sectors per interrupt */ @@ -405,8 +406,8 @@ struct IDEState { uint8_t *io_buffer; /* PIO save/restore */ int32_t io_buffer_total_len; - int cur_io_buffer_offset; - int cur_io_buffer_len; + int32_t cur_io_buffer_offset; + int32_t cur_io_buffer_len; uint8_t end_transfer_fn_idx; QEMUTimer *sector_write_timer; /* only used for win2k install hack */ uint32_t irq_count; /* counts IRQs when using win2k install hack */ @@ -430,7 +431,7 @@ struct IDEState { struct IDEDMAOps { DMAStartFunc *start_dma; DMAVoidFunc *start_transfer; - DMAIntFunc *prepare_buf; + DMAInt32Func *prepare_buf; DMAu32Func *commit_buf; DMAIntFunc *rw_buf; DMAIntFunc *set_unit; diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 9a55407059..f6074f2024 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -553,6 +553,11 @@ static int ide_nop_int(IDEDMA *dma, int x) return 0; } +static int32_t ide_nop_int32(IDEDMA *dma, int x) +{ + return 0; +} + static void ide_nop_restart(void *opaque, int x, RunState y) { } @@ -569,7 +574,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s, static const IDEDMAOps dbdma_ops = { .start_dma = ide_dbdma_start, - .prepare_buf = ide_nop_int, + .prepare_buf = ide_nop_int32, .rw_buf = ide_nop_int, .set_unit = ide_nop_int, .restart_cb = ide_nop_restart, diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 2dad50e8aa..bee5ad39fe 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -28,7 +28,7 @@ #include #include "sysemu/block-backend.h" #include "sysemu/dma.h" - +#include "qemu/error-report.h" #include #define BMDMA_PAGE_SIZE 4096 @@ -55,8 +55,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s, } } -/* return 0 if buffer completed */ -static int bmdma_prepare_buf(IDEDMA *dma, int is_write) +/** + * Return the number of bytes successfully prepared. + * -1 on error. + */ +static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write) { BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma); IDEState *s = bmdma_active_if(bm); @@ -74,8 +77,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write) if (bm->cur_prd_len == 0) { /* 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 != 0; + (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) { + return s->io_buffer_size; + } pci_dma_read(pci_dev, bm->cur_addr, &prd, 8); bm->cur_addr += 8; prd.addr = le32_to_cpu(prd.addr); @@ -90,12 +94,23 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write) l = bm->cur_prd_len; if (l > 0) { qemu_sglist_add(&s->sg, bm->cur_prd_addr, l); + + /* 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.\n"); + break; + } bm->cur_prd_addr += l; bm->cur_prd_len -= l; s->io_buffer_size += l; } } - return 1; + + qemu_sglist_destroy(&s->sg); + s->io_buffer_size = 0; + return -1; } /* return 0 if buffer completed */ From 72a065dbb13dd187040c61cdde79476720341cfa Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 3 Nov 2014 18:56:15 -0500 Subject: [PATCH 05/11] ahci: add is_ncq predicate helper A small helper to determine which S/ATA commands are destined to be routed to the NCQ pathways. This references SATA 3.2 section 13.6, Native Command Queueing. See sections 13.6.4, 13.6.5, 13.6.6, 13.6.7 and 13.6.8 for all SATA commands considered to be part of the NCQ feature set. This is summarized in a small list in section 13.6.3.1 and again in 13.6.3.2. Not all of these NCQ commands are currently supported, so the error pathways are adjusted slightly to be more informative in the case they are encountered. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1415058979-16604-2-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 28 ++++++++++++++++++++++++---- hw/ide/ahci.h | 3 +++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9647d94d9b..f2acddbea6 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -854,6 +854,21 @@ static void ncq_cb(void *opaque, int ret) ncq_tfs->used = 0; } +static int is_ncq(uint8_t ata_cmd) +{ + /* Based on SATA 3.2 section 13.6.3.2 */ + switch (ata_cmd) { + case READ_FPDMA_QUEUED: + case WRITE_FPDMA_QUEUED: + case NCQ_NON_DATA: + case RECEIVE_FPDMA_QUEUED: + case SEND_FPDMA_QUEUED: + return 1; + default: + return 0; + } +} + static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, int slot) { @@ -919,9 +934,15 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_cb, ncq_tfs); break; default: - DPRINTF(port, "error: tried to process non-NCQ command as NCQ\n"); + if (is_ncq(cmd_fis[2])) { + DPRINTF(port, + "error: unsupported NCQ command (0x%02x) received\n", + cmd_fis[2]); + } else { + DPRINTF(port, + "error: tried to process non-NCQ command as NCQ\n"); + } qemu_sglist_destroy(&ncq_tfs->sglist); - break; } } @@ -1013,8 +1034,7 @@ static int handle_cmd(AHCIState *s, int port, int slot) if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) { /* Check for NCQ command */ - if ((cmd_fis[2] == READ_FPDMA_QUEUED) || - (cmd_fis[2] == WRITE_FPDMA_QUEUED)) { + if (is_ncq(cmd_fis[2])) { process_ncq_command(s, port, cmd_fis, slot); goto out; } diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index b12323730b..e0d2eb8f15 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -186,6 +186,9 @@ #define READ_FPDMA_QUEUED 0x60 #define WRITE_FPDMA_QUEUED 0x61 +#define NCQ_NON_DATA 0x63 +#define RECEIVE_FPDMA_QUEUED 0x65 +#define SEND_FPDMA_QUEUED 0x64 #define RES_FIS_DSFIS 0x00 #define RES_FIS_PSFIS 0x20 From 1cbdd96813474de4191b0b37b859a5460373093b Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 3 Nov 2014 18:56:16 -0500 Subject: [PATCH 06/11] ahci: Fix FIS decomposition This patch introduces a few changes to how FIS packets are deciphered in the AHCI virtual device. The summary of changes can be grouped into two pieces: [A] Changes to how we apply a preliminary sieve to FISes, [B] Changes in how we internalize a decomposed FIS. == Changes to how we apply a preliminary sieve to FISes == (1) Packets may now either update the Control register or the Command register, but not both. This is according to the SATA 3.2 specification which states: "...the device either initiates processing of the command indicated in the Command register or initiates processing of the control request indicated [...] depending on the state of the C bit in the FIS." See SATA 3.2 section 10.5.5.4, "Reception" in the 10.5.5 "Register Host to Device FIS" section. This change accounts for the first two regions of change within the diff. All other changes belong to the following changes. == Changes in how we internalize a decomposed FIS == (2) Instead of trying to extract the sector number out of the FIS from bytes 4-10 and setting it with ide_set_sector, we set the appropriate IDEState registers and trust that ide_get_sector can retrieve the correct sector later. By "constructing" the sector for use with ide_set_sector, we are duplicating the mechanisms of ide_get_sector. This change makes the FIS decomposition more obvious. SATA 3.2 as a specification does not make the legacy register mapping with respect to the D2H FIS obvious. However, SATA 3.2 section 10.5.5.1 "Register Host to Device FIS layout" describes all of the "cmd_fis" bytes: 0 - FIS Type (0x27) 1 - Port Multiplier Port and Command Update flag 2 - ATA Command 3 - Features_Low 4 - LBA 7:0 5 - LBA 15:8 6 - LBA 23:16 7 - Device, AKA "Drive Select." 8 - LBA 31:24 9 - LBA 39:32 10 - LBA 47:40 11 - Features_High 12 - Count Low 13 - Count High 14 - ICC 15 - Control 16-19 - Auxiliary (for NCQ, defined per-command) Most of these registers map to existing IDEState registers in obvious ways, especially features, select, hob_features, and nsector (count). ICC is reserved in older specifications but is not supported in our implementation, and remains unused here. The Control register is not valid for a command that is trying to update the command register and is to be considered reserved at this point. What is not obvious is the LBA register mappings, but SATA 1.0 can help inform of us legacy device support, see SATA 1.0 section 8.5.2 "Register - Host to Device." LBA 7:0 - Sector Number (sector) LBA 15:8 - Cyl Low (lcyl) LBA 23:16 - Cyl High (hcyl) LBA 31:24 - Sector Num Exp. (hob_sector) LBA 39:32 - Cyl Low Exp. (hob_lcyl) LBA 47:40 - Cyl High Exp. (hob_hcyl) These mappings help guide which registers the FIS should be decomposed into/towards for CHS, LBA28 and LBA48 commands. As a note: The prior confusion that can be seen in the documentation arises from the fact that CHS and LBA28 commands use the low nybble of the drive select register to store LBA 27:24, whereas LNA48 commands use the hob_sector, hob_lcyl and hob_hcyl registers as explained above. The decomposition as it stands now will correctly decompose CHS, LBA28 and LBA48 commands into their appropriate registers where the core IDE/ATAPI layers can deal with them correctly. See the below point for more information. (3) We save cmd_fis[7] as ide_state->select, which informs decisions about if we are using LBA or CHS. This corrects a bug in AHCI wherein we attempt to set and/or retrieve the sector number by using ide_set_sector and ide_get_sector, which depend on the select register to determine if we are using LBA or CHS. Without this adjustment, LBA48 read/writes are currently broken. Thanks to Eniac Zheng @ HP for pointing this out. (4) Save cmd_fis[11] as ide_state->hob_feature, as defined in SATA 3.2. (5) For several ATA commands, the sector count register set to 0 is a magic number that means 256 sectors. For LBA48 commands, this means 65,536 sectors. We drop the magic sector correction here, and trust the ide core layer to handle the conversion appropriately, in ide_cmd_lba48_transform(). As it stands, the current AHCI code is only compliant with LBA28 commands. By simply removing the magic, it will work with LBA28 and LBA48. (6) We expand FIS decomposition to include both ATAPI and IDE devices. We leave the logic of determining if the fields are valid or not to the respective layers. This change intends to make it clearer that AHCI is only a composition mechanism for the FIS packets: the meanings of the registers is best left to the implementation layers for those devices. (7) Forcefully setting the feature, hcyl and lcyl registers for ATAPI commands is removed. - The hcyl and lcyl magic present here is valid at boot only, and should not be overridden for every PACKET command. - The feature register is defined as valid for the PACKET command, so we should not suppress it. The ATAPI layer does not even currently depend on or require 0x01 as mandatory. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1415058979-16604-3-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 64 +++++++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 38 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index f2acddbea6..43da3637da 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1018,7 +1018,8 @@ static int handle_cmd(AHCIState *s, int port, int slot) break; } - switch (s->dev[port].port_state) { + if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) { + switch (s->dev[port].port_state) { case STATE_RUN: if (cmd_fis[15] & ATA_SRST) { s->dev[port].port_state = STATE_RESET; @@ -1029,9 +1030,10 @@ static int handle_cmd(AHCIState *s, int port, int slot) ahci_reset_port(s, port); } break; + } } - if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) { + else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) { /* Check for NCQ command */ if (is_ncq(cmd_fis[2])) { @@ -1039,50 +1041,36 @@ static int handle_cmd(AHCIState *s, int port, int slot) goto out; } - /* Decompose the FIS */ - ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]); + /* Decompose the FIS: + * AHCI does not interpret FIS packets, it only forwards them. + * SATA 1.0 describes how to decode LBA28 and CHS FIS packets. + * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets. + * + * ATA4 describes sector number for LBA28/CHS commands. + * ATA6 describes sector number for LBA48 commands. + * ATA8 deprecates CHS fully, describing only LBA28/48. + * + * We dutifully convert the FIS into IDE registers, and allow the + * core layer to interpret them as needed. */ ide_state->feature = cmd_fis[3]; - if (!ide_state->nsector) { - ide_state->nsector = 256; - } - - if (ide_state->drive_kind != IDE_CD) { - /* - * We set the sector depending on the sector defined in the FIS. - * Unfortunately, the spec isn't exactly obvious on this one. - * - * Apparently LBA48 commands set fis bytes 10,9,8,6,5,4 to the - * 48 bit sector number. ATA_CMD_READ_DMA_EXT is an example for - * such a command. - * - * Non-LBA48 commands however use 7[lower 4 bits],6,5,4 to define a - * 28-bit sector number. ATA_CMD_READ_DMA is an example for such - * a command. - * - * Since the spec doesn't explicitly state what each field should - * do, I simply assume non-used fields as reserved and OR everything - * together, independent of the command. - */ - ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40) - | ((uint64_t)cmd_fis[9] << 32) - /* This is used for LBA48 commands */ - | ((uint64_t)cmd_fis[8] << 24) - /* This is used for non-LBA48 commands */ - | ((uint64_t)(cmd_fis[7] & 0xf) << 24) - | ((uint64_t)cmd_fis[6] << 16) - | ((uint64_t)cmd_fis[5] << 8) - | cmd_fis[4]); - } + ide_state->sector = cmd_fis[4]; /* LBA 7:0 */ + ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */ + ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */ + ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */ + ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */ + ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */ + ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */ + ide_state->hob_feature = cmd_fis[11]; + ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]); + /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */ + /* 15: Only valid when UPDATE_COMMAND not set. */ /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command * table to ide_state->io_buffer */ if (opts & AHCI_CMD_ATAPI) { memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10); - ide_state->lcyl = 0x14; - ide_state->hcyl = 0xeb; debug_print_fis(ide_state->io_buffer, 0x10); - ide_state->feature = IDE_FEATURE_DMA; s->dev[port].done_atapi_packet = false; /* XXX send PIO setup FIS */ } From 36ab3c3400ac941e4d9afc044be08143ff9eea62 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 3 Nov 2014 18:56:17 -0500 Subject: [PATCH 07/11] ahci: Reorder error cases in handle_cmd Error checking in ahci's handle_cmd is re-ordered so that we initialize as few things as possible before we've done our sanity checking. This simplifies returning from this call in case of an error. A check to make sure the DMA memory map succeeds with the correct size is also added, and the debug print of the command fis is cleaned up with its size corrected. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1415058979-16604-4-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 43da3637da..578a93b627 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -961,38 +961,37 @@ static int handle_cmd(AHCIState *s, int port, int slot) return -1; } - cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot]; - if (!s->dev[port].lst) { DPRINTF(port, "error: lst not given but cmd handled"); return -1; } - + cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot]; /* remember current slot handle for later */ s->dev[port].cur_cmd = cmd; - opts = le32_to_cpu(cmd->opts); - tbl_addr = le64_to_cpu(cmd->tbl_addr); - - cmd_len = 0x80; - cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len, - DMA_DIRECTION_FROM_DEVICE); - - if (!cmd_fis) { - DPRINTF(port, "error: guest passed us an invalid cmd fis\n"); + /* The device we are working for */ + ide_state = &s->dev[port].port.ifs[0]; + if (!ide_state->blk) { + DPRINTF(port, "error: guest accessed unused port"); return -1; } - /* The device we are working for */ - ide_state = &s->dev[port].port.ifs[0]; - - if (!ide_state->blk) { - DPRINTF(port, "error: guest accessed unused port"); + opts = le32_to_cpu(cmd->opts); + tbl_addr = le64_to_cpu(cmd->tbl_addr); + cmd_len = 0x80; + cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len, + DMA_DIRECTION_FROM_DEVICE); + if (!cmd_fis) { + DPRINTF(port, "error: guest passed us an invalid cmd fis\n"); + return -1; + } else if (cmd_len != 0x80) { + ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_HBUS_ERR); + DPRINTF(port, "error: dma_memory_map failed: " + "(len(%02"PRIx64") != 0x80)\n", + cmd_len); goto out; } - - debug_print_fis(cmd_fis, 0x90); - //debug_print_fis(cmd_fis, (opts & AHCI_CMD_HDR_CMD_FIS_LEN) * 4); + debug_print_fis(cmd_fis, 0x80); switch (cmd_fis[0]) { case SATA_FIS_TYPE_REGISTER_H2D: From 102e56254dbf5f789e43d7eb29023f296cb67536 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 3 Nov 2014 18:56:18 -0500 Subject: [PATCH 08/11] ahci: Check cmd_fis[1] more explicitly Instead of checking for a known byte, inspect the fields of this byte explicitly to produce more meaningful error messages and improve the readability of this section. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1415058979-16604-5-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 578a93b627..d6b012c88f 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1004,17 +1004,18 @@ static int handle_cmd(AHCIState *s, int port, int slot) break; } - switch (cmd_fis[1]) { - case SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER: - break; - case 0: - break; - default: - DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x " - "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1], - cmd_fis[2]); - goto out; - break; + if (cmd_fis[1] & 0x0F) { + DPRINTF(port, "Port Multiplier not supported." + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", + cmd_fis[0], cmd_fis[1], cmd_fis[2]); + goto out; + } + + if (cmd_fis[1] & 0x70) { + DPRINTF(port, "Reserved flags set in H2D Register FIS." + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", + cmd_fis[0], cmd_fis[1], cmd_fis[2]); + goto out; } if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) { From 107f0d4677e126b073d9b606788d2c126c520416 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 3 Nov 2014 18:56:19 -0500 Subject: [PATCH 09/11] ahci: factor out FIS decomposition from handle_cmd In order to make handle_cmd more readable at the macro level, the details of how to decompose particular types of FIS packets are left to helper functions. In our case, the only type of FIS packet we currently expect to see is a Register H2D FIS packet, but the gory details of its decomposition are of no particular interest in handle_cmd. This patch keeps the receipt of FIS packets and the decomposition thereof separated to two different functions. Signed-off-by: John Snow Reviewed-by: Paolo Bonzini Message-id: 1415058979-16604-6-git-send-email-jsnow@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/ide/ahci.c | 169 +++++++++++++++++++++++++------------------------- 1 file changed, 86 insertions(+), 83 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index d6b012c88f..94f28e6bac 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -946,10 +946,94 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, } } +static void handle_reg_h2d_fis(AHCIState *s, int port, + int slot, uint8_t *cmd_fis) +{ + IDEState *ide_state = &s->dev[port].port.ifs[0]; + AHCICmdHdr *cmd = s->dev[port].cur_cmd; + uint32_t opts = le32_to_cpu(cmd->opts); + + if (cmd_fis[1] & 0x0F) { + DPRINTF(port, "Port Multiplier not supported." + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", + cmd_fis[0], cmd_fis[1], cmd_fis[2]); + return; + } + + if (cmd_fis[1] & 0x70) { + DPRINTF(port, "Reserved flags set in H2D Register FIS." + " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", + cmd_fis[0], cmd_fis[1], cmd_fis[2]); + return; + } + + if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) { + switch (s->dev[port].port_state) { + case STATE_RUN: + if (cmd_fis[15] & ATA_SRST) { + s->dev[port].port_state = STATE_RESET; + } + break; + case STATE_RESET: + if (!(cmd_fis[15] & ATA_SRST)) { + ahci_reset_port(s, port); + } + break; + } + return; + } + + /* Check for NCQ command */ + if (is_ncq(cmd_fis[2])) { + process_ncq_command(s, port, cmd_fis, slot); + return; + } + + /* Decompose the FIS: + * AHCI does not interpret FIS packets, it only forwards them. + * SATA 1.0 describes how to decode LBA28 and CHS FIS packets. + * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets. + * + * ATA4 describes sector number for LBA28/CHS commands. + * ATA6 describes sector number for LBA48 commands. + * ATA8 deprecates CHS fully, describing only LBA28/48. + * + * We dutifully convert the FIS into IDE registers, and allow the + * core layer to interpret them as needed. */ + ide_state->feature = cmd_fis[3]; + ide_state->sector = cmd_fis[4]; /* LBA 7:0 */ + ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */ + ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */ + ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */ + ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */ + ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */ + ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */ + ide_state->hob_feature = cmd_fis[11]; + ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]); + /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */ + /* 15: Only valid when UPDATE_COMMAND not set. */ + + /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command + * table to ide_state->io_buffer */ + if (opts & AHCI_CMD_ATAPI) { + memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10); + debug_print_fis(ide_state->io_buffer, 0x10); + s->dev[port].done_atapi_packet = false; + /* XXX send PIO setup FIS */ + } + + ide_state->error = 0; + + /* Reset transferred byte counter */ + cmd->status = 0; + + /* We're ready to process the command in FIS byte 2. */ + ide_exec_cmd(&s->dev[port].port, cmd_fis[2]); +} + static int handle_cmd(AHCIState *s, int port, int slot) { IDEState *ide_state; - uint32_t opts; uint64_t tbl_addr; AHCICmdHdr *cmd; uint8_t *cmd_fis; @@ -976,7 +1060,6 @@ static int handle_cmd(AHCIState *s, int port, int slot) return -1; } - opts = le32_to_cpu(cmd->opts); tbl_addr = le64_to_cpu(cmd->tbl_addr); cmd_len = 0x80; cmd_fis = dma_memory_map(s->as, tbl_addr, &cmd_len, @@ -995,95 +1078,15 @@ static int handle_cmd(AHCIState *s, int port, int slot) switch (cmd_fis[0]) { case SATA_FIS_TYPE_REGISTER_H2D: + handle_reg_h2d_fis(s, port, slot, cmd_fis); break; default: DPRINTF(port, "unknown command cmd_fis[0]=%02x cmd_fis[1]=%02x " "cmd_fis[2]=%02x\n", cmd_fis[0], cmd_fis[1], cmd_fis[2]); - goto out; break; } - if (cmd_fis[1] & 0x0F) { - DPRINTF(port, "Port Multiplier not supported." - " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", - cmd_fis[0], cmd_fis[1], cmd_fis[2]); - goto out; - } - - if (cmd_fis[1] & 0x70) { - DPRINTF(port, "Reserved flags set in H2D Register FIS." - " cmd_fis[0]=%02x cmd_fis[1]=%02x cmd_fis[2]=%02x\n", - cmd_fis[0], cmd_fis[1], cmd_fis[2]); - goto out; - } - - if (!(cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER)) { - switch (s->dev[port].port_state) { - case STATE_RUN: - if (cmd_fis[15] & ATA_SRST) { - s->dev[port].port_state = STATE_RESET; - } - break; - case STATE_RESET: - if (!(cmd_fis[15] & ATA_SRST)) { - ahci_reset_port(s, port); - } - break; - } - } - - else if (cmd_fis[1] & SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) { - - /* Check for NCQ command */ - if (is_ncq(cmd_fis[2])) { - process_ncq_command(s, port, cmd_fis, slot); - goto out; - } - - /* Decompose the FIS: - * AHCI does not interpret FIS packets, it only forwards them. - * SATA 1.0 describes how to decode LBA28 and CHS FIS packets. - * Later specifications, e.g, SATA 3.2, describe LBA48 FIS packets. - * - * ATA4 describes sector number for LBA28/CHS commands. - * ATA6 describes sector number for LBA48 commands. - * ATA8 deprecates CHS fully, describing only LBA28/48. - * - * We dutifully convert the FIS into IDE registers, and allow the - * core layer to interpret them as needed. */ - ide_state->feature = cmd_fis[3]; - ide_state->sector = cmd_fis[4]; /* LBA 7:0 */ - ide_state->lcyl = cmd_fis[5]; /* LBA 15:8 */ - ide_state->hcyl = cmd_fis[6]; /* LBA 23:16 */ - ide_state->select = cmd_fis[7]; /* LBA 27:24 (LBA28) */ - ide_state->hob_sector = cmd_fis[8]; /* LBA 31:24 */ - ide_state->hob_lcyl = cmd_fis[9]; /* LBA 39:32 */ - ide_state->hob_hcyl = cmd_fis[10]; /* LBA 47:40 */ - ide_state->hob_feature = cmd_fis[11]; - ide_state->nsector = (int64_t)((cmd_fis[13] << 8) | cmd_fis[12]); - /* 14, 16, 17, 18, 19: Reserved (SATA 1.0) */ - /* 15: Only valid when UPDATE_COMMAND not set. */ - - /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command - * table to ide_state->io_buffer - */ - if (opts & AHCI_CMD_ATAPI) { - memcpy(ide_state->io_buffer, &cmd_fis[AHCI_COMMAND_TABLE_ACMD], 0x10); - debug_print_fis(ide_state->io_buffer, 0x10); - s->dev[port].done_atapi_packet = false; - /* XXX send PIO setup FIS */ - } - - ide_state->error = 0; - - /* Reset transferred byte counter */ - cmd->status = 0; - - /* We're ready to process the command in FIS byte 2. */ - ide_exec_cmd(&s->dev[port].port, cmd_fis[2]); - } - out: dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_FROM_DEVICE, cmd_len); From f3a9cfddaec127078ac1898de6b063db8ac3bb48 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Mon, 10 Nov 2014 15:07:44 +0800 Subject: [PATCH 10/11] block: Fix max nb_sectors in bdrv_make_zero In bdrv_rw_co we report -EINVAL for nb_sectors > INT_MAX / BDRV_SECTOR_SIZE, so a caller shouldn't exceed it. Signed-off-by: Fam Zheng Reviewed-by: Markus Armbruster Message-id: 1415603264-21497-1-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/block.c b/block.c index c612826c5c..a612594db5 100644 --- a/block.c +++ b/block.c @@ -2790,8 +2790,8 @@ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) if (nb_sectors <= 0) { return 0; } - if (nb_sectors > INT_MAX) { - nb_sectors = INT_MAX; + if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) { + nb_sectors = INT_MAX / BDRV_SECTOR_SIZE; } ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n); if (ret < 0) { From 5f58330790b72c4705b373ee0646fb3adf800b4e Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 14 Nov 2014 12:09:21 +0800 Subject: [PATCH 11/11] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info When extent types don't match, we return -ENOTSUP. In this case, be polite to the caller and don't modify bdi. Signed-off-by: Fam Zheng Reviewed-by: Max Reitz Message-id: 1415938161-16217-1-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- block/vmdk.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 673d3f5fb4..2cbfd3e72e 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -2137,23 +2137,29 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs) return spec_info; } +static bool vmdk_extents_type_eq(const VmdkExtent *a, const VmdkExtent *b) +{ + return a->flat == b->flat && + a->compressed == b->compressed && + (a->flat || a->cluster_sectors == b->cluster_sectors); +} + static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) { int i; BDRVVmdkState *s = bs->opaque; assert(s->num_extents); + + /* See if we have multiple extents but they have different cases */ + for (i = 1; i < s->num_extents; i++) { + if (!vmdk_extents_type_eq(&s->extents[0], &s->extents[i])) { + return -ENOTSUP; + } + } bdi->needs_compressed_writes = s->extents[0].compressed; if (!s->extents[0].flat) { bdi->cluster_size = s->extents[0].cluster_sectors << BDRV_SECTOR_BITS; } - /* See if we have multiple extents but they have different cases */ - for (i = 1; i < s->num_extents; i++) { - if (bdi->needs_compressed_writes != s->extents[i].compressed || - (bdi->cluster_size && bdi->cluster_size != - s->extents[i].cluster_sectors << BDRV_SECTOR_BITS)) { - return -ENOTSUP; - } - } return 0; }