* [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
@ 2014-06-26 23:28 Reza Jelveh
2014-06-27 11:35 ` Paolo Bonzini
2014-06-27 15:23 ` John Snow
0 siblings, 2 replies; 6+ messages in thread
From: Reza Jelveh @ 2014-06-26 23:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Reza Jelveh
Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de>
---
hw/ide/ahci.c | 11 ++++++++---
hw/ide/ahci.h | 2 ++
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9bae22e..ee3613f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -639,6 +639,11 @@ 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 +686,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 +705,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
2014-06-26 23:28 [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT Reza Jelveh
@ 2014-06-27 11:35 ` Paolo Bonzini
2014-06-27 15:23 ` John Snow
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-06-27 11:35 UTC (permalink / raw)
To: Reza Jelveh, qemu-devel; +Cc: Reza Jelveh
Il 27/06/2014 01:28, Reza Jelveh ha scritto:
> +static int prdt_tbl_entry_size(const AHCI_SG tbl) {
> + return (le32_to_cpu(tbl.flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
> +}
Apart from the incorrect indentation/formatting here, the patch seems okay.
How can this be reproduced?
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
2014-06-26 23:28 [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT Reza Jelveh
2014-06-27 11:35 ` Paolo Bonzini
@ 2014-06-27 15:23 ` John Snow
2014-06-27 16:19 ` Alexander Graf
1 sibling, 1 reply; 6+ messages in thread
From: John Snow @ 2014-06-27 15:23 UTC (permalink / raw)
To: Reza Jelveh; +Cc: qemu-devel
On 06/26/2014 07:28 PM, Reza Jelveh wrote:
> +#define AHCI_PRDT_SIZE_MASK 0x3fffff
>
out of rampant curiosity, is there ever a case where the lower bits
might be set and the mask 0x3fffc is not desirable, or can we always
trust those bits to simply be off anyway?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
2014-06-27 15:23 ` John Snow
@ 2014-06-27 16:19 ` Alexander Graf
2014-06-27 16:33 ` Reza Jelveh
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Graf @ 2014-06-27 16:19 UTC (permalink / raw)
To: John Snow, Reza Jelveh; +Cc: qemu-devel
On 27.06.14 17:23, John Snow wrote:
>
> On 06/26/2014 07:28 PM, Reza Jelveh wrote:
>> +#define AHCI_PRDT_SIZE_MASK 0x3fffff
>>
> out of rampant curiosity, is there ever a case where the lower bits
> might be set and the mask 0x3fffc is not desirable, or can we always
> trust those bits to simply be off anyway?
We can't really trust anything from an OS :). But the reason for this
patch is that PRDT.I was set on some entries to enable notification of
the OS when the entry has been successfully processed.
We currently don't emulate the I bit correctly, but get away without
doing so. However, we have to make sure we mask it out when interpreting
the values. Hence the mask.
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 :).
Alex
PS: Is your name really John Snow? That is so cool!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
2014-06-27 16:19 ` Alexander Graf
@ 2014-06-27 16:33 ` Reza Jelveh
2014-06-27 16:38 ` Alexander Graf
0 siblings, 1 reply; 6+ messages in thread
From: Reza Jelveh @ 2014-06-27 16:33 UTC (permalink / raw)
To: Alexander Graf; +Cc: Reza Jelveh, John Snow, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 369 bytes --]
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.
[-- Attachment #2: 0001-ahci.c-mask-the-interrupt-on-complete-flag-to-allow-.patch --]
[-- Type: text/x-diff, Size: 2594 bytes --]
>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
The last PRDT usually sets the I bit set, but qemu does not emulate it. The I bit needs to be masked when interpreting the length.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT
2014-06-27 16:33 ` Reza Jelveh
@ 2014-06-27 16:38 ` Alexander Graf
0 siblings, 0 replies; 6+ messages in thread
From: Alexander Graf @ 2014-06-27 16:38 UTC (permalink / raw)
To: Reza Jelveh; +Cc: Reza Jelveh, John Snow, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-27 16:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-26 23:28 [Qemu-devel] [PATCH] ahci.c: mask the interrupt on complete flag to allow ahci.c to read the correct size for the PRDT Reza Jelveh
2014-06-27 11:35 ` Paolo Bonzini
2014-06-27 15:23 ` John Snow
2014-06-27 16:19 ` Alexander Graf
2014-06-27 16:33 ` Reza Jelveh
2014-06-27 16:38 ` Alexander Graf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.