| Message ID | 1404213207-89115-1-git-send-email-reza.jelveh@tuhh.de |
|---|---|
| State | New |
| Headers | show |
On 01.07.14 13:13, reza.jelveh@tuhh.de wrote: > From: Reza Jelveh <reza.jelveh@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. > > Completion interrupts are triggered after every transaction instead of on > I-flag 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> > --- > This requires a custom ovmf image with sata controller for testing: > > http://reza.jelveh.me/assets/OVMF.fd.bz2 > > Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de> Reviewed-by: Alexander Graf <agraf@suse.de> I'm still puzzled that this ever worked at all ;). Alex
Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben: > From: Reza Jelveh <reza.jelveh@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. > > Completion interrupts are triggered after every transaction instead of on > I-flag 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> > --- > This requires a custom ovmf image with sata controller for testing: > > http://reza.jelveh.me/assets/OVMF.fd.bz2 > > Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de> Reviewed-by: Kevin Wolf <kwolf@redhat.com> The spec also seems to require an even byte count, which we don't seem to check. Do we want to add this? (In a separate patch, of course.) We'll also want a qtest case to verify the fix and for regression testing. John? And finally, please don't forget to CC the block maintainers (Stefan and me) for any AHCI patches. Kevin
On 01.07.14 13:36, Kevin Wolf wrote: > Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben: >> From: Reza Jelveh <reza.jelveh@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. >> >> Completion interrupts are triggered after every transaction instead of on >> I-flag 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> >> --- >> This requires a custom ovmf image with sata controller for testing: >> >> http://reza.jelveh.me/assets/OVMF.fd.bz2 >> >> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > > The spec also seems to require an even byte count, which we don't seem > to check. Do we want to add this? (In a separate patch, of course.) We could just remove the lowest bit in the mask, no? ;) Alex
On 07/01/2014 07:49 AM, Alexander Graf wrote: > > On 01.07.14 13:36, Kevin Wolf wrote: >> Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben: >>> From: Reza Jelveh <reza.jelveh@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. >>> >>> Completion interrupts are triggered after every transaction instead >>> of on >>> I-flag 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> >>> --- >>> This requires a custom ovmf image with sata controller for testing: >>> >>> http://reza.jelveh.me/assets/OVMF.fd.bz2 >>> >>> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de> >> Reviewed-by: Kevin Wolf <kwolf@redhat.com> >> >> The spec also seems to require an even byte count, which we don't seem >> to check. Do we want to add this? (In a separate patch, of course.) > > We could just remove the lowest bit in the mask, no? ;) > > > Alex > Reviewed-by: John Snow <jsnow@redhat.com> Taking a look at the spec, AHCI 1.3 sec 4.2.3.3 p. 40; a value of 0x00 implies one byte, and 0x01 implies two bytes. Masking off the one bit would probably lead to an off-by-one somewhere. The spec does state that it requires the 0th bit to be set, so in a separate patch we should check to make sure, but the mask as-is is appropriate. --John
On Tue, Jul 01, 2014 at 01:13:27PM +0200, reza.jelveh@tuhh.de wrote: > From: Reza Jelveh <reza.jelveh@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. > > Completion interrupts are triggered after every transaction instead of on > I-flag 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> > --- > This requires a custom ovmf image with sata controller for testing: > > http://reza.jelveh.me/assets/OVMF.fd.bz2 > > 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(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On 07/01/2014 07:36 AM, Kevin Wolf wrote: > Am 01.07.2014 um 13:13 hat reza.jelveh@tuhh.de geschrieben: >> From: Reza Jelveh <reza.jelveh@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. >> >> Completion interrupts are triggered after every transaction instead of on >> I-flag 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> >> --- >> This requires a custom ovmf image with sata controller for testing: >> >> http://reza.jelveh.me/assets/OVMF.fd.bz2 >> >> Signed-off-by: Reza Jelveh <reza.jelveh@tuhh.de> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > > The spec also seems to require an even byte count, which we don't seem > to check. Do we want to add this? (In a separate patch, of course.) > > We'll also want a qtest case to verify the fix and for regression > testing. John? > > And finally, please don't forget to CC the block maintainers (Stefan and > me) for any AHCI patches. > > Kevin > The test_identify case I submitted a patch for ( http://lists.gnu.org/archive/html/qemu-devel/2014-07/msg01241.html ) does attempt to use the interrupt bit, and as such, will fail without Reza's patch. The current version of the test only warns when it doesn't see the interrupt posted, though, but that's not relevant to testing this particular fix. --John
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9bae22e..cd140d1 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