[Qemu-commits] [qemu/qemu] a395f3: ahci: Fix byte count regression for ATAPI/PIO

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: a395f3fa2f26c94dac03b37e3dfb1074bfe2ddea
      
https://github.com/qemu/qemu/commit/a395f3fa2f26c94dac03b37e3dfb1074bfe2ddea
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 36334faf35ccc48d61ca3431a5c0787b125dd306
      
https://github.com/qemu/qemu/commit/36334faf35ccc48d61ca3431a5c0787b125dd306
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c
    M hw/ide/core.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: bef1301acb74d177b42890116e4eeaf26047b9e3
      
https://github.com/qemu/qemu/commit/bef1301acb74d177b42890116e4eeaf26047b9e3
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 3251bdcf1c67427d964517053c3d185b46e618e8
      
https://github.com/qemu/qemu/commit/3251bdcf1c67427d964517053c3d185b46e618e8
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c
    M hw/ide/core.c
    M hw/ide/internal.h
    M hw/ide/macio.c
    M hw/ide/pci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 72a065dbb13dd187040c61cdde79476720341cfa
      
https://github.com/qemu/qemu/commit/72a065dbb13dd187040c61cdde79476720341cfa
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c
    M hw/ide/ahci.h

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 1cbdd96813474de4191b0b37b859a5460373093b
      
https://github.com/qemu/qemu/commit/1cbdd96813474de4191b0b37b859a5460373093b
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 36ab3c3400ac941e4d9afc044be08143ff9eea62
      
https://github.com/qemu/qemu/commit/36ab3c3400ac941e4d9afc044be08143ff9eea62
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 102e56254dbf5f789e43d7eb29023f296cb67536
      
https://github.com/qemu/qemu/commit/102e56254dbf5f789e43d7eb29023f296cb67536
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 107f0d4677e126b073d9b606788d2c126c520416
      
https://github.com/qemu/qemu/commit/107f0d4677e126b073d9b606788d2c126c520416
  Author: John Snow <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M hw/ide/ahci.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Paolo Bonzini <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: f3a9cfddaec127078ac1898de6b063db8ac3bb48
      
https://github.com/qemu/qemu/commit/f3a9cfddaec127078ac1898de6b063db8ac3bb48
  Author: Fam Zheng <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M block.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: 5f58330790b72c4705b373ee0646fb3adf800b4e
      
https://github.com/qemu/qemu/commit/5f58330790b72c4705b373ee0646fb3adf800b4e
  Author: Fam Zheng <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M block/vmdk.c

  Log Message:
  -----------
  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 <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>


  Commit: b87dcdd0746dc110fa5a3353cbc257818e618930
      
https://github.com/qemu/qemu/commit/b87dcdd0746dc110fa5a3353cbc257818e618930
  Author: Peter Maydell <address@hidden>
  Date:   2014-11-14 (Fri, 14 Nov 2014)

  Changed paths:
    M block.c
    M block/vmdk.c
    M hw/ide/ahci.c
    M hw/ide/ahci.h
    M hw/ide/core.c
    M hw/ide/internal.h
    M hw/ide/macio.c
    M hw/ide/pci.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging

# gpg: Signature made Fri 14 Nov 2014 11:05:54 GMT using RSA key ID 81AB73C8
# gpg: Good signature from "Stefan Hajnoczi <address@hidden>"
# gpg:                 aka "Stefan Hajnoczi <address@hidden>"

* remotes/stefanha/tags/block-pull-request:
  vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info
  block: Fix max nb_sectors in bdrv_make_zero
  ahci: factor out FIS decomposition from handle_cmd
  ahci: Check cmd_fis[1] more explicitly
  ahci: Reorder error cases in handle_cmd
  ahci: Fix FIS decomposition
  ahci: add is_ncq predicate helper
  ide: Correct handling of malformed/short PRDTs
  ahci: unify sglist preparation
  ide: repair PIO transfers for cases where nsector > 1
  ahci: Fix byte count regression for ATAPI/PIO

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/c52e67924fbd...b87dcdd0746d