[Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Alexander Graf]

AHCI provides two ways of reading/writing data:

 1) NCQ
 2) ATA commands with the LBA in the command FIS

In the second code path, we didn't handle any LBAs that were bigger than
16 bits, so whenever a guest that used high LBA numbers wanted to access
data, the LBA got truncated down to 16 bits, giving the guest garbage.

This patch adds support for LBAs higher than 16 bits. I've tested that it
works just fine with SeaBIOS and Linux guests. This patch also unbreaks
the often reported grub errors people have seen with AHCI.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c6e0c77..bc5c553 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -884,8 +884,13 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         }
 
         if (ide_state->drive_kind != IDE_CD) {
-            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) |
-                           cmd_fis[4]);
+            ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
+                                    | ((uint64_t)cmd_fis[9] << 32)
+                                    | ((uint64_t)cmd_fis[8] << 24)
+                                    | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
+                                    | ((uint64_t)cmd_fis[6] << 16)
+                                    | ((uint64_t)cmd_fis[5] << 8)
+                                    | cmd_fis[4]);
         }
 
         /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On 2011-05-18 14:48, Alexander Graf wrote:
> AHCI provides two ways of reading/writing data:
> 
>  1) NCQ
>  2) ATA commands with the LBA in the command FIS
> 
> In the second code path, we didn't handle any LBAs that were bigger than
> 16 bits, so whenever a guest that used high LBA numbers wanted to access
> data, the LBA got truncated down to 16 bits, giving the guest garbage.
> 
> This patch adds support for LBAs higher than 16 bits. I've tested that it
> works just fine with SeaBIOS and Linux guests. This patch also unbreaks
> the often reported grub errors people have seen with AHCI.

Cool! I actually had such a guest as well, but I didn't manage to look
closer so far. Now this patch cures it.

Jan

> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ide/ahci.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index c6e0c77..bc5c553 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -884,8 +884,13 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>          }
>  
>          if (ide_state->drive_kind != IDE_CD) {
> -            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) |
> -                           cmd_fis[4]);
> +            ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
> +                                    | ((uint64_t)cmd_fis[9] << 32)
> +                                    | ((uint64_t)cmd_fis[8] << 24)
> +                                    | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
> +                                    | ((uint64_t)cmd_fis[6] << 16)
> +                                    | ((uint64_t)cmd_fis[5] << 8)
> +                                    | cmd_fis[4]);
>          }
>  
>          /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Alexander Graf]

On 18.05.2011, at 21:16, Jan Kiszka wrote:

> On 2011-05-18 14:48, Alexander Graf wrote:
>> AHCI provides two ways of reading/writing data:
>> 
>> 1) NCQ
>> 2) ATA commands with the LBA in the command FIS
>> 
>> In the second code path, we didn't handle any LBAs that were bigger than
>> 16 bits, so whenever a guest that used high LBA numbers wanted to access
>> data, the LBA got truncated down to 16 bits, giving the guest garbage.
>> 
>> This patch adds support for LBAs higher than 16 bits. I've tested that it
>> works just fine with SeaBIOS and Linux guests. This patch also unbreaks
>> the often reported grub errors people have seen with AHCI.
> 
> Cool! I actually had such a guest as well, but I didn't manage to look
> closer so far. Now this patch cures it.

Heh, yeah. Only costed me 2 days of my life - sigh :)


Alex

[Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Alexander Graf]

AHCI provides two ways of reading/writing data:

 1) NCQ
 2) ATA commands with the LBA in the command FIS

In the second code path, we didn't handle any LBAs that were bigger than
16 bits, so whenever a guest that used high LBA numbers wanted to access
data, the LBA got truncated down to 16 bits, giving the guest garbage.

This patch adds support for LBAs higher than 16 bits. I've tested that it
works just fine with SeaBIOS and Linux guests. This patch also unbreaks
the often reported grub errors people have seen with AHCI.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - document fis bytes
---
 hw/ide/ahci.c |   27 +++++++++++++++++++++++++--
 1 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c6e0c77..db20aae 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -884,8 +884,31 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         }
 
         if (ide_state->drive_kind != IDE_CD) {
-            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) |
-                           cmd_fis[4]);
+            /*
+             * 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]);
         }
 
         /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Kevin Wolf]

Am 19.05.2011 11:57, schrieb Alexander Graf:
> AHCI provides two ways of reading/writing data:
> 
>  1) NCQ
>  2) ATA commands with the LBA in the command FIS
> 
> In the second code path, we didn't handle any LBAs that were bigger than
> 16 bits, so whenever a guest that used high LBA numbers wanted to access
> data, the LBA got truncated down to 16 bits, giving the guest garbage.
> 
> This patch adds support for LBAs higher than 16 bits. I've tested that it
> works just fine with SeaBIOS and Linux guests. This patch also unbreaks
> the often reported grub errors people have seen with AHCI.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - document fis bytes

Thanks, applied to the block branch.

Kevin

[Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Alexander Graf]

AHCI provides two ways of reading/writing data:

 1) NCQ
 2) ATA commands with the LBA in the command FIS

In the second code path, we didn't handle any LBAs that were bigger than
16 bits, so whenever a guest that used high LBA numbers wanted to access
data, the LBA got truncated down to 16 bits, giving the guest garbage.

This patch adds support for LBAs higher than 16 bits. I've tested that it
works just fine with SeaBIOS and Linux guests. This patch also unbreaks
the often reported grub errors people have seen with AHCI.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c6e0c77..bc5c553 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -884,8 +884,13 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         }
 
         if (ide_state->drive_kind != IDE_CD) {
-            ide_set_sector(ide_state, (cmd_fis[6] << 16) | (cmd_fis[5] << 8) |
-                           cmd_fis[4]);
+            ide_set_sector(ide_state, ((uint64_t)cmd_fis[10] << 40)
+                                    | ((uint64_t)cmd_fis[9] << 32)
+                                    | ((uint64_t)cmd_fis[8] << 24)
+                                    | ((uint64_t)(cmd_fis[7] & 0xf) << 24)
+                                    | ((uint64_t)cmd_fis[6] << 16)
+                                    | ((uint64_t)cmd_fis[5] << 8)
+                                    | cmd_fis[4]);
         }
 
         /* Copy the ACMD field (ATAPI packet, if any) from the AHCI command
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH] ahci: Fix non-NCQ accesses for LBA > 16bits

[Alexander Graf]

On 25.05.2011, at 00:46, Alexander Graf wrote:

> AHCI provides two ways of reading/writing data:
> 
> 1) NCQ
> 2) ATA commands with the LBA in the command FIS
> 
> In the second code path, we didn't handle any LBAs that were bigger than
> 16 bits, so whenever a guest that used high LBA numbers wanted to access
> data, the LBA got truncated down to 16 bits, giving the guest garbage.
> 
> This patch adds support for LBAs higher than 16 bits. I've tested that it
> works just fine with SeaBIOS and Linux guests. This patch also unbreaks
> the often reported grub errors people have seen with AHCI.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Eh - ignore that one please. I was reusing an old git-send-email command line that accidently had this patch in it :).


Alex