Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT

[Alexander Graf <agraf@suse.de>]

On 27.06.14 18:33, Reza Jelveh wrote:
> On 27/06/14 18:19, Alexander Graf wrote:
>> >I do agree that this should have been in the patch description. Reza,
>> >could you please repost this with a proper patch description and as a
>> >checkpatch.pl compliant patch? Also please CC me on the next iteration:).
> Sorry about this, I didn't see it when I posted the 2nd version of the patch.
>
> New version is attached.

Thanks a lot for the respin :).

Please don't attach patches, but send them as full new top-level emails 
with a slightly changed subject line.

>
> 0001-ahci.c-mask-the-interrupt-on-complete-flag-to-allow-.patch
>
>
>  From fd18e0a7f287cbe176dbb98a530dd54ea3cc27c7 Mon Sep 17 00:00:00 2001
> From: Reza Jelveh<reza.jelveh@tuhh.de>
> Date: Fri, 27 Jun 2014 01:13:19 +0200
> Subject: [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c
>   to read the correct size for the PRDT

This subject line is too long :). Also you want to say "[PATCH v2]" to 
indicate that this is version 2 of the patch.

>
> The last PRDT usually sets the I bit set,

sets the I bit set?

>   but qemu does not emulate it. The I bit needs to be masked when interpreting the length.

You should mention the reason why we need to mask it. The 32bit value 
we're looking at is split between different fields according to the 
specification. There is an I field, a reserved field and a size field. 
We only want to read the size, so we mask out the other fields.

>
> 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)

Wouldn't it make more sense to pass the table per reference (read: as a 
pointer)?

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

No double newline here please.

>   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

Please define the I mask along the way as well, so it's clear how to 
decode the field later :). Also the field you're decoding is called 
"flags_size". It'll probably be easier to understand that we're masking 
that field if the name pops up in this define again.


Alex