From 07e415f2398d9cfb21cdd5ef902445032ba54556 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:31 +0200 Subject: [PATCH 01/12] fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase() What all callers of fdctrl_reset_fifo() really want to do is to start the command phase, where writes to the data port initiate a new command. The function doesn't only clear the FIFO, but also sets up the state so that a new command can be received. Rename it to reflect this. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-2-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index d8a8edd936..9f95ace97c 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -324,7 +324,7 @@ static void fd_revalidate(FDrive *drv) /* Intel 82078 floppy disk controller emulation */ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq); -static void fdctrl_reset_fifo(FDCtrl *fdctrl); +static void fdctrl_to_command_phase(FDCtrl *fdctrl); static int fdctrl_transfer_handler (void *opaque, int nchan, int dma_pos, int dma_len); static void fdctrl_raise_irq(FDCtrl *fdctrl); @@ -918,7 +918,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq) fdctrl->data_dir = FD_DIR_WRITE; for (i = 0; i < MAX_FD; i++) fd_recalibrate(&fdctrl->drives[i]); - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); if (do_irq) { fdctrl->status0 |= FD_SR0_RDYCHG; fdctrl_raise_irq(fdctrl); @@ -1134,8 +1134,8 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl) return retval; } -/* FIFO state control */ -static void fdctrl_reset_fifo(FDCtrl *fdctrl) +/* Clear the FIFO and update the state for receiving the next command */ +static void fdctrl_to_command_phase(FDCtrl *fdctrl) { fdctrl->data_dir = FD_DIR_WRITE; fdctrl->data_pos = 0; @@ -1533,7 +1533,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) if (fdctrl->msr & FD_MSR_NONDMA) { fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); } else { - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); fdctrl_reset_irq(fdctrl); } } @@ -1667,7 +1667,7 @@ static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction) fdctrl->config = fdctrl->fifo[11]; fdctrl->precomp_trk = fdctrl->fifo[12]; fdctrl->pwrd = fdctrl->fifo[13]; - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } static void fdctrl_handle_save(FDCtrl *fdctrl, int direction) @@ -1746,7 +1746,7 @@ static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction) else fdctrl->dor |= FD_DOR_DMAEN; /* No result back */ - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction) @@ -1772,7 +1772,7 @@ static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction) SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK); cur_drv = get_cur_drv(fdctrl); fd_recalibrate(cur_drv); - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); /* Raise Interrupt */ fdctrl->status0 |= FD_SR0_SEEK; fdctrl_raise_irq(fdctrl); @@ -1808,7 +1808,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction) SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK); cur_drv = get_cur_drv(fdctrl); - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); /* The seek command just sends step pulses to the drive and doesn't care if * there is a medium inserted of if it's banging the head against the drive. */ @@ -1825,7 +1825,7 @@ static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction) if (fdctrl->fifo[1] & 0x80) cur_drv->perpendicular = fdctrl->fifo[1] & 0x7; /* No result back */ - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction) @@ -1833,7 +1833,7 @@ static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction) fdctrl->config = fdctrl->fifo[2]; fdctrl->precomp_trk = fdctrl->fifo[3]; /* No result back */ - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction) @@ -1846,7 +1846,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction) static void fdctrl_handle_option(FDCtrl *fdctrl, int direction) { /* No result back */ - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction) @@ -1864,7 +1864,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct fdctrl->fifo[3] = 0; fdctrl_set_fifo(fdctrl, 4); } else { - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); } } else if (fdctrl->data_len > 7) { /* ERROR */ @@ -1887,7 +1887,7 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction) fd_seek(cur_drv, cur_drv->head, cur_drv->track + fdctrl->fifo[2], cur_drv->sect, 1); } - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); /* Raise Interrupt */ fdctrl->status0 |= FD_SR0_SEEK; fdctrl_raise_irq(fdctrl); @@ -1905,7 +1905,7 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction) fd_seek(cur_drv, cur_drv->head, cur_drv->track - fdctrl->fifo[2], cur_drv->sect, 1); } - fdctrl_reset_fifo(fdctrl); + fdctrl_to_command_phase(fdctrl); /* Raise Interrupt */ fdctrl->status0 |= FD_SR0_SEEK; fdctrl_raise_irq(fdctrl); From 83a260135f13db8b5d7df72090864a5ebcef2845 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:32 +0200 Subject: [PATCH 02/12] fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase() What callers really do with this function is to switch from execution phase (including data transfers) to result phase where the guest can read out one or more status bytes from the FIFO (the number depends on the command). Rename the function accordingly. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-3-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 9f95ace97c..8c41434ad2 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1142,8 +1142,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl) fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO); } -/* Set FIFO status for the host to read */ -static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len) +/* Update the state to allow the guest to read out the command status. + * @fifo_len is the number of result bytes to be read out. */ +static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len) { fdctrl->data_dir = FD_DIR_READ; fdctrl->data_len = fifo_len; @@ -1157,7 +1158,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction) qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n", fdctrl->fifo[0]); fdctrl->fifo[0] = FD_SR0_INVCMD; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } /* Seek to next sector @@ -1238,7 +1239,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0, fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO; fdctrl->msr &= ~FD_MSR_NONDMA; - fdctrl_set_fifo(fdctrl, 7); + fdctrl_to_result_phase(fdctrl, 7); fdctrl_raise_irq(fdctrl); } @@ -1606,7 +1607,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction) { fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0; fdctrl->fifo[0] = fdctrl->lock << 4; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction) @@ -1631,20 +1632,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction) (cur_drv->perpendicular << 2); fdctrl->fifo[8] = fdctrl->config; fdctrl->fifo[9] = fdctrl->precomp_trk; - fdctrl_set_fifo(fdctrl, 10); + fdctrl_to_result_phase(fdctrl, 10); } static void fdctrl_handle_version(FDCtrl *fdctrl, int direction) { /* Controller's version */ fdctrl->fifo[0] = fdctrl->version; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction) { fdctrl->fifo[0] = 0x41; /* Stepping 1 */ - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction) @@ -1697,7 +1698,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction) fdctrl->fifo[12] = fdctrl->pwrd; fdctrl->fifo[13] = 0; fdctrl->fifo[14] = 0; - fdctrl_set_fifo(fdctrl, 15); + fdctrl_to_result_phase(fdctrl, 15); } static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction) @@ -1762,7 +1763,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction) (cur_drv->head << 2) | GET_CUR_DRV(fdctrl) | 0x28; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction) @@ -1788,7 +1789,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction) fdctrl->reset_sensei--; } else if (!(fdctrl->sra & FD_SRA_INTPEND)) { fdctrl->fifo[0] = FD_SR0_INVCMD; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); return; } else { fdctrl->fifo[0] = @@ -1797,7 +1798,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction) } fdctrl->fifo[1] = cur_drv->track; - fdctrl_set_fifo(fdctrl, 2); + fdctrl_to_result_phase(fdctrl, 2); fdctrl_reset_irq(fdctrl); fdctrl->status0 = FD_SR0_RDYCHG; } @@ -1840,7 +1841,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction) { fdctrl->pwrd = fdctrl->fifo[1]; fdctrl->fifo[0] = fdctrl->fifo[1]; - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } static void fdctrl_handle_option(FDCtrl *fdctrl, int direction) @@ -1862,7 +1863,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct fdctrl->fifo[0] = fdctrl->fifo[1]; fdctrl->fifo[2] = 0; fdctrl->fifo[3] = 0; - fdctrl_set_fifo(fdctrl, 4); + fdctrl_to_result_phase(fdctrl, 4); } else { fdctrl_to_command_phase(fdctrl); } @@ -1870,7 +1871,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct /* ERROR */ fdctrl->fifo[0] = 0x80 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl); - fdctrl_set_fifo(fdctrl, 1); + fdctrl_to_result_phase(fdctrl, 1); } } From 85d291a08c91c07927bbbd29f72a27d3ad7478f3 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:33 +0200 Subject: [PATCH 03/12] fdc: Introduce fdctrl->phase The floppy controller spec describes three different controller phases, which are currently not explicitly modelled in our emulation. Instead, each phase is represented by a combination of flags in registers. This patch makes explicit in which phase the controller currently is. Signed-off-by: Kevin Wolf Acked-by: John Snow Message-id: 1432214378-31891-4-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 8c41434ad2..f5bcf0b145 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -495,6 +495,33 @@ enum { FD_DIR_DSKCHG = 0x80, }; +/* + * See chapter 5.0 "Controller phases" of the spec: + * + * Command phase: + * The host writes a command and its parameters into the FIFO. The command + * phase is completed when all parameters for the command have been supplied, + * and execution phase is entered. + * + * Execution phase: + * Data transfers, either DMA or non-DMA. For non-DMA transfers, the FIFO + * contains the payload now, otherwise it's unused. When all bytes of the + * required data have been transferred, the state is switched to either result + * phase (if the command produces status bytes) or directly back into the + * command phase for the next command. + * + * Result phase: + * The host reads out the FIFO, which contains one or more result bytes now. + */ +enum { + /* Only for migration: reconstruct phase from registers like qemu 2.3 */ + FD_PHASE_RECONSTRUCT = 0, + + FD_PHASE_COMMAND = 1, + FD_PHASE_EXECUTION = 2, + FD_PHASE_RESULT = 3, +}; + #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI) #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT) @@ -504,6 +531,7 @@ struct FDCtrl { /* Controller state */ QEMUTimer *result_timer; int dma_chann; + uint8_t phase; /* Controller's identification */ uint8_t version; /* HW */ @@ -744,6 +772,28 @@ static const VMStateDescription vmstate_fdrive = { } }; +/* + * Reconstructs the phase from register values according to the logic that was + * implemented in qemu 2.3. This is the default value that is used if the phase + * subsection is not present on migration. + * + * Don't change this function to reflect newer qemu versions, it is part of + * the migration ABI. + */ +static int reconstruct_phase(FDCtrl *fdctrl) +{ + if (fdctrl->msr & FD_MSR_NONDMA) { + return FD_PHASE_EXECUTION; + } else if ((fdctrl->msr & FD_MSR_RQM) == 0) { + /* qemu 2.3 disabled RQM only during DMA transfers */ + return FD_PHASE_EXECUTION; + } else if (fdctrl->msr & FD_MSR_DIO) { + return FD_PHASE_RESULT; + } else { + return FD_PHASE_COMMAND; + } +} + static void fdc_pre_save(void *opaque) { FDCtrl *s = opaque; @@ -751,12 +801,24 @@ static void fdc_pre_save(void *opaque) s->dor_vmstate = s->dor | GET_CUR_DRV(s); } +static int fdc_pre_load(void *opaque) +{ + FDCtrl *s = opaque; + s->phase = FD_PHASE_RECONSTRUCT; + return 0; +} + static int fdc_post_load(void *opaque, int version_id) { FDCtrl *s = opaque; SET_CUR_DRV(s, s->dor_vmstate & FD_DOR_SELMASK); s->dor = s->dor_vmstate & ~FD_DOR_SELMASK; + + if (s->phase == FD_PHASE_RECONSTRUCT) { + s->phase = reconstruct_phase(s); + } + return 0; } @@ -794,11 +856,29 @@ static const VMStateDescription vmstate_fdc_result_timer = { } }; +static bool fdc_phase_needed(void *opaque) +{ + FDCtrl *fdctrl = opaque; + + return reconstruct_phase(fdctrl) != fdctrl->phase; +} + +static const VMStateDescription vmstate_fdc_phase = { + .name = "fdc/phase", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(phase, FDCtrl), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_fdc = { .name = "fdc", .version_id = 2, .minimum_version_id = 2, .pre_save = fdc_pre_save, + .pre_load = fdc_pre_load, .post_load = fdc_post_load, .fields = (VMStateField[]) { /* Controller State */ @@ -838,6 +918,9 @@ static const VMStateDescription vmstate_fdc = { } , { .vmsd = &vmstate_fdc_result_timer, .needed = fdc_result_timer_needed, + } , { + .vmsd = &vmstate_fdc_phase, + .needed = fdc_phase_needed, } , { /* empty */ } @@ -1137,6 +1220,7 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl) /* Clear the FIFO and update the state for receiving the next command */ static void fdctrl_to_command_phase(FDCtrl *fdctrl) { + fdctrl->phase = FD_PHASE_COMMAND; fdctrl->data_dir = FD_DIR_WRITE; fdctrl->data_pos = 0; fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO); @@ -1146,6 +1230,7 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl) * @fifo_len is the number of result bytes to be read out. */ static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len) { + fdctrl->phase = FD_PHASE_RESULT; fdctrl->data_dir = FD_DIR_READ; fdctrl->data_len = fifo_len; fdctrl->data_pos = 0; @@ -1912,6 +1997,9 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction) fdctrl_raise_irq(fdctrl); } +/* + * Handlers for the execution phase of each command + */ static const struct { uint8_t value; uint8_t mask; @@ -2015,6 +2103,7 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) /* We now have all parameters * and will be able to treat the command */ + fdctrl->phase = FD_PHASE_EXECUTION; if (fdctrl->data_state & FD_STATE_FORMAT) { fdctrl_format_sector(fdctrl); return; From 5b0a25e8d2f15f89255c745c71d297b5b24d138c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:34 +0200 Subject: [PATCH 04/12] fdc: Use phase in fdctrl_write_data() Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-5-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 69 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index f5bcf0b145..e3515a1bf8 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -2059,8 +2059,12 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) return; } fdctrl->dsr &= ~FD_DSR_PWRDOWN; - /* Is it write command time ? */ - if (fdctrl->msr & FD_MSR_NONDMA) { + + switch (fdctrl->phase) { + case FD_PHASE_EXECUTION: + /* For DMA requests, RQM should be cleared during execution phase, so + * we would have errored out above. */ + assert(fdctrl->msr & FD_MSR_NONDMA); /* FIFO data write */ pos = fdctrl->data_pos++; pos %= FD_SECTOR_LEN; @@ -2072,12 +2076,12 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) < 0) { FLOPPY_DPRINTF("error writing sector %d\n", fd_sector(cur_drv)); - return; + break; } if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { FLOPPY_DPRINTF("error seeking to next sector %d\n", fd_sector(cur_drv)); - return; + break; } } /* Switch from transfer mode to status mode @@ -2085,33 +2089,42 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) */ if (fdctrl->data_pos == fdctrl->data_len) fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); - return; - } - if (fdctrl->data_pos == 0) { - /* Command */ - pos = command_to_handler[value & 0xff]; - FLOPPY_DPRINTF("%s command\n", handlers[pos].name); - fdctrl->data_len = handlers[pos].parameters + 1; - fdctrl->msr |= FD_MSR_CMDBUSY; - } + break; - FLOPPY_DPRINTF("%s: %02x\n", __func__, value); - pos = fdctrl->data_pos++; - pos %= FD_SECTOR_LEN; - fdctrl->fifo[pos] = value; - if (fdctrl->data_pos == fdctrl->data_len) { - /* We now have all parameters - * and will be able to treat the command - */ - fdctrl->phase = FD_PHASE_EXECUTION; - if (fdctrl->data_state & FD_STATE_FORMAT) { - fdctrl_format_sector(fdctrl); - return; + case FD_PHASE_COMMAND: + assert(!(fdctrl->msr & FD_MSR_NONDMA)); + + if (fdctrl->data_pos == 0) { + /* Command */ + pos = command_to_handler[value & 0xff]; + FLOPPY_DPRINTF("%s command\n", handlers[pos].name); + fdctrl->data_len = handlers[pos].parameters + 1; + fdctrl->msr |= FD_MSR_CMDBUSY; } - pos = command_to_handler[fdctrl->fifo[0] & 0xff]; - FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name); - (*handlers[pos].handler)(fdctrl, handlers[pos].direction); + FLOPPY_DPRINTF("%s: %02x\n", __func__, value); + pos = fdctrl->data_pos++; + pos %= FD_SECTOR_LEN; + fdctrl->fifo[pos] = value; + if (fdctrl->data_pos == fdctrl->data_len) { + /* We now have all parameters + * and will be able to treat the command + */ + fdctrl->phase = FD_PHASE_EXECUTION; + if (fdctrl->data_state & FD_STATE_FORMAT) { + fdctrl_format_sector(fdctrl); + break; + } + + pos = command_to_handler[fdctrl->fifo[0] & 0xff]; + FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name); + (*handlers[pos].handler)(fdctrl, handlers[pos].direction); + } + break; + + case FD_PHASE_RESULT: + default: + abort(); } } From d275b33d76c8ed9d5a3dca22ea0fdec8d5a5c8e6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:35 +0200 Subject: [PATCH 05/12] fdc: Code cleanup in fdctrl_write_data() Factor out a few common lines of code, reformat, improve comments. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-6-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 63 +++++++++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index e3515a1bf8..1cc1e3a984 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -2000,14 +2000,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction) /* * Handlers for the execution phase of each command */ -static const struct { +typedef struct FDCtrlCommand { uint8_t value; uint8_t mask; const char* name; int parameters; void (*handler)(FDCtrl *fdctrl, int direction); int direction; -} handlers[] = { +} FDCtrlCommand; + +static const FDCtrlCommand handlers[] = { { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ }, { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE }, { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek }, @@ -2044,9 +2046,19 @@ static const struct { /* Associate command to an index in the 'handlers' array */ static uint8_t command_to_handler[256]; +static const FDCtrlCommand *get_command(uint8_t cmd) +{ + int idx; + + idx = command_to_handler[cmd]; + FLOPPY_DPRINTF("%s command\n", handlers[idx].name); + return &handlers[idx]; +} + static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) { FDrive *cur_drv; + const FDCtrlCommand *cmd; uint32_t pos; /* Reset mode */ @@ -2060,15 +2072,22 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) } fdctrl->dsr &= ~FD_DSR_PWRDOWN; + FLOPPY_DPRINTF("%s: %02x\n", __func__, value); + + /* If data_len spans multiple sectors, the current position in the FIFO + * wraps around while fdctrl->data_pos is the real position in the whole + * request. */ + pos = fdctrl->data_pos++; + pos %= FD_SECTOR_LEN; + fdctrl->fifo[pos] = value; + switch (fdctrl->phase) { case FD_PHASE_EXECUTION: /* For DMA requests, RQM should be cleared during execution phase, so * we would have errored out above. */ assert(fdctrl->msr & FD_MSR_NONDMA); + /* FIFO data write */ - pos = fdctrl->data_pos++; - pos %= FD_SECTOR_LEN; - fdctrl->fifo[pos] = value; if (pos == FD_SECTOR_LEN - 1 || fdctrl->data_pos == fdctrl->data_len) { cur_drv = get_cur_drv(fdctrl); @@ -2084,41 +2103,37 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) break; } } - /* Switch from transfer mode to status mode - * then from status mode to command mode - */ - if (fdctrl->data_pos == fdctrl->data_len) + + /* Switch to result phase when done with the transfer */ + if (fdctrl->data_pos == fdctrl->data_len) { fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); + } break; case FD_PHASE_COMMAND: assert(!(fdctrl->msr & FD_MSR_NONDMA)); + assert(fdctrl->data_pos < FD_SECTOR_LEN); - if (fdctrl->data_pos == 0) { - /* Command */ - pos = command_to_handler[value & 0xff]; - FLOPPY_DPRINTF("%s command\n", handlers[pos].name); - fdctrl->data_len = handlers[pos].parameters + 1; + if (pos == 0) { + /* The first byte specifies the command. Now we start reading + * as many parameters as this command requires. */ + cmd = get_command(value); + fdctrl->data_len = cmd->parameters + 1; fdctrl->msr |= FD_MSR_CMDBUSY; } - FLOPPY_DPRINTF("%s: %02x\n", __func__, value); - pos = fdctrl->data_pos++; - pos %= FD_SECTOR_LEN; - fdctrl->fifo[pos] = value; if (fdctrl->data_pos == fdctrl->data_len) { - /* We now have all parameters - * and will be able to treat the command - */ + /* We have all parameters now, execute the command */ fdctrl->phase = FD_PHASE_EXECUTION; + if (fdctrl->data_state & FD_STATE_FORMAT) { fdctrl_format_sector(fdctrl); break; } - pos = command_to_handler[fdctrl->fifo[0] & 0xff]; - FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name); - (*handlers[pos].handler)(fdctrl, handlers[pos].direction); + cmd = get_command(fdctrl->fifo[0]); + FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name); + cmd->handler(fdctrl, cmd->direction); } break; From f6c2d1d8425fd0ca450d515b06821e2224d4b43c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:36 +0200 Subject: [PATCH 06/12] fdc: Disentangle phases in fdctrl_read_data() This commit makes similar improvements as have already been made to the write function: Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-7-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 1cc1e3a984..3c64194f05 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1591,9 +1591,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) FLOPPY_DPRINTF("error: controller not ready for reading\n"); return 0; } + + /* If data_len spans multiple sectors, the current position in the FIFO + * wraps around while fdctrl->data_pos is the real position in the whole + * request. */ pos = fdctrl->data_pos; pos %= FD_SECTOR_LEN; - if (fdctrl->msr & FD_MSR_NONDMA) { + + switch (fdctrl->phase) { + case FD_PHASE_EXECUTION: + assert(fdctrl->msr & FD_MSR_NONDMA); if (pos == 0) { if (fdctrl->data_pos != 0) if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { @@ -1609,20 +1616,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) memset(fdctrl->fifo, 0, FD_SECTOR_LEN); } } - } - retval = fdctrl->fifo[pos]; - if (++fdctrl->data_pos == fdctrl->data_len) { - fdctrl->data_pos = 0; - /* Switch from transfer mode to status mode - * then from status mode to command mode - */ - if (fdctrl->msr & FD_MSR_NONDMA) { + + if (++fdctrl->data_pos == fdctrl->data_len) { fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); - } else { + } + break; + + case FD_PHASE_RESULT: + assert(!(fdctrl->msr & FD_MSR_NONDMA)); + if (++fdctrl->data_pos == fdctrl->data_len) { fdctrl_to_command_phase(fdctrl); fdctrl_reset_irq(fdctrl); } + break; + + case FD_PHASE_COMMAND: + default: + abort(); } + + retval = fdctrl->fifo[pos]; FLOPPY_DPRINTF("data register: 0x%02x\n", retval); return retval; From 6cc8a11c84ddc18c64fc88d54c8e9dca24ada489 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:37 +0200 Subject: [PATCH 07/12] fdc: Fix MSR.RQM flag The RQM bit in MSR should be set whenever the guest is supposed to access the FIFO, and it should be cleared in all other cases. This is important so the guest can't continue writing/reading the FIFO beyond the length that it's suppossed to access (see CVE-2015-3456). Commit e9077462 fixed the CVE by adding code that avoids the buffer overflow; however it doesn't correct the wrong behaviour of the floppy controller which should already have cleared RQM. Currently, RQM stays set all the time and during all phases while a command is being processed. This is error-prone because the command has to explicitly clear the flag if it doesn't need data (and indeed, the two buggy commands that are the culprits for the CVE just forgot to do that). This patch clears RQM immediately as soon as all bytes that are expected have been received. If the the FIFO is used in the next phase, the flag has to be set explicitly there. It also clear RQM after receiving all bytes even if the phase transition immediately sets it again. While it's technically not necessary at the moment because the state between clearing and setting RQM is not observable by the guest, this is more explicit and matches how real hardware works. It will actually become necessary in qemu once asynchronous code paths are introduced. This alone should have been enough to fix the CVE, but now we have two lines of defense - even better. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-8-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- hw/block/fdc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 3c64194f05..6e794597dc 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1223,7 +1223,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl) fdctrl->phase = FD_PHASE_COMMAND; fdctrl->data_dir = FD_DIR_WRITE; fdctrl->data_pos = 0; + fdctrl->data_len = 1; /* Accept command byte, adjust for params later */ fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO); + fdctrl->msr |= FD_MSR_RQM; } /* Update the state to allow the guest to read out the command status. @@ -1438,7 +1440,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction) } } FLOPPY_DPRINTF("start non-DMA transfer\n"); - fdctrl->msr |= FD_MSR_NONDMA; + fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM; if (direction != FD_DIR_WRITE) fdctrl->msr |= FD_MSR_DIO; /* IO based transfer: calculate len */ @@ -1618,6 +1620,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) } if (++fdctrl->data_pos == fdctrl->data_len) { + fdctrl->msr &= ~FD_MSR_RQM; fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); } break; @@ -1625,6 +1628,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) case FD_PHASE_RESULT: assert(!(fdctrl->msr & FD_MSR_NONDMA)); if (++fdctrl->data_pos == fdctrl->data_len) { + fdctrl->msr &= ~FD_MSR_RQM; fdctrl_to_command_phase(fdctrl); fdctrl_reset_irq(fdctrl); } @@ -2094,6 +2098,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) pos %= FD_SECTOR_LEN; fdctrl->fifo[pos] = value; + if (fdctrl->data_pos == fdctrl->data_len) { + fdctrl->msr &= ~FD_MSR_RQM; + } + switch (fdctrl->phase) { case FD_PHASE_EXECUTION: /* For DMA requests, RQM should be cleared during execution phase, so @@ -2132,6 +2140,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value) * as many parameters as this command requires. */ cmd = get_command(value); fdctrl->data_len = cmd->parameters + 1; + if (cmd->parameters) { + fdctrl->msr |= FD_MSR_RQM; + } fdctrl->msr |= FD_MSR_CMDBUSY; } From 4964e18e490f3ecad35c9e4cc9b613316a98755e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 21 May 2015 15:19:38 +0200 Subject: [PATCH 08/12] fdc-test: Test state for existing cases more thoroughly This just adds a few additional checks of the MSR and interrupt pin to the already existing test cases. Signed-off-by: Kevin Wolf Reviewed-by: John Snow Message-id: 1432214378-31891-9-git-send-email-kwolf@redhat.com Signed-off-by: John Snow --- tests/fdc-test.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/fdc-test.c b/tests/fdc-test.c index 3c6c83cac4..416394fc77 100644 --- a/tests/fdc-test.c +++ b/tests/fdc-test.c @@ -218,6 +218,10 @@ static uint8_t send_read_no_dma_command(int nb_sect, uint8_t expected_st0) inb(FLOPPY_BASE + reg_fifo); } + msr = inb(FLOPPY_BASE + reg_msr); + assert_bit_set(msr, BUSY | RQM | DIO); + g_assert(get_irq(FLOPPY_IRQ)); + st0 = floppy_recv(); if (st0 != expected_st0) { ret = 1; @@ -228,8 +232,15 @@ static uint8_t send_read_no_dma_command(int nb_sect, uint8_t expected_st0) floppy_recv(); floppy_recv(); floppy_recv(); + g_assert(get_irq(FLOPPY_IRQ)); floppy_recv(); + /* Check that we're back in command phase */ + msr = inb(FLOPPY_BASE + reg_msr); + assert_bit_clear(msr, BUSY | DIO); + assert_bit_set(msr, RQM); + g_assert(!get_irq(FLOPPY_IRQ)); + return ret; } @@ -403,6 +414,7 @@ static void test_read_id(void) uint8_t head = 0; uint8_t cyl; uint8_t st0; + uint8_t msr; /* Seek to track 0 and check with READ ID */ send_seek(0); @@ -411,18 +423,29 @@ static void test_read_id(void) g_assert(!get_irq(FLOPPY_IRQ)); floppy_send(head << 2 | drive); + msr = inb(FLOPPY_BASE + reg_msr); + if (!get_irq(FLOPPY_IRQ)) { + assert_bit_set(msr, BUSY); + assert_bit_clear(msr, RQM); + } + while (!get_irq(FLOPPY_IRQ)) { /* qemu involves a timer with READ ID... */ clock_step(1000000000LL / 50); } + msr = inb(FLOPPY_BASE + reg_msr); + assert_bit_set(msr, BUSY | RQM | DIO); + st0 = floppy_recv(); floppy_recv(); floppy_recv(); cyl = floppy_recv(); head = floppy_recv(); floppy_recv(); + g_assert(get_irq(FLOPPY_IRQ)); floppy_recv(); + g_assert(!get_irq(FLOPPY_IRQ)); g_assert_cmpint(cyl, ==, 0); g_assert_cmpint(head, ==, 0); @@ -443,18 +466,29 @@ static void test_read_id(void) g_assert(!get_irq(FLOPPY_IRQ)); floppy_send(head << 2 | drive); + msr = inb(FLOPPY_BASE + reg_msr); + if (!get_irq(FLOPPY_IRQ)) { + assert_bit_set(msr, BUSY); + assert_bit_clear(msr, RQM); + } + while (!get_irq(FLOPPY_IRQ)) { /* qemu involves a timer with READ ID... */ clock_step(1000000000LL / 50); } + msr = inb(FLOPPY_BASE + reg_msr); + assert_bit_set(msr, BUSY | RQM | DIO); + st0 = floppy_recv(); floppy_recv(); floppy_recv(); cyl = floppy_recv(); head = floppy_recv(); floppy_recv(); + g_assert(get_irq(FLOPPY_IRQ)); floppy_recv(); + g_assert(!get_irq(FLOPPY_IRQ)); g_assert_cmpint(cyl, ==, 8); g_assert_cmpint(head, ==, 1); From 0389b8f8c7688fe512e16bdc00c5f35d2d8df12c Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Thu, 4 Jun 2015 22:59:34 +0100 Subject: [PATCH 09/12] macio: switch pmac_dma_read() over to new offset/len implementation For better handling of unaligned block device accesses. Signed-off-by: Mark Cave-Ayland Reviewed-by: John Snow Message-id: 1433455177-21243-2-git-send-email-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/macio.c | 106 ++++++++++++++++++++----------------------------- 1 file changed, 42 insertions(+), 64 deletions(-) diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 585a27bd6c..52ee4ac6fa 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -52,7 +52,7 @@ static const int debug_macio = 0; #define MACIO_PAGE_SIZE 4096 static void pmac_dma_read(BlockBackend *blk, - int64_t sector_num, int nb_sectors, + int64_t offset, unsigned int bytes, void (*cb)(void *opaque, int ret), void *opaque) { DBDMA_io *io = opaque; @@ -60,76 +60,48 @@ static void pmac_dma_read(BlockBackend *blk, IDEState *s = idebus_active_if(&m->bus); dma_addr_t dma_addr, dma_len; void *mem; - int nsector, remainder; + int64_t sector_num; + int nsector; + uint64_t align = BDRV_SECTOR_SIZE; + size_t head_bytes, tail_bytes; qemu_iovec_destroy(&io->iov); qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1); - if (io->remainder_len > 0) { - /* Return remainder of request */ - int transfer = MIN(io->remainder_len, io->len); + sector_num = (offset >> 9); + nsector = (io->len >> 9); - MACIO_DPRINTF("--- DMA read pop - bounce addr: %p addr: %" - HWADDR_PRIx " remainder_len: %x\n", - &io->remainder + (0x200 - transfer), io->addr, - io->remainder_len); - - cpu_physical_memory_write(io->addr, - &io->remainder + (0x200 - transfer), - transfer); - - io->remainder_len -= transfer; - io->len -= transfer; - io->addr += transfer; - - s->io_buffer_index += transfer; - s->io_buffer_size -= transfer; - - if (io->remainder_len != 0) { - /* Still waiting for remainder */ - return; - } - - if (io->len == 0) { - MACIO_DPRINTF("--- finished all read processing; go and finish\n"); - cb(opaque, 0); - return; - } - } - - if (s->drive_kind == IDE_CD) { - sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9); - } else { - sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9); - } - - nsector = ((io->len + 0x1ff) >> 9); - remainder = (nsector << 9) - io->len; - - MACIO_DPRINTF("--- DMA read transfer - addr: %" HWADDR_PRIx " len: %x\n", - io->addr, io->len); + MACIO_DPRINTF("--- DMA read transfer (0x%" HWADDR_PRIx ",0x%x): " + "sector_num: %" PRId64 ", nsector: %d\n", io->addr, io->len, + sector_num, nsector); dma_addr = io->addr; dma_len = io->len; mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len, DMA_DIRECTION_FROM_DEVICE); - if (!remainder) { - MACIO_DPRINTF("--- DMA read aligned - addr: %" HWADDR_PRIx - " len: %x\n", io->addr, io->len); - qemu_iovec_add(&io->iov, mem, io->len); - } else { - MACIO_DPRINTF("--- DMA read unaligned - addr: %" HWADDR_PRIx - " len: %x\n", io->addr, io->len); - qemu_iovec_add(&io->iov, mem, io->len); + if (offset & (align - 1)) { + head_bytes = offset & (align - 1); - MACIO_DPRINTF("--- DMA read push - bounce addr: %p " - "remainder_len: %x\n", - &io->remainder + 0x200 - remainder, remainder); - qemu_iovec_add(&io->iov, &io->remainder + 0x200 - remainder, - remainder); + MACIO_DPRINTF("--- DMA unaligned head: sector %" PRId64 ", " + "discarding %zu bytes\n", sector_num, head_bytes); - io->remainder_len = remainder; + qemu_iovec_add(&io->iov, &io->remainder, head_bytes); + + bytes += offset & (align - 1); + offset = offset & ~(align - 1); + } + + qemu_iovec_add(&io->iov, mem, io->len); + + if ((offset + bytes) & (align - 1)) { + tail_bytes = (offset + bytes) & (align - 1); + + MACIO_DPRINTF("--- DMA unaligned tail: sector %" PRId64 ", " + "discarding bytes %zu\n", sector_num, tail_bytes); + + qemu_iovec_add(&io->iov, &io->remainder, align - tail_bytes); + bytes = ROUND_UP(bytes, align); } s->io_buffer_size -= io->len; @@ -137,11 +109,11 @@ static void pmac_dma_read(BlockBackend *blk, io->len = 0; - MACIO_DPRINTF("--- Block read transfer - sector_num: %"PRIx64" " - "nsector: %x\n", - sector_num, nsector); + MACIO_DPRINTF("--- Block read transfer - sector_num: %" PRIx64 " " + "nsector: %x\n", (offset >> 9), (bytes >> 9)); - m->aiocb = blk_aio_readv(blk, sector_num, &io->iov, nsector, cb, io); + m->aiocb = blk_aio_readv(blk, (offset >> 9), &io->iov, (bytes >> 9), + cb, io); } static void pmac_dma_write(BlockBackend *blk, @@ -246,6 +218,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) IDEState *s = idebus_active_if(&m->bus); int64_t sector_num; int nsector, remainder; + int64_t offset; MACIO_DPRINTF("\ns is %p\n", s); MACIO_DPRINTF("io_buffer_index: %x\n", s->io_buffer_index); @@ -294,10 +267,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) nsector = (io->len + 0x1ff) >> 9; remainder = io->len & 0x1ff; + /* Calculate current offset */ + offset = (int64_t)(s->lba << 11) + s->io_buffer_index; + MACIO_DPRINTF("nsector: %d remainder: %x\n", nsector, remainder); MACIO_DPRINTF("sector: %"PRIx64" %zx\n", sector_num, io->iov.size / 512); - pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_atapi_transfer_cb, io); + pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io); return; done: @@ -315,6 +291,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) IDEState *s = idebus_active_if(&m->bus); int64_t sector_num; int nsector, remainder; + int64_t offset; MACIO_DPRINTF("pmac_ide_transfer_cb\n"); @@ -349,6 +326,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) /* Calculate number of sectors */ sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9); + offset = (ide_get_sector(s) << 9) + s->io_buffer_index; nsector = (io->len + 0x1ff) >> 9; remainder = io->len & 0x1ff; @@ -359,7 +337,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) switch (s->dma_cmd) { case IDE_DMA_READ: - pmac_dma_read(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io); + pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io); break; case IDE_DMA_WRITE: pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io); From ac58fe7b2c67a9be142beacd4c6ee51f3264d90f Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Thu, 4 Jun 2015 22:59:35 +0100 Subject: [PATCH 10/12] macio: switch pmac_dma_write() over to new offset/len implementation In particular, this fixes a bug whereby chains of overlapping head/tail chains would incorrectly write over each other's remainder cache. This is the access pattern used by OS X/Darwin and fixes an issue with a corrupt Darwin installation in my local tests. While we are here, rename the DBDMA_io struct property remainder to head_remainder for clarification. Signed-off-by: Mark Cave-Ayland Reviewed-by: John Snow Message-id: 1433455177-21243-3-git-send-email-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/macio.c | 122 +++++++++++++++++-------------------- include/hw/ppc/mac_dbdma.h | 3 +- 2 files changed, 58 insertions(+), 67 deletions(-) diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 52ee4ac6fa..85e315fa09 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -86,7 +86,7 @@ static void pmac_dma_read(BlockBackend *blk, MACIO_DPRINTF("--- DMA unaligned head: sector %" PRId64 ", " "discarding %zu bytes\n", sector_num, head_bytes); - qemu_iovec_add(&io->iov, &io->remainder, head_bytes); + qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes); bytes += offset & (align - 1); offset = offset & ~(align - 1); @@ -100,7 +100,7 @@ static void pmac_dma_read(BlockBackend *blk, MACIO_DPRINTF("--- DMA unaligned tail: sector %" PRId64 ", " "discarding bytes %zu\n", sector_num, tail_bytes); - qemu_iovec_add(&io->iov, &io->remainder, align - tail_bytes); + qemu_iovec_add(&io->iov, &io->tail_remainder, align - tail_bytes); bytes = ROUND_UP(bytes, align); } @@ -117,7 +117,7 @@ static void pmac_dma_read(BlockBackend *blk, } static void pmac_dma_write(BlockBackend *blk, - int64_t sector_num, int nb_sectors, + int64_t offset, int bytes, void (*cb)(void *opaque, int ret), void *opaque) { DBDMA_io *io = opaque; @@ -125,53 +125,20 @@ static void pmac_dma_write(BlockBackend *blk, IDEState *s = idebus_active_if(&m->bus); dma_addr_t dma_addr, dma_len; void *mem; - int nsector, remainder; - int extra = 0; + int64_t sector_num; + int nsector; + uint64_t align = BDRV_SECTOR_SIZE; + size_t head_bytes, tail_bytes; + bool unaligned_head = false, unaligned_tail = false; qemu_iovec_destroy(&io->iov); qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1); - if (io->remainder_len > 0) { - /* Return remainder of request */ - int transfer = MIN(io->remainder_len, io->len); - - MACIO_DPRINTF("--- processing write remainder %x\n", transfer); - cpu_physical_memory_read(io->addr, - &io->remainder + (0x200 - transfer), - transfer); - - io->remainder_len -= transfer; - io->len -= transfer; - io->addr += transfer; - - s->io_buffer_index += transfer; - s->io_buffer_size -= transfer; - - if (io->remainder_len != 0) { - /* Still waiting for remainder */ - return; - } - - MACIO_DPRINTF("--> prepending bounce buffer with size 0x200\n"); - - /* Sector transfer complete - prepend to request */ - qemu_iovec_add(&io->iov, &io->remainder, 0x200); - extra = 1; - } - - if (s->drive_kind == IDE_CD) { - sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9); - } else { - sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9); - } - + sector_num = (offset >> 9); nsector = (io->len >> 9); - remainder = io->len - (nsector << 9); - MACIO_DPRINTF("--- DMA write transfer - addr: %" HWADDR_PRIx " len: %x\n", - io->addr, io->len); - MACIO_DPRINTF("xxx remainder: %x\n", remainder); - MACIO_DPRINTF("xxx sector_num: %"PRIx64" nsector: %x\n", + MACIO_DPRINTF("--- DMA write transfer (0x%" HWADDR_PRIx ",0x%x): " + "sector_num: %" PRId64 ", nsector: %d\n", io->addr, io->len, sector_num, nsector); dma_addr = io->addr; @@ -179,36 +146,59 @@ static void pmac_dma_write(BlockBackend *blk, mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len, DMA_DIRECTION_TO_DEVICE); - if (!remainder) { - MACIO_DPRINTF("--- DMA write aligned - addr: %" HWADDR_PRIx - " len: %x\n", io->addr, io->len); + if (offset & (align - 1)) { + head_bytes = offset & (align - 1); + sector_num = ((offset & ~(align - 1)) >> 9); + + MACIO_DPRINTF("--- DMA unaligned head: pre-reading head sector %" + PRId64 "\n", sector_num); + + blk_pread(s->blk, (sector_num << 9), &io->head_remainder, align); + + qemu_iovec_add(&io->iov, &io->head_remainder, head_bytes); qemu_iovec_add(&io->iov, mem, io->len); - } else { - /* Write up to last complete sector */ - MACIO_DPRINTF("--- DMA write unaligned - addr: %" HWADDR_PRIx - " len: %x\n", io->addr, (nsector << 9)); - qemu_iovec_add(&io->iov, mem, (nsector << 9)); - MACIO_DPRINTF("--- DMA write read - bounce addr: %p " - "remainder_len: %x\n", &io->remainder, remainder); - cpu_physical_memory_read(io->addr + (nsector << 9), &io->remainder, - remainder); + bytes += offset & (align - 1); + offset = offset & ~(align - 1); - io->remainder_len = 0x200 - remainder; - - MACIO_DPRINTF("xxx remainder_len: %x\n", io->remainder_len); + unaligned_head = true; } - s->io_buffer_size -= ((nsector + extra) << 9); - s->io_buffer_index += ((nsector + extra) << 9); + if ((offset + bytes) & (align - 1)) { + tail_bytes = (offset + bytes) & (align - 1); + sector_num = (((offset + bytes) & ~(align - 1)) >> 9); + + MACIO_DPRINTF("--- DMA unaligned tail: pre-reading tail sector %" + PRId64 "\n", sector_num); + + blk_pread(s->blk, (sector_num << 9), &io->tail_remainder, align); + + if (!unaligned_head) { + qemu_iovec_add(&io->iov, mem, io->len); + } + + qemu_iovec_add(&io->iov, &io->tail_remainder + tail_bytes, + align - tail_bytes); + + bytes = ROUND_UP(bytes, align); + + unaligned_tail = true; + } + + if (!unaligned_head && !unaligned_tail) { + qemu_iovec_add(&io->iov, mem, io->len); + } + + s->io_buffer_size -= io->len; + s->io_buffer_index += io->len; io->len = 0; - MACIO_DPRINTF("--- Block write transfer - sector_num: %"PRIx64" " - "nsector: %x\n", sector_num, nsector + extra); + MACIO_DPRINTF("--- Block write transfer - sector_num: %" PRIx64 " " + "nsector: %x\n", (offset >> 9), (bytes >> 9)); - m->aiocb = blk_aio_writev(blk, sector_num, &io->iov, nsector + extra, cb, - io); + m->aiocb = blk_aio_writev(blk, (offset >> 9), &io->iov, (bytes >> 9), + cb, io); } static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) @@ -340,7 +330,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) pmac_dma_read(s->blk, offset, io->len, pmac_ide_transfer_cb, io); break; case IDE_DMA_WRITE: - pmac_dma_write(s->blk, sector_num, nsector, pmac_ide_transfer_cb, io); + pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io); break; case IDE_DMA_TRIM: MACIO_DPRINTF("TRIM command issued!"); diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h index c5803279da..7f247fa4f1 100644 --- a/include/hw/ppc/mac_dbdma.h +++ b/include/hw/ppc/mac_dbdma.h @@ -40,7 +40,8 @@ struct DBDMA_io { /* DMA is in progress, don't start another one */ bool processing; /* unaligned last sector of a request */ - uint8_t remainder[0x200]; + uint8_t head_remainder[0x200]; + uint8_t tail_remainder[0x200]; int remainder_len; QEMUIOVector iov; }; From b01d44cd0623dec66e583d6cd2438451443261df Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Thu, 4 Jun 2015 22:59:36 +0100 Subject: [PATCH 11/12] macio: update comment/constants to reflect the new code With the offset/len functions taking care of all of the alignment mapping in isolation from the DMA tranasaction, many comments are now unnecessary. Remove these and tidy up a few constants at the same time. Signed-off-by: Mark Cave-Ayland Reviewed-by: John Snow Message-id: 1433455177-21243-4-git-send-email-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/macio.c | 46 +++++++++++++--------------------------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/hw/ide/macio.c b/hw/ide/macio.c index 85e315fa09..d353bd254c 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -51,6 +51,13 @@ static const int debug_macio = 0; #define MACIO_PAGE_SIZE 4096 +/* + * Unaligned DMA read/write access functions required for OS X/Darwin which + * don't perform DMA transactions on sector boundaries. These functions are + * modelled on bdrv_co_do_preadv()/bdrv_co_do_pwritev() and so should be + * easy to remove if the unaligned block APIs are ever exposed. + */ + static void pmac_dma_read(BlockBackend *blk, int64_t offset, unsigned int bytes, void (*cb)(void *opaque, int ret), void *opaque) @@ -206,20 +213,12 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) DBDMA_io *io = opaque; MACIOIDEState *m = io->opaque; IDEState *s = idebus_active_if(&m->bus); - int64_t sector_num; - int nsector, remainder; int64_t offset; - MACIO_DPRINTF("\ns is %p\n", s); - MACIO_DPRINTF("io_buffer_index: %x\n", s->io_buffer_index); - MACIO_DPRINTF("io_buffer_size: %x packet_transfer_size: %x\n", - s->io_buffer_size, s->packet_transfer_size); - MACIO_DPRINTF("lba: %x\n", s->lba); - MACIO_DPRINTF("io_addr: %" HWADDR_PRIx " io_len: %x\n", io->addr, - io->len); + MACIO_DPRINTF("pmac_ide_atapi_transfer_cb\n"); if (ret < 0) { - MACIO_DPRINTF("THERE WAS AN ERROR! %d\n", ret); + MACIO_DPRINTF("DMA error: %d\n", ret); ide_atapi_io_error(s, ret); goto done; } @@ -233,6 +232,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) } if (s->io_buffer_size <= 0) { + MACIO_DPRINTF("End of IDE transfer\n"); ide_atapi_cmd_ok(s); m->dma_active = false; goto done; @@ -252,22 +252,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret) goto done; } - /* Calculate number of sectors */ - sector_num = (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9); - nsector = (io->len + 0x1ff) >> 9; - remainder = io->len & 0x1ff; - /* Calculate current offset */ offset = (int64_t)(s->lba << 11) + s->io_buffer_index; - MACIO_DPRINTF("nsector: %d remainder: %x\n", nsector, remainder); - MACIO_DPRINTF("sector: %"PRIx64" %zx\n", sector_num, io->iov.size / 512); - pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io); return; done: - MACIO_DPRINTF("done DMA\n\n"); block_acct_done(blk_get_stats(s->blk), &s->acct); io->dma_end(opaque); @@ -279,14 +270,12 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) DBDMA_io *io = opaque; MACIOIDEState *m = io->opaque; IDEState *s = idebus_active_if(&m->bus); - int64_t sector_num; - int nsector, remainder; int64_t offset; MACIO_DPRINTF("pmac_ide_transfer_cb\n"); if (ret < 0) { - MACIO_DPRINTF("DMA error\n"); + MACIO_DPRINTF("DMA error: %d\n", ret); m->aiocb = NULL; ide_dma_error(s); io->remainder_len = 0; @@ -302,7 +291,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) } if (s->io_buffer_size <= 0) { - MACIO_DPRINTF("end of transfer\n"); + MACIO_DPRINTF("End of IDE transfer\n"); s->status = READY_STAT | SEEK_STAT; ide_set_irq(s->bus); m->dma_active = false; @@ -315,15 +304,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) } /* Calculate number of sectors */ - sector_num = ide_get_sector(s) + (s->io_buffer_index >> 9); offset = (ide_get_sector(s) << 9) + s->io_buffer_index; - nsector = (io->len + 0x1ff) >> 9; - remainder = io->len & 0x1ff; - - s->nsector -= nsector; - - MACIO_DPRINTF("nsector: %d remainder: %x\n", nsector, remainder); - MACIO_DPRINTF("sector: %"PRIx64" %x\n", sector_num, nsector); switch (s->dma_cmd) { case IDE_DMA_READ: @@ -333,7 +314,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io); break; case IDE_DMA_TRIM: - MACIO_DPRINTF("TRIM command issued!"); break; } @@ -537,7 +517,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s, if (s->drive_kind == IDE_CD) { s->io_buffer_size = s->packet_transfer_size; } else { - s->io_buffer_size = s->nsector * 0x200; + s->io_buffer_size = s->nsector * BDRV_SECTOR_SIZE; } MACIO_DPRINTF("\n\n------------ IDE transfer\n"); From 0ba98885a0e965a17df214ab12b819ef630d8a14 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Thu, 4 Jun 2015 22:59:37 +0100 Subject: [PATCH 12/12] macio: remove remainder_len DBDMA_io property Since the block alignment code is now effectively independent of the DMA implementation, this variable is no longer required and can be removed. Signed-off-by: Mark Cave-Ayland Reviewed-by: John Snow Message-id: 1433455177-21243-5-git-send-email-mark.cave-ayland@ilande.co.uk Signed-off-by: John Snow --- hw/ide/macio.c | 13 ------------- include/hw/ppc/mac_dbdma.h | 1 - 2 files changed, 14 deletions(-) diff --git a/hw/ide/macio.c b/hw/ide/macio.c index d353bd254c..dd52d50732 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -278,7 +278,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret) MACIO_DPRINTF("DMA error: %d\n", ret); m->aiocb = NULL; ide_dma_error(s); - io->remainder_len = 0; goto done; } @@ -509,9 +508,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s, BlockCompletionFunc *cb) { MACIOIDEState *m = container_of(dma, MACIOIDEState, dma); - DBDMAState *dbdma = m->dbdma; - DBDMA_io *io; - int i; s->io_buffer_index = 0; if (s->drive_kind == IDE_CD) { @@ -526,15 +522,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s, MACIO_DPRINTF("lba: %x size: %x\n", s->lba, s->io_buffer_size); MACIO_DPRINTF("-------------------------\n"); - for (i = 0; i < DBDMA_CHANNELS; i++) { - io = &dbdma->channels[i].io; - - if (io->opaque == m) { - io->remainder_len = 0; - } - } - - MACIO_DPRINTF("\n"); m->dma_active = true; DBDMA_kick(m->dbdma); } diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h index 7f247fa4f1..c6870212e9 100644 --- a/include/hw/ppc/mac_dbdma.h +++ b/include/hw/ppc/mac_dbdma.h @@ -42,7 +42,6 @@ struct DBDMA_io { /* unaligned last sector of a request */ uint8_t head_remainder[0x200]; uint8_t tail_remainder[0x200]; - int remainder_len; QEMUIOVector iov; };