Kevin Wolf <[email protected]> writes: > Am 04.07.2014 um 15:32 hat Markus Armbruster geschrieben: >> The block layer fails such reads and writes just fine. However, they >> then get treated like valid operations that fail: the error action >> gets executed. Unwanted; reporting the error to the guest is the only >> sensible action. >> >> Reject them before passing them to the block layer. This bypasses the >> error action and, for PIO but not DMA, I/O accounting. Tolerable, >> because I/O accounting is an inconsistent mess anyway. >> >> Signed-off-by: Markus Armbruster <[email protected]> >> --- >> hw/ide/core.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/hw/ide/core.c b/hw/ide/core.c >> index 3a38f1e..63a500d 100644 >> --- a/hw/ide/core.c >> +++ b/hw/ide/core.c >> @@ -499,6 +499,18 @@ static void ide_rw_error(IDEState *s) { >> ide_set_irq(s->bus); >> } >> >> +static bool ide_sect_range_ok(IDEState *s, >> + uint64_t sector, uint64_t nb_sectors) >> +{ >> + uint64_t total_sectors; >> + >> + bdrv_get_geometry(s->bs, &total_sectors); >> + if (sector > total_sectors || nb_sectors > total_sectors - sector) { >> + return false; >> + } >> + return true; >> +} >> + >> static void ide_sector_read_cb(void *opaque, int ret) >> { >> IDEState *s = opaque; >> @@ -554,6 +566,11 @@ void ide_sector_read(IDEState *s) >> printf("sector=%" PRId64 "\n", sector_num); >> #endif >> >> + if (!ide_sect_range_ok(s, sector_num, n)) { >> + ide_rw_error(s); >> + return; >> + } >> + >> s->iov.iov_base = s->io_buffer; >> s->iov.iov_len = n * BDRV_SECTOR_SIZE; >> qemu_iovec_init_external(&s->qiov, &s->iov, 1); >> @@ -671,6 +688,12 @@ void ide_dma_cb(void *opaque, int ret) >> sector_num, n, s->dma_cmd); >> #endif >> >> + if (!ide_sect_range_ok(s, sector_num, n)) { >> + dma_buf_commit(s); >> + ide_dma_error(s); >> + goto eot; > > Are you sure that this should be 'goto eot' rather than just 'return'? > When jumping to eot, we do the I/O accounting (which we said we don't > care about) and call ide_set_inactive() for a second time. The condition > for setting BM_STATUS_DMAING is never met when coming from here. > > I am worried about ide_set_inactive() doing double request cleanup.
You're right; I missed the fact that ide_dma_error() calls ide_set_inactive() already. Immediate return also skips the other things happening after eot, but that's okay, because: * skipping the bdrv_acct_done() merely changes I/O accounting to be busted somewhat differently, and * stay_active is certainly false, so we don't actually skip anything there. Respin sent.
