On Fri, Jul 27, 2012 at 09:59:01AM +0200, Kevin Wolf wrote: > Am 26.07.2012 21:40, schrieb Jason Baron: > > While testing q35, which has its cdrom attached to the ahci controller, I > > found > > that the Fedora 17 install would panic on boot. The panic occurs while > > squashfs is trying to read from the cdrom. The errors are: > > > > [ 8.622711] SQUASHFS error: xz_dec_run error, data probably corrupt > > [ 8.625180] SQUASHFS error: squashfs_read_data failed to read block > > 0x20be48a > > > > I was also able to produce corrupt data reads using an installed piix based > > qemu machine, using 'dd'. I found that the corruptions were only occuring > > when > > the read size was greater than 128k. For example, the following command > > results in corrupted reads: > > > > dd if=/dev/sr0 of=/tmp/blah bs=256k iflag=direct > > > > The > 128k size reads exercise a different code path than 128k and below. In > > ide_atapi_cmd_read_dma_cb() s->io_buffer_size is capped at 128k. Thus, > > ide_atapi_cmd_read_dma_cb() is called a second time when the read is > 128k. > > However, ahci_dma_rw_buf() restart the read from offset 0, instead of at > > 128k. > > Thus, resulting in a corrupted read. > > > > To fix this, I've introduced 'io_buffer_offset' field in IDEState to keep > > track of the offset. I've also modified ahci_populate_sglist() to take a new > > 3rd offset argument, so that the sglist is property initialized. > > > > I've tested this patch using 'dd' testing, and Fedora 17 now correctly boots > > and installs on q35 with the cdrom ahci controller. > > > > Signed-off-by: Jason Baron <jba...@redhat.com> > > --- > > hw/ide/ahci.c | 41 ++++++++++++++++++++++++++++++++++------- > > hw/ide/internal.h | 1 + > > 2 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > > index efea93f..9c95714 100644 > > --- a/hw/ide/ahci.c > > +++ b/hw/ide/ahci.c > > @@ -636,7 +636,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t > > *cmd_fis) > > } > > } > > > > -static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist) > > +static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int > > offset) > > { > > AHCICmdHdr *cmd = ad->cur_cmd; > > uint32_t opts = le32_to_cpu(cmd->opts); > > @@ -647,6 +647,10 @@ static int ahci_populate_sglist(AHCIDevice *ad, > > QEMUSGList *sglist) > > uint8_t *prdt; > > int i; > > int r = 0; > > + int sum = 0; > > + int off_idx = -1; > > + int off_pos = 0; > > + int tbl_entry_size; > > > > if (!sglist_alloc_hint) { > > DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts); > > @@ -669,10 +673,31 @@ static int ahci_populate_sglist(AHCIDevice *ad, > > QEMUSGList *sglist) > > /* Get entries in the PRDT, init a qemu sglist accordingly */ > > if (sglist_alloc_hint > 0) { > > AHCI_SG *tbl = (AHCI_SG *)prdt; > > - > > - qemu_sglist_init(sglist, sglist_alloc_hint, ad->hba->dma); > > + 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); > > + if (offset <= (sum + tbl_entry_size)) { > > + off_idx = i; > > + off_pos = offset - sum; > > + break; > > + } > > + sum += tbl_entry_size; > > + } > > + if ((off_idx == -1) || (off_pos < 0) || (off_pos > > > tbl_entry_size)) { > > + fprintf(stderr, "%s: Incorrect offset! " > > + "off_idx: %d, off_pos: %d\n", > > + __func__, off_idx, off_pos); > > I would use DPRINTF here, it's not a good idea to allow the guest to > trigger fprintfs. >
ok. I'm also wondering if we need to 0 s->io_buffer_offset in more places than 'ahci_start_dma()'. For example, do we need it in ahci_dma_reset(), or ahci_dma_prepare_buf()? hw/ide/pci.c seems to do that. I didn't include those places in my original patch, because I saw that start_dma was always called before rw_buf. But perhaps, there is another codepath that I missed. Thanks, -Jason