[Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Daniel Verkamp]

As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes
each, which comes to 20 bytes (decimal), not 0x20.

Signed-off-by: Daniel Verkamp <daniel@drv.nu>
---
 hw/ide/ahci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a883a92..2d7d03d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -462,7 +462,7 @@ static void ahci_check_cmd_bh(void *opaque)
 
 static void ahci_init_d2h(AHCIDevice *ad)
 {
-    uint8_t init_fis[0x20];
+    uint8_t init_fis[20];
     IDEState *ide_state = &ad->port.ifs[0];
 
     memset(init_fis, 0, sizeof(init_fis));
@@ -619,7 +619,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     d2h_fis[11] = cmd_fis[11];
     d2h_fis[12] = cmd_fis[12];
     d2h_fis[13] = cmd_fis[13];
-    for (i = 14; i < 0x20; i++) {
+    for (i = 14; i < 20; i++) {
         d2h_fis[i] = 0;
     }
 
-- 
1.7.8.6

Re: [Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Kevin Wolf]

Am 23.05.2012 01:26, schrieb Daniel Verkamp:
> As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes
> each, which comes to 20 bytes (decimal), not 0x20.
> 
> Signed-off-by: Daniel Verkamp <daniel@drv.nu>
> ---
>  hw/ide/ahci.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a883a92..2d7d03d 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -462,7 +462,7 @@ static void ahci_check_cmd_bh(void *opaque)
>  
>  static void ahci_init_d2h(AHCIDevice *ad)
>  {
> -    uint8_t init_fis[0x20];
> +    uint8_t init_fis[20];
>      IDEState *ide_state = &ad->port.ifs[0];
>  
>      memset(init_fis, 0, sizeof(init_fis));
> @@ -619,7 +619,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
>      d2h_fis[11] = cmd_fis[11];
>      d2h_fis[12] = cmd_fis[12];
>      d2h_fis[13] = cmd_fis[13];
> -    for (i = 14; i < 0x20; i++) {
> +    for (i = 14; i < 20; i++) {
>          d2h_fis[i] = 0;
>      }
>  

Alex or Andreas, can you please ack/nack?

Kevin

Re: [Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Alexander Graf]

Am 23.05.2012 um 11:13 schrieb Kevin Wolf <kwolf@redhat.com>:

> Am 23.05.2012 01:26, schrieb Daniel Verkamp:
>> As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes
>> each, which comes to 20 bytes (decimal), not 0x20.

Not sure I understand. FISs can have different sizes depending on the payload they are. The one you are looking at here is the d2h init FIS.

From the SATA 1.0a spec:

FIS Type - Set to a value of 34h. Defines the rest of the FIS fields. Defines the length of the
FIS as five Dwords.

So yes, you are right. The register FIS is 20 bytes, not 0x20 bytes long.

Does this fix some actual breakage for you?


Alex

>> 
>> Signed-off-by: Daniel Verkamp <daniel@drv.nu>
>> ---
>> hw/ide/ahci.c |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index a883a92..2d7d03d 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -462,7 +462,7 @@ static void ahci_check_cmd_bh(void *opaque)
>> 
>> static void ahci_init_d2h(AHCIDevice *ad)
>> {
>> -    uint8_t init_fis[0x20];
>> +    uint8_t init_fis[20];
>>     IDEState *ide_state = &ad->port.ifs[0];
>> 
>>     memset(init_fis, 0, sizeof(init_fis));
>> @@ -619,7 +619,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
>>     d2h_fis[11] = cmd_fis[11];
>>     d2h_fis[12] = cmd_fis[12];
>>     d2h_fis[13] = cmd_fis[13];
>> -    for (i = 14; i < 0x20; i++) {
>> +    for (i = 14; i < 20; i++) {
>>         d2h_fis[i] = 0;
>>     }
>> 
> 
> Alex or Andreas, can you please ack/nack?
> 
> Kevin

Re: [Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Kevin Wolf]

Am 23.05.2012 11:53, schrieb Alexander Graf:
> 
> 
> Am 23.05.2012 um 11:13 schrieb Kevin Wolf <kwolf@redhat.com>:
> 
>> Am 23.05.2012 01:26, schrieb Daniel Verkamp:
>>> As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes
>>> each, which comes to 20 bytes (decimal), not 0x20.
> 
> Not sure I understand. FISs can have different sizes depending on the payload they are. The one you are looking at here is the d2h init FIS.
> 
> From the SATA 1.0a spec:
> 
> FIS Type - Set to a value of 34h. Defines the rest of the FIS fields. Defines the length of the
> FIS as five Dwords.
> 
> So yes, you are right. The register FIS is 20 bytes, not 0x20 bytes long.
> 
> Does this fix some actual breakage for you?

In theory the SDBFIS could be overwritten with zeros. No idea what this
means or if it matters in practice.

Kevin

Re: [Qemu-devel] [PATCH] ahci: SATA FIS is 20 bytes, not 0x20

[Stefan Weil]

Am 23.05.2012 01:26, schrieb Daniel Verkamp:
> As in the SATA and AHCI specifications, a FIS is 5 Dwords of 4 bytes
> each, which comes to 20 bytes (decimal), not 0x20.
>
> Signed-off-by: Daniel Verkamp<daniel@drv.nu>
> ---
>   hw/ide/ahci.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a883a92..2d7d03d 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -462,7 +462,7 @@ static void ahci_check_cmd_bh(void *opaque)
>
>   static void ahci_init_d2h(AHCIDevice *ad)
>   {
> -    uint8_t init_fis[0x20];
> +    uint8_t init_fis[20];
>       IDEState *ide_state =&ad->port.ifs[0];

The current code only uses 14 elements, so 20 elements
still waste some local memory (and 0x20 elements waste
even more).

>
>
>       memset(init_fis, 0, sizeof(init_fis));
> @@ -619,7 +619,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
>       d2h_fis[11] = cmd_fis[11];
>       d2h_fis[12] = cmd_fis[12];
>       d2h_fis[13] = cmd_fis[13];
> -    for (i = 14; i<  0x20; i++) {
> +    for (i = 14; i<  20; i++) {
>           d2h_fis[i] = 0;
>       }

I am not sure whether this change is correct.
This code does _not_ access the array which was allocated above:

     d2h_fis = &ad->res_fis[RES_FIS_RFIS];

Regards,
Stefan W.