[Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[reza.jelveh]

This requires a custom ovmf image with sata controller for testing [0]

[0]: http://reza.jelveh.me/assets/OVMF.fd.bz2

[Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[reza.jelveh]

From: Reza Jelveh <reza.jelveh@tuhh.de>

The data byte count(DBC) read from the description information is defined for
21:00. 30:22 are reserved and 31 is the Interrupt on Completion (I) flag.

Interrupt is not implemented in QEMU. tbl_entry_size is a signed integer and
improperly reading the DBC leads to a negative offset that causes sglist
allocation to fail.

Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
---
 hw/ide/ahci.c | 12 +++++++++---
 hw/ide/ahci.h |  2 ++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9bae22e..93aa981 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -639,6 +639,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     }
 }
 
+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)
 {
     AHCICmdHdr *cmd = ad->cur_cmd;
@@ -681,7 +687,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
         sum = 0;
         for (i = 0; i < sglist_alloc_hint; i++) {
             /* flags_size is zero-based */
-            tbl_entry_size = (le32_to_cpu(tbl[i].flags_size) + 1);
+            tbl_entry_size = prdt_tbl_entry_size(tbl[i]);
             if (offset <= (sum + tbl_entry_size)) {
                 off_idx = i;
                 off_pos = offset - sum;
@@ -700,12 +706,12 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
         qemu_sglist_init(sglist, qbus->parent, (sglist_alloc_hint - off_idx),
                          ad->hba->as);
         qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
-                        le32_to_cpu(tbl[off_idx].flags_size) + 1 - off_pos);
+                        prdt_tbl_entry_size(tbl[off_idx]) - off_pos);
 
         for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
             /* flags_size is zero-based */
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
-                            le32_to_cpu(tbl[i].flags_size) + 1);
+                            prdt_tbl_entry_size(tbl[i]));
         }
     }
 
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 9a4064f..f418b30 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -201,6 +201,8 @@
 
 #define AHCI_COMMAND_TABLE_ACMD            0x40
 
+#define AHCI_PRDT_SIZE_MASK                0x3fffff
+
 #define IDE_FEATURE_DMA                    1
 
 #define READ_FPDMA_QUEUED                  0x60
-- 
1.9.2

Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[Alexander Graf]

On 29.06.14 20:21, reza.jelveh@gmail.com wrote:
> This requires a custom ovmf image with sata controller for testing [0]
>
> [0]: http://reza.jelveh.me/assets/OVMF.fd.bz2
>

I guess this is supposed to be a cover letter? A few rules for cover 
letters:

   1) Cover letters only make sense for patch sets. This is a single 
patch, so you don't need one
   2) Because they come with patch sets and you number patch sets, cover 
letters are [PATCH 0/n].
   3) Usually cover letters contain git statistics. Try git format-patch 
--cover-letter. It will give you a nice template.

The usual way to add the comment you have here to a patch you're trying 
to submit it is to put volatile information (things that shouldn't end 
up in the git log) behind a "---" line in the commit message. Everything 
after that line gets ignored by git when the patch gets applied.


Alex

Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[Alexander Graf]

On 29.06.14 20:21, reza.jelveh@gmail.com wrote:
> From: Reza Jelveh <reza.jelveh@tuhh.de>

This is a hint that your git configuration isn't fully correct. If the 
email address git thinks you want to use is the same as the From: email 
address, it will not print this line. I suppose the problem is with the 
difference in @gmail.com and @tuhh.de?

> The data byte count(DBC) read from the description information is defined for

bits

> 21:00.

Bits

> 30:22 are reserved and

bit

> 31 is the Interrupt on Completion (I) flag.
>
> Interrupt is not implemented in QEMU.

They are implemented in QEMU, but incorrectly. We trigger a completion 
interrupt after every transaction, not every time we see the I bit in 
the PRDT.

> tbl_entry_size is a signed integer and
> improperly reading the DBC leads to a negative offset that causes sglist
> allocation to fail.
>
> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
> ---
>   hw/ide/ahci.c | 12 +++++++++---
>   hw/ide/ahci.h |  2 ++
>   2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9bae22e..93aa981 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -639,6 +639,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
>       }
>   }
>   
> +static int prdt_tbl_entry_size(const AHCI_SG tbl)

The argument should still be a pointer.

> +{
> +    return (le32_to_cpu(tbl.flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
> +}
> +
> +

There is still one whitespace line too much :).


Alex

>   static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
>   {
>       AHCICmdHdr *cmd = ad->cur_cmd;
> @@ -681,7 +687,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
>           sum = 0;
>           for (i = 0; i < sglist_alloc_hint; i++) {
>               /* flags_size is zero-based */
> -            tbl_entry_size = (le32_to_cpu(tbl[i].flags_size) + 1);
> +            tbl_entry_size = prdt_tbl_entry_size(tbl[i]);
>               if (offset <= (sum + tbl_entry_size)) {
>                   off_idx = i;
>                   off_pos = offset - sum;
> @@ -700,12 +706,12 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
>           qemu_sglist_init(sglist, qbus->parent, (sglist_alloc_hint - off_idx),
>                            ad->hba->as);
>           qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
> -                        le32_to_cpu(tbl[off_idx].flags_size) + 1 - off_pos);
> +                        prdt_tbl_entry_size(tbl[off_idx]) - off_pos);
>   
>           for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
>               /* flags_size is zero-based */
>               qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
> -                            le32_to_cpu(tbl[i].flags_size) + 1);
> +                            prdt_tbl_entry_size(tbl[i]));
>           }
>       }
>   
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 9a4064f..f418b30 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -201,6 +201,8 @@
>   
>   #define AHCI_COMMAND_TABLE_ACMD            0x40
>   
> +#define AHCI_PRDT_SIZE_MASK                0x3fffff
> +
>   #define IDE_FEATURE_DMA                    1
>   
>   #define READ_FPDMA_QUEUED                  0x60

Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC

[Eric Blake]

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

On 06/30/2014 06:09 AM, Alexander Graf wrote:
> 
> On 29.06.14 20:21, reza.jelveh@gmail.com wrote:
>> This requires a custom ovmf image with sata controller for testing [0]
>>
>> [0]: http://reza.jelveh.me/assets/OVMF.fd.bz2
>>
> 
> I guess this is supposed to be a cover letter? A few rules for cover
> letters:
> 
>   1) Cover letters only make sense for patch sets. This is a single
> patch, so you don't need one
>   2) Because they come with patch sets and you number patch sets, cover
> letters are [PATCH 0/n].
>   3) Usually cover letters contain git statistics. Try git format-patch
> --cover-letter. It will give you a nice template.
> 
> The usual way to add the comment you have here to a patch you're trying
> to submit it is to put volatile information (things that shouldn't end
> up in the git log) behind a "---" line in the commit message. Everything
> after that line gets ignored by git when the patch gets applied.

For more hints: http://wiki.qemu.org/Contribute/SubmitAPatch

(if anything said here isn't on that page, well, it's a wiki and we
should add it :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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