On Mon, Oct 12, 2015 at 02:27:24PM +0200, Peter Lieven wrote:
> this patch adds a new aio readv compatible function which copies
> all data through a bounce buffer. The benefit is that these requests
> can be flagged as canceled to avoid guest memory corruption when
> a canceled request is completed by the backend at a later stage.
>
> If an IDE protocol wants to use this function it has to pipe
> all read requests through ide_readv_cancelable and it may then
> enable requests_cancelable in the IDEState.
>
> If this state is enable we can avoid the blocking blk_drain_all
> in case of a BMDMA reset.
>
> Currently only read operations are cancelable thus we can only
> use this logic for read-only devices.
Naming is confusing here. Requests are already "cancelable" using
bdv_aio_cancel().
Please use a different name, for example "orphan" requests. These are
requests that QEMU still knows about but the guest believes are
complete. Or maybe "IDEBufferedRequest" since data is transferred
through a bounce buffer.
> Signed-off-by: Peter Lieven <p...@kamp.de>
> ---
> hw/ide/core.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hw/ide/internal.h | 16 ++++++++++++++++
> hw/ide/pci.c | 42 ++++++++++++++++++++++++++++--------------
> 3 files changed, 98 insertions(+), 14 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 317406d..24547ce 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -561,6 +561,59 @@ static bool ide_sect_range_ok(IDEState *s,
> return true;
> }
>
> +static void ide_readv_cancelable_cb(void *opaque, int ret)
> +{
> + IDECancelableRequest *req = opaque;
> + if (!req->canceled) {
> + if (!ret) {
> + qemu_iovec_from_buf(req->org_qiov, 0, req->buf,
> req->org_qiov->size);
> + }
> + req->org_cb(req->org_opaque, ret);
> + }
> + QLIST_REMOVE(req, list);
> + qemu_vfree(req->buf);
> + qemu_iovec_destroy(&req->qiov);
> + g_free(req);
> +}
> +
> +#define MAX_CANCELABLE_REQS 16
> +
> +BlockAIOCB *ide_readv_cancelable(IDEState *s, int64_t sector_num,
> + QEMUIOVector *iov, int nb_sectors,
> + BlockCompletionFunc *cb, void *opaque)
> +{
> + BlockAIOCB *aioreq;
> + IDECancelableRequest *req;
> + int c = 0;
> +
> + QLIST_FOREACH(req, &s->cancelable_requests, list) {
> + c++;
> + }
> + if (c > MAX_CANCELABLE_REQS) {
> + return NULL;
> + }
A BH is probably needed here to schedule an cb(-EIO) call since this
function isn't supposed to return NULL if it's a direct replacement for
blk_aio_readv().
> +
> + req = g_new0(IDECancelableRequest, 1);
> + qemu_iovec_init(&req->qiov, 1);
It saves a g_new() call if you add a struct iovec field to
IDECancelableRequest and use qemu_iovec_init_external() instead of
qemu_iovec_init().
The qemu_iovec_destroy() calls must be dropped when an external struct
iovec is used.
The qemu_iovec_init_external() call must be moved after the
qemu_blockalign() and struct iovec setup below.
> + req->buf = qemu_blockalign(blk_bs(s->blk), iov->size);
> + qemu_iovec_add(&req->qiov, req->buf, iov->size);
> + req->org_qiov = iov;
> + req->org_cb = cb;
> + req->org_opaque = opaque;
> +
> + aioreq = blk_aio_readv(s->blk, sector_num, &req->qiov, nb_sectors,
> + ide_readv_cancelable_cb, req);
> + if (aioreq == NULL) {
> + qemu_vfree(req->buf);
> + qemu_iovec_destroy(&req->qiov);
> + g_free(req);
> + } else {
> + QLIST_INSERT_HEAD(&s->cancelable_requests, req, list);
> + }
> +
> + return aioreq;
> +}
> +
> static void ide_sector_read(IDEState *s);
>
> static void ide_sector_read_cb(void *opaque, int ret)
> @@ -805,6 +858,7 @@ void ide_start_dma(IDEState *s, BlockCompletionFunc *cb)
> s->bus->retry_unit = s->unit;
> s->bus->retry_sector_num = ide_get_sector(s);
> s->bus->retry_nsector = s->nsector;
> + s->bus->s = s;
How is 's' different from 'unit' and 'retry_unit'?
The logic for switching between units is already a little tricky since
the guest can write to the hardware registers while requests are
in-flight.
Please don't duplicate "active unit" state, that increases the risk of
inconsistencies.
Can you use idebus_active_if() to get an equivalent IDEState pointer
without storing s?
> if (s->bus->dma->ops->start_dma) {
> s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
> }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 05e93ff..ad188c2 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -343,6 +343,16 @@ enum ide_dma_cmd {
> #define ide_cmd_is_read(s) \
> ((s)->dma_cmd == IDE_DMA_READ)
>
> +typedef struct IDECancelableRequest {
> + QLIST_ENTRY(IDECancelableRequest) list;
> + QEMUIOVector qiov;
> + uint8_t *buf;
> + QEMUIOVector *org_qiov;
> + BlockCompletionFunc *org_cb;
> + void *org_opaque;
Please don't shorten names, original_* is clearer than org_*.
> + bool canceled;
> +} IDECancelableRequest;
> +
> /* NOTE: IDEState represents in fact one drive */
> struct IDEState {
> IDEBus *bus;
> @@ -396,6 +406,8 @@ struct IDEState {
> BlockAIOCB *pio_aiocb;
> struct iovec iov;
> QEMUIOVector qiov;
> + QLIST_HEAD(, IDECancelableRequest) cancelable_requests;
> + bool requests_cancelable;
> /* ATA DMA state */
> int32_t io_buffer_offset;
> int32_t io_buffer_size;
> @@ -468,6 +480,7 @@ struct IDEBus {
> uint8_t retry_unit;
> int64_t retry_sector_num;
> uint32_t retry_nsector;
> + IDEState *s;
> };
>
> #define TYPE_IDE_DEVICE "ide-device"
> @@ -572,6 +585,9 @@ void ide_set_inactive(IDEState *s, bool more);
> BlockAIOCB *ide_issue_trim(BlockBackend *blk,
> int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
> BlockCompletionFunc *cb, void *opaque);
> +BlockAIOCB *ide_readv_cancelable(IDEState *s, int64_t sector_num,
> + QEMUIOVector *iov, int nb_sectors,
> + BlockCompletionFunc *cb, void *opaque);
>
> /* hw/ide/atapi.c */
> void ide_atapi_cmd(IDEState *s);
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index d31ff88..5587183 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -240,21 +240,35 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
> /* Ignore writes to SSBM if it keeps the old value */
> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
> if (!(val & BM_CMD_START)) {
> - /*
> - * We can't cancel Scatter Gather DMA in the middle of the
> - * operation or a partial (not full) DMA transfer would reach
> - * the storage so we wait for completion instead (we beahve
> - * like if the DMA was completed by the time the guest trying
> - * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
> - * set).
> - *
> - * In the future we'll be able to safely cancel the I/O if the
> - * whole DMA operation will be submitted to disk with a single
> - * aio operation with preadv/pwritev.
> - */
> if (bm->bus->dma->aiocb) {
> - blk_drain_all();
> - assert(bm->bus->dma->aiocb == NULL);
> + if (bm->bus->s && bm->bus->s->requests_cancelable) {
> + /*
> + * If the used IDE protocol supports request cancelation
> we
> + * can flag requests as canceled here and disable DMA.
> + * The IDE protocol used MUST use ide_readv_cancelable
> for all
> + * read operations and then subsequently can enable this
> code
> + * path. Currently this is only supported for read-only
> + * devices.
> + */
> + IDECancelableRequest *req;
> + QLIST_FOREACH(req, &bm->bus->s->cancelable_requests,
> list) {
> + if (!req->canceled) {
> + req->org_cb(req->org_opaque, -ECANCELED);
> + }
> + req->canceled = true;
> + }
> + } else {
> + /*
> + * We can't cancel Scatter Gather DMA in the middle of
> the
> + * operation or a partial (not full) DMA transfer would
> reach
> + * the storage so we wait for completion instead (we
> beahve
> + * like if the DMA was completed by the time the guest
> trying
> + * to cancel dma with bmdma_cmd_writeb with BM_CMD_START
> not
> + * set).
> + */
> + blk_drain_all();
> + assert(bm->bus->dma->aiocb == NULL);
This assertion applies is both branches of the if statement, it could be
moved after the if statement.
> + }
> }
> bm->status &= ~BM_STATUS_DMAING;
> } else {
> --
> 1.9.1
>
