The following changes since commit 171199f56f5f9bdf1e5d670d09ef1351d8f01bae:

Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20200619-3' into staging (2020-06-22 14:45:25 +0100)

https://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 7838c67f22a81fcf669785cd6c0876438422071a:

block/nvme: support nested aio_poll() (2020-06-23 15:46:08 +0100)

Daniele Buono (4):

coroutine: support SafeStack in ucontext backend

coroutine: add check for SafeStack in sigaltstack

configure: add flags to support SafeStack

check-block: enable iotests with SafeStack

Stefan Hajnoczi (8):

minikconf: explicitly set encoding to UTF-8

block/nvme: poll queues without q->lock

block/nvme: drop tautologous assertion

block/nvme: don't access CQE after moving cq.head

block/nvme: switch to a NVMeRequest freelist

block/nvme: clarify that free_req_queue is protected by q->lock

block/nvme: keep BDRVNVMeState pointer in NVMeQueuePair

block/nvme: support nested aio_poll()

configure | 73 ++++++++++++

include/qemu/coroutine_int.h | 5 +

block/nvme.c | 220 +++++++++++++++++++++++++----------

util/coroutine-sigaltstack.c | 4 +

util/coroutine-ucontext.c | 28 +++++

block/trace-events | 2 +-

scripts/minikconf.py | 6 +-

tests/check-block.sh | 12 +-

8 files changed, 284 insertions(+), 66 deletions(-)

2.26.2

Passing around both BDRVNVMeState and NVMeQueuePair is unwieldy. Reduce

the number of function arguments by keeping the BDRVNVMeState pointer in

NVMeQueuePair. This will come in handly when a BH is introduced in a

later patch and only one argument can be passed to it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Sergio Lopez <slp@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-id: 20200617132201.1832152-7-stefanha@redhat.com

block/nvme.c | 70 ++++++++++++++++++++++++++++------------------------

1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c

--- a/block/nvme.c

+++ b/block/nvme.c

@@ -XXX,XX +XXX,XX @@

*/

#define NVME_NUM_REQS (NVME_QUEUE_SIZE - 1)

+typedef struct BDRVNVMeState BDRVNVMeState;

+

typedef struct {

int32_t head, tail;

uint8_t *queue;

@@ -XXX,XX +XXX,XX @@ typedef struct {

typedef struct {

QemuMutex lock;

+ /* Read from I/O code path, initialized under BQL */

+ BDRVNVMeState *s;

+ int index;

+

/* Fields protected by BQL */

- int index;

uint8_t *prp_list_pages;

/* Fields protected by @lock */

@@ -XXX,XX +XXX,XX @@ typedef volatile struct {

QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);

-typedef struct {

+struct BDRVNVMeState {

AioContext *aio_context;

QEMUVFIOState *vfio;

NVMeRegs *regs;

@@ -XXX,XX +XXX,XX @@ typedef struct {

/* PCI address (required for nvme_refresh_filename()) */

char *device;

-} BDRVNVMeState;

+};

#define NVME_BLOCK_OPT_DEVICE "device"

#define NVME_BLOCK_OPT_NAMESPACE "namespace"

@@ -XXX,XX +XXX,XX @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,

-static void nvme_free_queue_pair(BlockDriverState *bs, NVMeQueuePair *q)

+static void nvme_free_queue_pair(NVMeQueuePair *q)

qemu_vfree(q->prp_list_pages);

qemu_vfree(q->sq.queue);

@@ -XXX,XX +XXX,XX @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,

uint64_t prp_list_iova;

qemu_mutex_init(&q->lock);

+ q->s = s;

q->index = idx;

qemu_co_queue_init(&q->free_req_queue);

q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);

@@ -XXX,XX +XXX,XX @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,

return q;

fail:

- nvme_free_queue_pair(bs, q);

+ nvme_free_queue_pair(q);

return NULL;

}

/* With q->lock */

-static void nvme_kick(BDRVNVMeState *s, NVMeQueuePair *q)

+static void nvme_kick(NVMeQueuePair *q)

{

+ BDRVNVMeState *s = q->s;

if (s->plugged || !q->need_kick) {

return;

}

@@ -XXX,XX +XXX,XX @@ static void nvme_put_free_req_locked(NVMeQueuePair *q, NVMeRequest *req)

}

/* With q->lock */

-static void nvme_wake_free_req_locked(BDRVNVMeState *s, NVMeQueuePair *q)

+static void nvme_wake_free_req_locked(NVMeQueuePair *q)

{

if (!qemu_co_queue_empty(&q->free_req_queue)) {

- replay_bh_schedule_oneshot_event(s->aio_context,

+ replay_bh_schedule_oneshot_event(q->s->aio_context,

nvme_free_req_queue_cb, q);

}

}

/* Insert a request in the freelist and wake waiters */

-static void nvme_put_free_req_and_wake(BDRVNVMeState *s, NVMeQueuePair *q,

- NVMeRequest *req)

+static void nvme_put_free_req_and_wake(NVMeQueuePair *q, NVMeRequest *req)

{

qemu_mutex_lock(&q->lock);

nvme_put_free_req_locked(q, req);

- nvme_wake_free_req_locked(s, q);

+ nvme_wake_free_req_locked(q);

qemu_mutex_unlock(&q->lock);

}

@@ -XXX,XX +XXX,XX @@ static inline int nvme_translate_error(const NvmeCqe *c)

}

/* With q->lock */

-static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)

+static bool nvme_process_completion(NVMeQueuePair *q)

{

+ BDRVNVMeState *s = q->s;

bool progress = false;

NVMeRequest *preq;

NVMeRequest req;

@@ -XXX,XX +XXX,XX @@ static bool nvme_process_completion(BDRVNVMeState *s, NVMeQueuePair *q)

/* Notify the device so it can post more completions. */

smp_mb_release();

*q->cq.doorbell = cpu_to_le32(q->cq.head);

- nvme_wake_free_req_locked(s, q);

+ nvme_wake_free_req_locked(q);

}

q->busy = false;

return progress;

@@ -XXX,XX +XXX,XX @@ static void nvme_trace_command(const NvmeCmd *cmd)

}

}

-static void nvme_submit_command(BDRVNVMeState *s, NVMeQueuePair *q,

- NVMeRequest *req,

+static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req,

NvmeCmd *cmd, BlockCompletionFunc cb,

void *opaque)

{

@@ -XXX,XX +XXX,XX @@ static void nvme_submit_command(BDRVNVMeState *s, NVMeQueuePair *q,

req->opaque = opaque;

cmd->cid = cpu_to_le32(req->cid);

- trace_nvme_submit_command(s, q->index, req->cid);

+ trace_nvme_submit_command(q->s, q->index, req->cid);

nvme_trace_command(cmd);

qemu_mutex_lock(&q->lock);

memcpy((uint8_t *)q->sq.queue +

q->sq.tail * NVME_SQ_ENTRY_BYTES, cmd, sizeof(*cmd));

q->sq.tail = (q->sq.tail + 1) % NVME_QUEUE_SIZE;

q->need_kick++;

- nvme_kick(s, q);

- nvme_process_completion(s, q);

+ nvme_kick(q);

+ nvme_process_completion(q);

qemu_mutex_unlock(&q->lock);

}

@@ -XXX,XX +XXX,XX @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,

NvmeCmd *cmd)

{

NVMeRequest *req;

- BDRVNVMeState *s = bs->opaque;

int ret = -EINPROGRESS;

req = nvme_get_free_req(q);

if (!req) {

return -EBUSY;

}

- nvme_submit_command(s, q, req, cmd, nvme_cmd_sync_cb, &ret);

+ nvme_submit_command(q, req, cmd, nvme_cmd_sync_cb, &ret);

BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);

return ret;

@@ -XXX,XX +XXX,XX @@ static bool nvme_poll_queues(BDRVNVMeState *s)

}

qemu_mutex_lock(&q->lock);

- while (nvme_process_completion(s, q)) {

+ while (nvme_process_completion(q)) {

/* Keep polling */

progress = true;

}

@@ -XXX,XX +XXX,XX @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)

};

if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {

error_setg(errp, "Failed to create io queue [%d]", n);

- nvme_free_queue_pair(bs, q);

+ nvme_free_queue_pair(q);

return false;

}

cmd = (NvmeCmd) {

@@ -XXX,XX +XXX,XX @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)

};

if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {

error_setg(errp, "Failed to create io queue [%d]", n);

- nvme_free_queue_pair(bs, q);

+ nvme_free_queue_pair(q);

return false;

}

s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);

@@ -XXX,XX +XXX,XX @@ static void nvme_close(BlockDriverState *bs)

BDRVNVMeState *s = bs->opaque;

for (i = 0; i < s->nr_queues; ++i) {

- nvme_free_queue_pair(bs, s->queues[i]);

+ nvme_free_queue_pair(s->queues[i]);

}

g_free(s->queues);

aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,

r = nvme_cmd_map_qiov(bs, &cmd, req, qiov);

qemu_co_mutex_unlock(&s->dma_map_lock);

if (r) {

- nvme_put_free_req_and_wake(s, ioq, req);

+ nvme_put_free_req_and_wake(ioq, req);

return r;

}

- nvme_submit_command(s, ioq, req, &cmd, nvme_rw_cb, &data);

+ nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);

data.co = qemu_coroutine_self();

while (data.ret == -EINPROGRESS) {

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int nvme_co_flush(BlockDriverState *bs)

assert(s->nr_queues > 1);

req = nvme_get_free_req(ioq);

assert(req);

- nvme_submit_command(s, ioq, req, &cmd, nvme_rw_cb, &data);

+ nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);

data.co = qemu_coroutine_self();

if (data.ret == -EINPROGRESS) {

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,

req = nvme_get_free_req(ioq);

assert(req);

- nvme_submit_command(s, ioq, req, &cmd, nvme_rw_cb, &data);

+ nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);

data.co = qemu_coroutine_self();

while (data.ret == -EINPROGRESS) {

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,

qemu_co_mutex_unlock(&s->dma_map_lock);

if (ret) {

- nvme_put_free_req_and_wake(s, ioq, req);

+ nvme_put_free_req_and_wake(ioq, req);

goto out;

}

trace_nvme_dsm(s, offset, bytes);

- nvme_submit_command(s, ioq, req, &cmd, nvme_rw_cb, &data);

+ nvme_submit_command(ioq, req, &cmd, nvme_rw_cb, &data);

data.co = qemu_coroutine_self();

while (data.ret == -EINPROGRESS) {

@@ -XXX,XX +XXX,XX @@ static void nvme_aio_unplug(BlockDriverState *bs)

for (i = 1; i < s->nr_queues; i++) {

NVMeQueuePair *q = s->queues[i];

qemu_mutex_lock(&q->lock);

- nvme_kick(s, q);

- nvme_process_completion(s, q);

+ nvme_kick(q);

+ nvme_process_completion(q);

qemu_mutex_unlock(&q->lock);

}

}

2.26.2

QEMU block drivers are supposed to support aio_poll() from I/O

completion callback functions. This means completion processing must be

re-entrant.

The standard approach is to schedule a BH during completion processing

and cancel it at the end of processing. If aio_poll() is invoked by a

callback function then the BH will run. The BH continues the suspended

completion processing.

All of this means that request A's cb() can synchronously wait for

request B to complete. Previously the nvme block driver would hang

because it didn't process completions from nested aio_poll().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Sergio Lopez <slp@redhat.com>

Message-id: 20200617132201.1832152-8-stefanha@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

---

block/nvme.c | 67 ++++++++++++++++++++++++++++++++++++++++------

block/trace-events | 2 +-

2 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c

--- a/block/nvme.c

+++ b/block/nvme.c

@@ -XXX,XX +XXX,XX @@ typedef struct {

int cq_phase;

int free_req_head;

NVMeRequest reqs[NVME_NUM_REQS];

- bool busy;

int need_kick;

int inflight;

+

+ /* Thread-safe, no lock necessary */

+ QEMUBH *completion_bh;

} NVMeQueuePair;

/* Memory mapped registers */

@@ -XXX,XX +XXX,XX @@ struct BDRVNVMeState {

#define NVME_BLOCK_OPT_DEVICE "device"

#define NVME_BLOCK_OPT_NAMESPACE "namespace"

+static void nvme_process_completion_bh(void *opaque);

+

static QemuOptsList runtime_opts = {

.name = "nvme",

.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),

@@ -XXX,XX +XXX,XX @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,

static void nvme_free_queue_pair(NVMeQueuePair *q)

+ if (q->completion_bh) {

+ qemu_bh_delete(q->completion_bh);

+ }

qemu_vfree(q->prp_list_pages);

qemu_vfree(q->sq.queue);

qemu_vfree(q->cq.queue);

@@ -XXX,XX +XXX,XX @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,

q->index = idx;

qemu_co_queue_init(&q->free_req_queue);

q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);

+ q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),

+ nvme_process_completion_bh, q);

r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,

s->page_size * NVME_NUM_REQS,

false, &prp_list_iova);

@@ -XXX,XX +XXX,XX @@ static bool nvme_process_completion(NVMeQueuePair *q)

NvmeCqe *c;

trace_nvme_process_completion(s, q->index, q->inflight);

- if (q->busy || s->plugged) {

- trace_nvme_process_completion_queue_busy(s, q->index);

+ if (s->plugged) {

+ trace_nvme_process_completion_queue_plugged(s, q->index);

return false;

}

- q->busy = true;

+

+ /*

+ * Support re-entrancy when a request cb() function invokes aio_poll().

+ * Pending completions must be visible to aio_poll() so that a cb()

+ * function can wait for the completion of another request.

+ *

+ * The aio_poll() loop will execute our BH and we'll resume completion

+ * processing there.

+ */

+ qemu_bh_schedule(q->completion_bh);

+

assert(q->inflight >= 0);

while (q->inflight) {

int ret;

@@ -XXX,XX +XXX,XX @@ static bool nvme_process_completion(NVMeQueuePair *q)

assert(req.cb);

nvme_put_free_req_locked(q, preq);

preq->cb = preq->opaque = NULL;

- qemu_mutex_unlock(&q->lock);

- req.cb(req.opaque, ret);

- qemu_mutex_lock(&q->lock);

q->inflight--;

+ qemu_mutex_unlock(&q->lock);

+ req.cb(req.opaque, ret);

+ qemu_mutex_lock(&q->lock);

progress = true;

}

if (progress) {

@@ -XXX,XX +XXX,XX @@ static bool nvme_process_completion(NVMeQueuePair *q)

*q->cq.doorbell = cpu_to_le32(q->cq.head);

nvme_wake_free_req_locked(q);

}

- q->busy = false;

+

+ qemu_bh_cancel(q->completion_bh);

+

return progress;

}

+static void nvme_process_completion_bh(void *opaque)

+{

+ NVMeQueuePair *q = opaque;

+

+ /*

+ * We're being invoked because a nvme_process_completion() cb() function

+ * called aio_poll(). The callback may be waiting for further completions

+ * so notify the device that it has space to fill in more completions now.

+ */

+ smp_mb_release();

+ *q->cq.doorbell = cpu_to_le32(q->cq.head);

+ nvme_wake_free_req_locked(q);

+

+ nvme_process_completion(q);

+}

+

static void nvme_trace_command(const NvmeCmd *cmd)

{

int i;

@@ -XXX,XX +XXX,XX @@ static void nvme_detach_aio_context(BlockDriverState *bs)

{

BDRVNVMeState *s = bs->opaque;

+ for (int i = 0; i < s->nr_queues; i++) {

+ NVMeQueuePair *q = s->queues[i];

+

+ qemu_bh_delete(q->completion_bh);

+ q->completion_bh = NULL;

+ }

+

aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,

false, NULL, NULL);

}

@@ -XXX,XX +XXX,XX @@ static void nvme_attach_aio_context(BlockDriverState *bs,

s->aio_context = new_context;

aio_set_event_notifier(new_context, &s->irq_notifier,

false, nvme_handle_event, nvme_poll_cb);

+

+ for (int i = 0; i < s->nr_queues; i++) {

+ NVMeQueuePair *q = s->queues[i];

+

+ q->completion_bh =

+ aio_bh_new(new_context, nvme_process_completion_bh, q);

+ }

}

static void nvme_aio_plug(BlockDriverState *bs)

diff --git a/block/trace-events b/block/trace-events

index XXXXXXX..XXXXXXX 100644

--- a/block/trace-events

+++ b/block/trace-events

@@ -XXX,XX +XXX,XX @@ nvme_kick(void *s, int queue) "s %p queue %d"

nvme_dma_flush_queue_wait(void *s) "s %p"

nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"

nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"

-nvme_process_completion_queue_busy(void *s, int index) "s %p queue %d"

+nvme_process_completion_queue_plugged(void *s, int index) "s %p queue %d"

nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"

nvme_submit_command(void *s, int index, int cid) "s %p queue %d cid %d"

nvme_submit_command_raw(int c0, int c1, int c2, int c3, int c4, int c5, int c6, int c7) "%02x %02x %02x %02x %02x %02x %02x %02x"

2.26.2

From: Daniele Buono <dbuono@linux.vnet.ibm.com>

LLVM's SafeStack instrumentation does not yet support programs that make

use of the APIs in ucontext.h

With the current implementation of coroutine-ucontext, the resulting

binary is incorrect, with different coroutines sharing the same unsafe

stack and producing undefined behavior at runtime.

This fix allocates an additional unsafe stack area for each coroutine,

and sets the new unsafe stack pointer before calling swapcontext() in

qemu_coroutine_new.

This is the only place where the pointer needs to be manually updated,

since sigsetjmp/siglongjmp are already instrumented by LLVM to properly

support SafeStack.

The additional stack is then freed in qemu_coroutine_delete.

Signed-off-by: Daniele Buono <dbuono@linux.vnet.ibm.com>

Message-id: 20200529205122.714-2-dbuono@linux.vnet.ibm.com

include/qemu/coroutine_int.h | 5 +++++

util/coroutine-ucontext.c | 28 ++++++++++++++++++++++++++++

2 files changed, 33 insertions(+)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h

--- a/include/qemu/coroutine_int.h

+++ b/include/qemu/coroutine_int.h

@@ -XXX,XX +XXX,XX @@

#include "qemu/queue.h"

#include "qemu/coroutine.h"

+#ifdef CONFIG_SAFESTACK

+/* Pointer to the unsafe stack, defined by the compiler */

+extern __thread void *__safestack_unsafe_stack_ptr;

+#endif

+

#define COROUTINE_STACK_SIZE (1 << 20)

typedef enum {

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c

--- a/util/coroutine-ucontext.c

+++ b/util/coroutine-ucontext.c

@@ -XXX,XX +XXX,XX @@ typedef struct {

Coroutine base;

void *stack;

size_t stack_size;

+#ifdef CONFIG_SAFESTACK

+ /* Need an unsafe stack for each coroutine */

+ void *unsafe_stack;

+ size_t unsafe_stack_size;

+#endif

sigjmp_buf env;

void *tsan_co_fiber;

@@ -XXX,XX +XXX,XX @@ Coroutine *qemu_coroutine_new(void)

co = g_malloc0(sizeof(*co));

co->stack_size = COROUTINE_STACK_SIZE;

co->stack = qemu_alloc_stack(&co->stack_size);

+#ifdef CONFIG_SAFESTACK

+ co->unsafe_stack_size = COROUTINE_STACK_SIZE;

+ co->unsafe_stack = qemu_alloc_stack(&co->unsafe_stack_size);

+#endif

co->base.entry_arg = &old_env; /* stash away our jmp_buf */

uc.uc_link = &old_uc;

@@ -XXX,XX +XXX,XX @@ Coroutine *qemu_coroutine_new(void)

COROUTINE_YIELD,

&fake_stack_save,

co->stack, co->stack_size, co->tsan_co_fiber);

+

+#ifdef CONFIG_SAFESTACK

+ /*

+ * Before we swap the context, set the new unsafe stack

+ * The unsafe stack grows just like the normal stack, so start from

+ * the last usable location of the memory area.

+ * NOTE: we don't have to re-set the usp afterwards because we are

+ * coming back to this context through a siglongjmp.

+ * The compiler already wrapped the corresponding sigsetjmp call with

+ * code that saves the usp on the (safe) stack before the call, and

+ * restores it right after (which is where we return with siglongjmp).

+ */

+ void *usp = co->unsafe_stack + co->unsafe_stack_size;

+ __safestack_unsafe_stack_ptr = usp;

+#endif

+

swapcontext(&old_uc, &uc);

2.26.2

Existing users access free_req_queue under q->lock. Document this.

Reviewed-by: Sergio Lopez <slp@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-id: 20200617132201.1832152-6-stefanha@redhat.com

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

block/nvme.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c

--- a/block/nvme.c

+++ b/block/nvme.c

@@ -XXX,XX +XXX,XX @@ typedef struct {