Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC
On 06/30/2014 06:09 AM, Alexander Graf wrote: > > On 29.06.14 20:21, reza.jel...@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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC
On 29.06.14 20:21, reza.jel...@gmail.com wrote: From: Reza Jelveh 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 --- 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_ACMD0x40 +#define AHCI_PRDT_SIZE_MASK0x3f + #define IDE_FEATURE_DMA1 #define READ_FPDMA_QUEUED 0x60
Re: [Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC
On 29.06.14 20:21, reza.jel...@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
[Qemu-devel] [PATCH] ahci.c: mask unused flags when reading size PRDT DBC
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
From: Reza Jelveh 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 --- 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_ACMD0x40 +#define AHCI_PRDT_SIZE_MASK0x3f + #define IDE_FEATURE_DMA1 #define READ_FPDMA_QUEUED 0x60 -- 1.9.2