Re: Host folder sharing via USB issue
> On Jul 14, 2021, at 6:35 AM, Vladimir Sementsov-Ogievskiy > wrote: > > 14.07.2021 00:04, Programmingkid wrote: >> Hi I have noticed that host folder sharing via USB has recently stopped >> working. After doing some git bisecting I found this as the patch that seems >> to be the issue: >> 25f78d9e2de528473d52acfcf7acdfb64e3453d4 is the first bad commit >> commit 25f78d9e2de528473d52acfcf7acdfb64e3453d4 >> Author: Vladimir Sementsov-Ogievskiy >> Date: Thu Jun 10 15:05:34 2021 +0300 >> block: move supports_backing check to bdrv_set_file_or_backing_noperm() >> Move supports_backing check of bdrv_reopen_parse_backing to called >> (through bdrv_set_backing_noperm()) bdrv_set_file_or_backing_noperm() >> function. The check applies to general case, so it's appropriate for >> bdrv_set_file_or_backing_noperm(). >> We have to declare backing support for two test drivers, otherwise >> new >> check fails. >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> >> Message-Id: <20210610120537.196183-7-vsement...@virtuozzo.com> >> Signed-off-by: Kevin Wolf >> block.c | 29 +++-- >> tests/unit/test-bdrv-drain.c | 1 + >> tests/unit/test-bdrv-graph-mod.c | 1 + >> 3 files changed, 17 insertions(+), 14 deletions(-) >> To reproduce this issue run this command: >> qemu-system-i386 -usb -device usb-storage,drive=fat16 -drive >> file=fat:rw:fat-type=16:"",id=fat16,format=raw,if=none >> Results: >> Unexpected error in bdrv_set_file_or_backing_noperm() at ../block.c:3159: >> qemu-system-i386: -drive file=fat:rw:fat-type=16:> path>,id=fat16,format=raw,if=none: Driver 'vvfat' of node '#block057' does >> not support backing files >> Abort trap: 6 >> Expected results: >> QEMU start running normally. >> Please let me know if you need more information. >> Thank you. > > Hi! > > Look at bt: > > #0 0x7fd34f6939d5 in raise () at /lib64/libc.so.6 > #1 0x7fd34f67c8a4 in abort () at /lib64/libc.so.6 > #2 0x55e446d967aa in error_handle_fatal (errp=0x55e447652680 > , err=0x55e448d17a20) at ../util/error.c:40 > #3 0x55e446d968da in error_setv >(errp=0x55e447652680 , src=0x55e446f8755b "../block.c", > line=3158, func=0x55e446f89c20 <__func__.64> > "bdrv_set_file_or_backing_noperm", err_class=ERROR_CLASS_GENERIC_ERROR, > fmt=0x55e446f88458 "Driver '%s' of node '%s' does not support backing files", > ap=0x7ffc31aba090, suffix=0x0) at ../util/error.c:73 > #4 0x55e446d96ab8 in error_setg_internal >(errp=0x55e447652680 , src=0x55e446f8755b "../block.c", > line=3158, func=0x55e446f89c20 <__func__.64> > "bdrv_set_file_or_backing_noperm", fmt=0x55e446f88458 "Driver '%s' of node > '%s' does not support backing files") at ../util/error.c:97 > #5 0x55e446c411cf in bdrv_set_file_or_backing_noperm > (parent_bs=0x55e448ceebe0, child_bs=0x55e448d21e40, is_backing=true, > tran=0x55e448d16c20, errp=0x55e447652680 ) at ../block.c:3158 > #6 0x55e446c41377 in bdrv_set_backing_noperm (bs=0x55e448ceebe0, > backing_hd=0x55e448d21e40, tran=0x55e448d16c20, errp=0x55e447652680 > ) at ../block.c:3218 > #7 0x55e446c413ae in bdrv_set_backing_hd (bs=0x55e448ceebe0, > backing_hd=0x55e448d21e40, errp=0x55e447652680 ) at > ../block.c:3227 > #8 0x55e446c1bd37 in enable_write_target (bs=0x55e448ceebe0, > errp=0x7ffc31aba360) at ../block/vvfat.c:3191 > #9 0x55e446c16fe8 in vvfat_open (bs=0x55e448ceebe0, > options=0x55e448cf4330, flags=155650, errp=0x7ffc31aba360) at > ../block/vvfat.c:1236 > #10 0x55e446c3df37 in bdrv_open_driver (bs=0x55e448ceebe0, > drv=0x55e4475e9760 , node_name=0x0, options=0x55e448cf4330, > open_flags=155650, errp=0x7ffc31aba470) at ../block.c:1550 > #11 0x55e446c3e8ee in bdrv_open_common (bs=0x55e448ceebe0, file=0x0, > options=0x55e448cf4330, errp=0x7ffc31aba470) at ../block.c:1827 > #12 0x55e446c427b6 in bdrv_open_inherit >(filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, > options=0x55e448cf4330, flags=40962, parent=0x55e448ce75a0, > child_class=0x55e4475099c0 , child_role=19, errp=0x7ffc31aba670) >at ../block.c:3747 > #13 0x55e446c419f5 in bdrv_open_child_bs >(filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", > options=0x55e448cec9f0, bdref_key=0x55e446f884d0 "file", > parent=0x55e448ce75a0, child_class=0x55e4475099c0 , > child_role=19, allow_none=true, errp=0x7ffc31aba670) at ../block.c:3387 > #14 0x55e446c42568 in bdrv_open_inherit >(filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, > options=0x55e448cec9f0, flags=8194, parent=0x0, child_class=0x0, > child_role=0, errp=0x55e447652688 ) at ../block.c:3694 > #15 0x55e446c42cf6 in bdrv_open (filename=0x55e448ce4300 > "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448ce4f00, flags=0, > errp=0x55e447652688 ) at ../block.c:3840 > #16 0x55e446c5fcaf in blk_new_open (filename=0x55e448ce4300 >
[PATCH v5 5/5] block/nbd: check that received handle is valid
If we don't have active request, that waiting for this handle to be
received, we should report an error.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 8313937f0f..ca9fcb732e 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -58,6 +58,7 @@ typedef struct {
Coroutine *coroutine;
uint64_t offset;/* original offset of the request */
bool receiving; /* sleeping in the yield in nbd_receive_replies */
+bool reply_possible;/* reply header not yet received */
} NBDClientRequest;
typedef enum NBDClientState {
@@ -415,16 +416,11 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState
*s, uint64_t handle)
return 0;
}
ind2 = HANDLE_TO_INDEX(s, s->reply.handle);
-if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].coroutine) {
-/*
- * We only check that ind2 request exists. But don't check is it
now
- * waiting for the reply header or not. We can't just check
- * s->requests[ind2].receiving: ind2 request may wait in trying to
- * lock receive_mutex. So that's a TODO.
- */
+if (ind2 >= MAX_NBD_REQUESTS || !s->requests[ind2].reply_possible) {
nbd_channel_error(s, -EINVAL);
return -EINVAL;
}
+s->requests[ind2].reply_possible = false;
nbd_recv_coroutine_wake(>requests[ind2]);
}
}
@@ -467,6 +463,7 @@ static int nbd_co_send_request(BlockDriverState *bs,
s->requests[i].coroutine = qemu_coroutine_self();
s->requests[i].offset = request->from;
s->requests[i].receiving = false;
+s->requests[i].reply_possible = true;
request->handle = INDEX_TO_HANDLE(s, i);
--
2.29.2
Re: [PATCH v5 4/5] block/nbd: drop connection_co
[add Roman} 14.07.2021 19:59, Vladimir Sementsov-Ogievskiy wrote: OK, that's a big rewrite of the logic. Pre-patch we have an always running coroutine - connection_co. It does reply receiving and reconnecting. And it leads to a lot of difficult and unobvious code around drained sections and context switch. We also abuse bs->in_flight counter which is increased for connection_co and temporary decreased in points where we want to allow drained section to begin. One of these place is in another file: in nbd_read_eof() in nbd/client.c. We also cancel reconnect and requests waiting for reconnect on drained begin which is not correct. And this patch fixes that. Let's finally drop this always running coroutine and go another way: Hmm here is missed something like: go another way: do both reconnect and receiving in request coroutines. The detailed list of changes below (in the sequence of diff hunks). 1. receiving coroutines are woken directly from nbd_channel_error, when we change s->state 2. nbd_co_establish_connection_cancel(): we don't have drain_begin now, and in nbd_teardown_connection() all requests should already be finished (and reconnect is done from request). So nbd_co_establish_connection_cancel() is called from nbd_cancel_in_flight() (to cancel the request that is doing -- Best regards, Vladimir
[PATCH v5 2/5] block/nbd: move nbd_recv_coroutines_wake_all() up
We are going to use it in nbd_channel_error(), so move it up. Note,
that we are going also refactor and rename
nbd_recv_coroutines_wake_all() in future anyway, so keeping it where it
is and making forward declaration doesn't make real sense.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index d88f4b954c..32e3826ba2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -127,6 +127,20 @@ static bool nbd_client_connected(BDRVNBDState *s)
return qatomic_load_acquire(>state) == NBD_CLIENT_CONNECTED;
}
+static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
+{
+int i;
+
+for (i = 0; i < MAX_NBD_REQUESTS; i++) {
+NBDClientRequest *req = >requests[i];
+
+if (req->coroutine && req->receiving) {
+req->receiving = false;
+aio_co_wake(req->coroutine);
+}
+}
+}
+
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
if (nbd_client_connected(s)) {
@@ -143,20 +157,6 @@ static void nbd_channel_error(BDRVNBDState *s, int ret)
}
}
-static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
-{
-int i;
-
-for (i = 0; i < MAX_NBD_REQUESTS; i++) {
-NBDClientRequest *req = >requests[i];
-
-if (req->coroutine && req->receiving) {
-req->receiving = false;
-aio_co_wake(req->coroutine);
-}
-}
-}
-
static void reconnect_delay_timer_del(BDRVNBDState *s)
{
if (s->reconnect_delay_timer) {
--
2.29.2
[PATCH v5 1/5] block/nbd: nbd_channel_error() shutdown channel unconditionally
Don't rely on connection being totally broken in case of -EIO. More
safe and correct just shutdown the channel anyway, as we change the
state and going to reconnect.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index f6ff1c4fb4..d88f4b954c 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -129,15 +129,16 @@ static bool nbd_client_connected(BDRVNBDState *s)
static void nbd_channel_error(BDRVNBDState *s, int ret)
{
+if (nbd_client_connected(s)) {
+qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
+
if (ret == -EIO) {
if (nbd_client_connected(s)) {
s->state = s->reconnect_delay ? NBD_CLIENT_CONNECTING_WAIT :
NBD_CLIENT_CONNECTING_NOWAIT;
}
} else {
-if (nbd_client_connected(s)) {
-qio_channel_shutdown(s->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
-}
s->state = NBD_CLIENT_QUIT;
}
}
--
2.29.2
[PATCH v5 4/5] block/nbd: drop connection_co
OK, that's a big rewrite of the logic. Pre-patch we have an always running coroutine - connection_co. It does reply receiving and reconnecting. And it leads to a lot of difficult and unobvious code around drained sections and context switch. We also abuse bs->in_flight counter which is increased for connection_co and temporary decreased in points where we want to allow drained section to begin. One of these place is in another file: in nbd_read_eof() in nbd/client.c. We also cancel reconnect and requests waiting for reconnect on drained begin which is not correct. Let's finally drop this always running coroutine and go another way: 1. receiving coroutines are woken directly from nbd_channel_error, when we change s->state 2. nbd_co_establish_connection_cancel(): we don't have drain_begin now, and in nbd_teardown_connection() all requests should already be finished (and reconnect is done from request). So nbd_co_establish_connection_cancel() is called from nbd_cancel_in_flight() (to cancel the request that is doing nbd_co_establish_connection()) and from reconnect_delay_timer_cb() (previously we didn't need it, as reconnect delay only should cancel active requests not the reconnection itself. But now reconnection itself is done in the separate thread (we now call nbd_client_connection_enable_retry() in nbd_open()), and we need to cancel the requests that waits in nbd_co_establish_connection() now). 2. We do receive headers in request coroutine. But we also should dispatch replies for another pending requests. So, nbd_connection_entry() is turned into nbd_receive_replies(), which does reply dispatching until it receive another request headers, and returns when it receive the requested header. 3. All old staff around drained sections and context switch is dropped. In details: - we don't need to move connection_co to new aio context, as we don't have connection_co anymore - we don't have a fake "request" of connection_co (extra increasing in_flight), so don't care with it in drain_begin/end - we don't stop reconnection during drained section anymore. This means that drain_begin may wait for a long time (up to reconnect_delay). But that's an improvement and more correct behavior see below[*] 4. In nbd_teardown_connection() we don't have to wait for connection_co, as it is dropped. And cleanup for s->ioc and nbd_yank is moved here from removed connection_co. 5. In nbd_co_do_establish_connection() we now should handle NBD_CLIENT_CONNECTING_NOWAIT: if new request comes when we are in NBD_CLIENT_CONNECTING_NOWAIT, it still should call nbd_co_establish_connection() (who knows, maybe connection already established by thread in background). But we shouldn't wait: if nbd_co_establish_connection() can't return new channel immediately the request should fail (we are in NBD_CLIENT_CONNECTING_NOWAIT state). 6. nbd_reconnect_attempt() is simplified: it's now easier to wait for other requests in the caller, so here we just assert that fact. Also delay time is now initialized here: we can easily detect first attempt and start a timer. 7. nbd_co_reconnect_loop() is dropped, we don't need it. Reconnect retries are fully handle by thread (nbd/client-connection.c), delay timer we initialize in nbd_reconnect_attempt(), we don't have to bother with s->drained and friends. nbd_reconnect_attempt() now called from nbd_co_send_request(). 8. nbd_connection_entry is dropped: reconnect is now handled by nbd_co_send_request(), receiving reply is now handled by nbd_receive_replies(): all handled from request coroutines. 9. So, welcome new nbd_receive_replies() called from request coroutine, that receives reply header instead of nbd_connection_entry(). Like with sending requests, only one coroutine may receive in a moment. So we introduce receive_mutex, which is locked around nbd_receive_reply(). It also protects some related fields. Still, full audit of thread-safety in nbd driver is a separate task. New function waits for a reply with specified handle being received and works rather simple: Under mutex: - if current handle is 0, do receive by hand. If another handle received - switch to other request coroutine, release mutex and yield. Otherwise return success - if current handle == requested handle, we are done - otherwise, release mutex and yield 10: in nbd_co_send_request() we now do nbd_reconnect_attempt() if needed. Also waiting in free_sema queue we now wait for one of two conditions: - connectED, in_flight < MAX_NBD_REQUESTS (so we can start new one) - connectING, in_flight == 0, so we can call nbd_reconnect_attempt() And this logic is protected by s->send_mutex Also, on failure we don't have to care of removed s->connection_co 11. nbd_co_do_receive_one_chunk(): now instead of yield() and wait for
[PATCH v5 3/5] block/nbd: refactor nbd_recv_coroutines_wake_all()
Split out nbd_recv_coroutine_wake(), as it will be used in separate.
Also add a possibility to wake only first found sleeping coroutine.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/nbd.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 32e3826ba2..3b91954b1f 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -127,16 +127,24 @@ static bool nbd_client_connected(BDRVNBDState *s)
return qatomic_load_acquire(>state) == NBD_CLIENT_CONNECTED;
}
-static void nbd_recv_coroutines_wake_all(BDRVNBDState *s)
+static bool nbd_recv_coroutine_wake(NBDClientRequest *req)
+{
+if (req->receiving) {
+req->receiving = false;
+aio_co_wake(req->coroutine);
+return true;
+}
+
+return false;
+}
+
+static void nbd_recv_coroutines_wake_all(BDRVNBDState *s, bool only_one)
{
int i;
for (i = 0; i < MAX_NBD_REQUESTS; i++) {
-NBDClientRequest *req = >requests[i];
-
-if (req->coroutine && req->receiving) {
-req->receiving = false;
-aio_co_wake(req->coroutine);
+if (nbd_recv_coroutine_wake(>requests[i]) && only_one) {
+return;
}
}
}
@@ -415,7 +423,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState
*s)
while (s->in_flight > 0) {
qemu_co_mutex_unlock(>send_mutex);
-nbd_recv_coroutines_wake_all(s);
+nbd_recv_coroutines_wake_all(s, false);
s->wait_in_flight = true;
qemu_coroutine_yield();
s->wait_in_flight = false;
@@ -558,7 +566,7 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
}
qemu_co_queue_restart_all(>free_sema);
-nbd_recv_coroutines_wake_all(s);
+nbd_recv_coroutines_wake_all(s, false);
bdrv_dec_in_flight(s->bs);
s->connection_co = NULL;
--
2.29.2
[PATCH v5 0/5] block/nbd: drop connection_co
Hi all! That's finally a new version of [PATCH v3 33/33] block/nbd: drop connection_co (v4 was sent and merged without that last patch of v3) I tried to do some things in separate patches, but still "block/nbd: drop connection_co" is big and complicated. Vladimir Sementsov-Ogievskiy (5): block/nbd: nbd_channel_error() shutdown channel unconditionally block/nbd: move nbd_recv_coroutines_wake_all() up block/nbd: refactor nbd_recv_coroutines_wake_all() block/nbd: drop connection_co block/nbd: check that received handle is valid block/nbd.c | 412 +++ nbd/client.c | 2 - 2 files changed, 121 insertions(+), 293 deletions(-) -- 2.29.2
Re: [PATCH 0/3] Fix active mirror dead-lock
Am 02.07.2021 um 23:16 hat Vladimir Sementsov-Ogievskiy geschrieben: > Hi all! > > We've faced a dead-lock in active mirror in our Rhev-8.4 based Qemu > build. And it's reproducible on master too. Thanks, applied to the block branch. Kevin
Re: [PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test
On Wed, Jul 14, 2021 at 08:01:25AM +0200, Klaus Jensen wrote:
From: Klaus Jensen
Add a regression test for mmio read on big-endian hosts.
Signed-off-by: Klaus Jensen
---
tests/qtest/nvme-test.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
index 47e757d7e2af..f8bafb5d70fb 100644
--- a/tests/qtest/nvme-test.c
+++ b/tests/qtest/nvme-test.c
@@ -67,6 +67,30 @@ static void nvmetest_oob_cmb_test(void *obj, void *data,
QGuestAllocator *alloc)
g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=, 0x44332211);
}
+static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator
*alloc)
+{
+QNvme *nvme = obj;
+QPCIDevice *pdev = >dev;
+QPCIBar bar;
+uint32_t cap_lo, cap_hi;
+uint64_t cap;
+
+qpci_device_enable(pdev);
+bar = qpci_iomap(pdev, 0, NULL);
+
+cap_lo = qpci_io_readl(pdev, bar, 0x0);
+g_assert_cmpint(NVME_CAP_MQES(cap_lo), ==, 0x7ff);
+
+cap_hi = qpci_io_readl(pdev, bar, 0x4);
+g_assert_cmpint(NVME_CAP_MPSMAX((uint64_t)cap_hi << 32), ==, 0x4);
+
+cap = qpci_io_readq(pdev, bar, 0x0);
+g_assert_cmpint(NVME_CAP_MQES(cap), ==, 0x7ff);
+g_assert_cmpint(NVME_CAP_MPSMAX(cap), ==, 0x4);
+
+qpci_iounmap(pdev, bar);
+}
+
static void nvmetest_pmr_reg_test(void *obj, void *data, QGuestAllocator *alloc)
{
QNvme *nvme = obj;
@@ -142,6 +166,8 @@ static void nvme_register_nodes(void)
&(QOSGraphTestOptions) {
.edge.extra_device_opts = "pmrdev=pmr0"
});
+
+qos_add_test("reg-read", "nvme", nvmetest_reg_read_test, NULL);
}
libqos_init(nvme_register_nodes);
--
2.32.0
Reviewed-by: Gollu Appalanaidu
Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers
On Wed, Jul 14, 2021 at 08:01:22AM +0200, Klaus Jensen wrote:
From: Klaus Jensen
Add the NvmeBarRegs enum and use these instead of explicit register
offsets.
Signed-off-by: Klaus Jensen
---
include/block/nvme.h | 29 -
hw/nvme/ctrl.c | 44 ++--
2 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..77aae0117494 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_tcc;
uint8_t rsvd24[4];
uint32_tcsts;
-uint32_tnssrc;
+uint32_tnssr;
uint32_taqa;
uint64_tasq;
uint64_tacq;
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
uint8_t css[484];
} NvmeBar;
+enum NvmeBarRegs {
+NVME_REG_CAP = offsetof(NvmeBar, cap),
+NVME_REG_VS = offsetof(NvmeBar, vs),
+NVME_REG_INTMS = offsetof(NvmeBar, intms),
+NVME_REG_INTMC = offsetof(NvmeBar, intmc),
+NVME_REG_CC = offsetof(NvmeBar, cc),
+NVME_REG_CSTS= offsetof(NvmeBar, csts),
+NVME_REG_NSSR= offsetof(NvmeBar, nssr),
+NVME_REG_AQA = offsetof(NvmeBar, aqa),
+NVME_REG_ASQ = offsetof(NvmeBar, asq),
+NVME_REG_ACQ = offsetof(NvmeBar, acq),
+NVME_REG_CMBLOC = offsetof(NvmeBar, cmbloc),
+NVME_REG_CMBSZ = offsetof(NvmeBar, cmbsz),
+NVME_REG_BPINFO = offsetof(NvmeBar, bpinfo),
+NVME_REG_BPRSEL = offsetof(NvmeBar, bprsel),
+NVME_REG_BPMBL = offsetof(NvmeBar, bpmbl),
+NVME_REG_CMBMSC = offsetof(NvmeBar, cmbmsc),
+NVME_REG_CMBSTS = offsetof(NvmeBar, cmbsts),
+NVME_REG_PMRCAP = offsetof(NvmeBar, pmrcap),
+NVME_REG_PMRCTL = offsetof(NvmeBar, pmrctl),
+NVME_REG_PMRSTS = offsetof(NvmeBar, pmrsts),
+NVME_REG_PMREBS = offsetof(NvmeBar, pmrebs),
+NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
+NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
+NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
+};
+
enum NvmeCapShift {
CAP_MQES_SHIFT = 0,
CAP_CQR_SHIFT = 16,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 28299c6f3764..8c305315f41c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
}
switch (offset) {
-case 0xc: /* INTMS */
+case NVME_REG_INTMS:
if (unlikely(msix_enabled(&(n->parent_obj {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask set"
@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
trace_pci_nvme_mmio_intm_set(data & 0x, n->bar.intmc);
nvme_irq_check(n);
break;
-case 0x10: /* INTMC */
+case NVME_REG_INTMC:
if (unlikely(msix_enabled(&(n->parent_obj {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask clr"
@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
trace_pci_nvme_mmio_intm_clr(data & 0x, n->bar.intmc);
nvme_irq_check(n);
break;
-case 0x14: /* CC */
+case NVME_REG_CC:
trace_pci_nvme_mmio_cfg(data & 0x);
/* Windows first sends data, then sends enable bit */
if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
n->bar.cc = data;
}
break;
-case 0x1c: /* CSTS */
+case NVME_REG_CSTS:
if (data & (1 << 4)) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
"attempted to W1C CSTS.NSSRO"
@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
" of controller status");
}
break;
-case 0x20: /* NSSR */
+case NVME_REG_NSSR:
if (data == 0x4e564d65) {
trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
} else {
@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
break;
-case 0x24: /* AQA */
+case NVME_REG_AQA:
n->bar.aqa = data & 0x;
trace_pci_nvme_mmio_aqattr(data & 0x);
break;
-case 0x28: /* ASQ */
+case NVME_REG_ASQ:
n->bar.asq = size == 8 ? data :
(n->bar.asq & ~0xULL) | (data & 0x);
trace_pci_nvme_mmio_asqaddr(data);
break;
-case 0x2c: /* ASQ hi */
+case NVME_REG_ASQ + 4:
n->bar.asq = (n->bar.asq & 0x) | (data << 32);
trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
break;
-case 0x30: /* ACQ */
+case NVME_REG_ACQ:
Re: [PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
On Wed, Jul 14, 2021 at 08:01:21AM +0200, Klaus Jensen wrote:
From: Klaus Jensen
The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
make up the 64 bit logical PMRMSC register.
Make it so.
Signed-off-by: Klaus Jensen
---
include/block/nvme.h | 31 ---
hw/nvme/ctrl.c | 9 +
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 527105fafc0b..84053b68b987 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -26,7 +26,8 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_tpmrsts;
uint32_tpmrebs;
uint32_tpmrswtp;
-uint64_tpmrmsc;
+uint32_tpmrmscl;
+uint32_tpmrmscu;
uint8_t css[484];
} NvmeBar;
@@ -475,25 +476,25 @@ enum NvmePmrswtpMask {
#define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val) \
(pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) << PMRSWTP_PMRSWTV_SHIFT)
-enum NvmePmrmscShift {
-PMRMSC_CMSE_SHIFT = 1,
-PMRMSC_CBA_SHIFT= 12,
+enum NvmePmrmsclShift {
+PMRMSCL_CMSE_SHIFT = 1,
+PMRMSCL_CBA_SHIFT= 12,
};
-enum NvmePmrmscMask {
-PMRMSC_CMSE_MASK = 0x1,
-PMRMSC_CBA_MASK= 0xf,
+enum NvmePmrmsclMask {
+PMRMSCL_CMSE_MASK = 0x1,
+PMRMSCL_CBA_MASK= 0xf,
};
-#define NVME_PMRMSC_CMSE(pmrmsc)\
-((pmrmsc >> PMRMSC_CMSE_SHIFT) & PMRMSC_CMSE_MASK)
-#define NVME_PMRMSC_CBA(pmrmsc) \
-((pmrmsc >> PMRMSC_CBA_SHIFT) & PMRMSC_CBA_MASK)
+#define NVME_PMRMSCL_CMSE(pmrmscl)\
+((pmrmscl >> PMRMSCL_CMSE_SHIFT) & PMRMSCL_CMSE_MASK)
+#define NVME_PMRMSCL_CBA(pmrmscl) \
+((pmrmscl >> PMRMSCL_CBA_SHIFT) & PMRMSCL_CBA_MASK)
-#define NVME_PMRMSC_SET_CMSE(pmrmsc, val) \
-(pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT)
-#define NVME_PMRMSC_SET_CBA(pmrmsc, val) \
-(pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
+#define NVME_PMRMSCL_SET_CMSE(pmrmscl, val) \
+(pmrmscl |= (uint32_t)(val & PMRMSCL_CMSE_MASK) << PMRMSCL_CMSE_SHIFT)
+#define NVME_PMRMSCL_SET_CBA(pmrmscl, val) \
+(pmrmscl |= (uint32_t)(val & PMRMSCL_CBA_MASK) << PMRMSCL_CBA_SHIFT)
enum NvmeSglDescriptorType {
NVME_SGL_DESCR_TYPE_DATA_BLOCK = 0x0,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2f0524e12a36..28299c6f3764 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
-n->bar.pmrmsc = (n->bar.pmrmsc & ~0x) | (data & 0x);
+n->bar.pmrmscl = data & 0x;
n->pmr.cmse = false;
-if (NVME_PMRMSC_CMSE(n->bar.pmrmsc)) {
-hwaddr cba = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
+hwaddr cba = n->bar.pmrmscu |
+(NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
return;
@@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
-n->bar.pmrmsc = (n->bar.pmrmsc & 0x) | (data << 32);
+n->bar.pmrmscu = data & 0x;
return;
default:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
--
2.32.0
Changes LGTM.
Reviewed-by: Gollu Appalanaidu
Re: Host folder sharing via USB issue
On Wed, 14 Jul 2021, Vladimir Sementsov-Ogievskiy wrote: 14.07.2021 00:04, Programmingkid wrote: Hi I have noticed that host folder sharing via USB has recently stopped working. After doing some git bisecting I found this as the patch that seems to be the issue: 25f78d9e2de528473d52acfcf7acdfb64e3453d4 is the first bad commit commit 25f78d9e2de528473d52acfcf7acdfb64e3453d4 Author: Vladimir Sementsov-Ogievskiy Date: Thu Jun 10 15:05:34 2021 +0300 block: move supports_backing check to bdrv_set_file_or_backing_noperm() Move supports_backing check of bdrv_reopen_parse_backing to called (through bdrv_set_backing_noperm()) bdrv_set_file_or_backing_noperm() function. The check applies to general case, so it's appropriate for bdrv_set_file_or_backing_noperm(). We have to declare backing support for two test drivers, otherwise new check fails. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-7-vsement...@virtuozzo.com> Signed-off-by: Kevin Wolf block.c | 29 +++-- tests/unit/test-bdrv-drain.c | 1 + tests/unit/test-bdrv-graph-mod.c | 1 + 3 files changed, 17 insertions(+), 14 deletions(-) To reproduce this issue run this command: qemu-system-i386 -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"",id=fat16,format=raw,if=none Results: Unexpected error in bdrv_set_file_or_backing_noperm() at ../block.c:3159: qemu-system-i386: -drive file=fat:rw:fat-type=16:path>,id=fat16,format=raw,if=none: Driver 'vvfat' of node '#block057' does not support backing files Abort trap: 6 Expected results: QEMU start running normally. Please let me know if you need more information. Thank you. Hi! Look at bt: #0 0x7fd34f6939d5 in raise () at /lib64/libc.so.6 #1 0x7fd34f67c8a4 in abort () at /lib64/libc.so.6 #2 0x55e446d967aa in error_handle_fatal (errp=0x55e447652680 , err=0x55e448d17a20) at ../util/error.c:40 #3 0x55e446d968da in error_setv (errp=0x55e447652680 , src=0x55e446f8755b "../block.c", line=3158, func=0x55e446f89c20 <__func__.64> "bdrv_set_file_or_backing_noperm", err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55e446f88458 "Driver '%s' of node '%s' does not support backing files", ap=0x7ffc31aba090, suffix=0x0) at ../util/error.c:73 #4 0x55e446d96ab8 in error_setg_internal (errp=0x55e447652680 , src=0x55e446f8755b "../block.c", line=3158, func=0x55e446f89c20 <__func__.64> "bdrv_set_file_or_backing_noperm", fmt=0x55e446f88458 "Driver '%s' of node '%s' does not support backing files") at ../util/error.c:97 #5 0x55e446c411cf in bdrv_set_file_or_backing_noperm (parent_bs=0x55e448ceebe0, child_bs=0x55e448d21e40, is_backing=true, tran=0x55e448d16c20, errp=0x55e447652680 ) at ../block.c:3158 #6 0x55e446c41377 in bdrv_set_backing_noperm (bs=0x55e448ceebe0, backing_hd=0x55e448d21e40, tran=0x55e448d16c20, errp=0x55e447652680 ) at ../block.c:3218 #7 0x55e446c413ae in bdrv_set_backing_hd (bs=0x55e448ceebe0, backing_hd=0x55e448d21e40, errp=0x55e447652680 ) at ../block.c:3227 #8 0x55e446c1bd37 in enable_write_target (bs=0x55e448ceebe0, errp=0x7ffc31aba360) at ../block/vvfat.c:3191 #9 0x55e446c16fe8 in vvfat_open (bs=0x55e448ceebe0, options=0x55e448cf4330, flags=155650, errp=0x7ffc31aba360) at ../block/vvfat.c:1236 #10 0x55e446c3df37 in bdrv_open_driver (bs=0x55e448ceebe0, drv=0x55e4475e9760 , node_name=0x0, options=0x55e448cf4330, open_flags=155650, errp=0x7ffc31aba470) at ../block.c:1550 #11 0x55e446c3e8ee in bdrv_open_common (bs=0x55e448ceebe0, file=0x0, options=0x55e448cf4330, errp=0x7ffc31aba470) at ../block.c:1827 #12 0x55e446c427b6 in bdrv_open_inherit (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448cf4330, flags=40962, parent=0x55e448ce75a0, child_class=0x55e4475099c0 , child_role=19, errp=0x7ffc31aba670) at ../block.c:3747 #13 0x55e446c419f5 in bdrv_open_child_bs (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", options=0x55e448cec9f0, bdref_key=0x55e446f884d0 "file", parent=0x55e448ce75a0, child_class=0x55e4475099c0 , child_role=19, allow_none=true, errp=0x7ffc31aba670) at ../block.c:3387 #14 0x55e446c42568 in bdrv_open_inherit (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448cec9f0, flags=8194, parent=0x0, child_class=0x0, child_role=0, errp=0x55e447652688 ) at ../block.c:3694 #15 0x55e446c42cf6 in bdrv_open (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448ce4f00, flags=0, errp=0x55e447652688 ) at ../block.c:3840 #16 0x55e446c5fcaf in blk_new_open (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448ce4f00, flags=0, errp=0x55e447652688 ) at ../block/block-backend.c:435 #17 0x55e446beca1d in blockdev_init (file=0x55e448ce4300 "fat:rw:fat-type=16:/tmp",
Re: Host folder sharing via USB issue
14.07.2021 00:04, Programmingkid wrote: Hi I have noticed that host folder sharing via USB has recently stopped working. After doing some git bisecting I found this as the patch that seems to be the issue: 25f78d9e2de528473d52acfcf7acdfb64e3453d4 is the first bad commit commit 25f78d9e2de528473d52acfcf7acdfb64e3453d4 Author: Vladimir Sementsov-Ogievskiy Date: Thu Jun 10 15:05:34 2021 +0300 block: move supports_backing check to bdrv_set_file_or_backing_noperm() Move supports_backing check of bdrv_reopen_parse_backing to called (through bdrv_set_backing_noperm()) bdrv_set_file_or_backing_noperm() function. The check applies to general case, so it's appropriate for bdrv_set_file_or_backing_noperm(). We have to declare backing support for two test drivers, otherwise new check fails. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-7-vsement...@virtuozzo.com> Signed-off-by: Kevin Wolf block.c | 29 +++-- tests/unit/test-bdrv-drain.c | 1 + tests/unit/test-bdrv-graph-mod.c | 1 + 3 files changed, 17 insertions(+), 14 deletions(-) To reproduce this issue run this command: qemu-system-i386 -usb -device usb-storage,drive=fat16 -drive file=fat:rw:fat-type=16:"",id=fat16,format=raw,if=none Results: Unexpected error in bdrv_set_file_or_backing_noperm() at ../block.c:3159: qemu-system-i386: -drive file=fat:rw:fat-type=16:,id=fat16,format=raw,if=none: Driver 'vvfat' of node '#block057' does not support backing files Abort trap: 6 Expected results: QEMU start running normally. Please let me know if you need more information. Thank you. Hi! Look at bt: #0 0x7fd34f6939d5 in raise () at /lib64/libc.so.6 #1 0x7fd34f67c8a4 in abort () at /lib64/libc.so.6 #2 0x55e446d967aa in error_handle_fatal (errp=0x55e447652680 , err=0x55e448d17a20) at ../util/error.c:40 #3 0x55e446d968da in error_setv (errp=0x55e447652680 , src=0x55e446f8755b "../block.c", line=3158, func=0x55e446f89c20 <__func__.64> "bdrv_set_file_or_backing_noperm", err_class=ERROR_CLASS_GENERIC_ERROR, fmt=0x55e446f88458 "Driver '%s' of node '%s' does not support backing files", ap=0x7ffc31aba090, suffix=0x0) at ../util/error.c:73 #4 0x55e446d96ab8 in error_setg_internal (errp=0x55e447652680 , src=0x55e446f8755b "../block.c", line=3158, func=0x55e446f89c20 <__func__.64> "bdrv_set_file_or_backing_noperm", fmt=0x55e446f88458 "Driver '%s' of node '%s' does not support backing files") at ../util/error.c:97 #5 0x55e446c411cf in bdrv_set_file_or_backing_noperm (parent_bs=0x55e448ceebe0, child_bs=0x55e448d21e40, is_backing=true, tran=0x55e448d16c20, errp=0x55e447652680 ) at ../block.c:3158 #6 0x55e446c41377 in bdrv_set_backing_noperm (bs=0x55e448ceebe0, backing_hd=0x55e448d21e40, tran=0x55e448d16c20, errp=0x55e447652680 ) at ../block.c:3218 #7 0x55e446c413ae in bdrv_set_backing_hd (bs=0x55e448ceebe0, backing_hd=0x55e448d21e40, errp=0x55e447652680 ) at ../block.c:3227 #8 0x55e446c1bd37 in enable_write_target (bs=0x55e448ceebe0, errp=0x7ffc31aba360) at ../block/vvfat.c:3191 #9 0x55e446c16fe8 in vvfat_open (bs=0x55e448ceebe0, options=0x55e448cf4330, flags=155650, errp=0x7ffc31aba360) at ../block/vvfat.c:1236 #10 0x55e446c3df37 in bdrv_open_driver (bs=0x55e448ceebe0, drv=0x55e4475e9760 , node_name=0x0, options=0x55e448cf4330, open_flags=155650, errp=0x7ffc31aba470) at ../block.c:1550 #11 0x55e446c3e8ee in bdrv_open_common (bs=0x55e448ceebe0, file=0x0, options=0x55e448cf4330, errp=0x7ffc31aba470) at ../block.c:1827 #12 0x55e446c427b6 in bdrv_open_inherit (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448cf4330, flags=40962, parent=0x55e448ce75a0, child_class=0x55e4475099c0 , child_role=19, errp=0x7ffc31aba670) at ../block.c:3747 #13 0x55e446c419f5 in bdrv_open_child_bs (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", options=0x55e448cec9f0, bdref_key=0x55e446f884d0 "file", parent=0x55e448ce75a0, child_class=0x55e4475099c0 , child_role=19, allow_none=true, errp=0x7ffc31aba670) at ../block.c:3387 #14 0x55e446c42568 in bdrv_open_inherit (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448cec9f0, flags=8194, parent=0x0, child_class=0x0, child_role=0, errp=0x55e447652688 ) at ../block.c:3694 #15 0x55e446c42cf6 in bdrv_open (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448ce4f00, flags=0, errp=0x55e447652688 ) at ../block.c:3840 #16 0x55e446c5fcaf in blk_new_open (filename=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", reference=0x0, options=0x55e448ce4f00, flags=0, errp=0x55e447652688 ) at ../block/block-backend.c:435 #17 0x55e446beca1d in blockdev_init (file=0x55e448ce4300 "fat:rw:fat-type=16:/tmp", bs_opts=0x55e448ce4f00, errp=0x55e447652688 ) at ../blockdev.c:609 #18
Re: [PATCH v3 2/5] hw/nvme: use symbolic names for registers
On 7/14/21 8:01 AM, Klaus Jensen wrote: > From: Klaus Jensen > > Add the NvmeBarRegs enum and use these instead of explicit register > offsets. > > Signed-off-by: Klaus Jensen > --- > include/block/nvme.h | 29 - > hw/nvme/ctrl.c | 44 ++-- > 2 files changed, 50 insertions(+), 23 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH v3 5/5] tests/qtest/nvme-test: add mmio read test
From: Klaus Jensen
Add a regression test for mmio read on big-endian hosts.
Signed-off-by: Klaus Jensen
---
tests/qtest/nvme-test.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/nvme-test.c b/tests/qtest/nvme-test.c
index 47e757d7e2af..f8bafb5d70fb 100644
--- a/tests/qtest/nvme-test.c
+++ b/tests/qtest/nvme-test.c
@@ -67,6 +67,30 @@ static void nvmetest_oob_cmb_test(void *obj, void *data,
QGuestAllocator *alloc)
g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=,
0x44332211);
}
+static void nvmetest_reg_read_test(void *obj, void *data, QGuestAllocator
*alloc)
+{
+QNvme *nvme = obj;
+QPCIDevice *pdev = >dev;
+QPCIBar bar;
+uint32_t cap_lo, cap_hi;
+uint64_t cap;
+
+qpci_device_enable(pdev);
+bar = qpci_iomap(pdev, 0, NULL);
+
+cap_lo = qpci_io_readl(pdev, bar, 0x0);
+g_assert_cmpint(NVME_CAP_MQES(cap_lo), ==, 0x7ff);
+
+cap_hi = qpci_io_readl(pdev, bar, 0x4);
+g_assert_cmpint(NVME_CAP_MPSMAX((uint64_t)cap_hi << 32), ==, 0x4);
+
+cap = qpci_io_readq(pdev, bar, 0x0);
+g_assert_cmpint(NVME_CAP_MQES(cap), ==, 0x7ff);
+g_assert_cmpint(NVME_CAP_MPSMAX(cap), ==, 0x4);
+
+qpci_iounmap(pdev, bar);
+}
+
static void nvmetest_pmr_reg_test(void *obj, void *data, QGuestAllocator
*alloc)
{
QNvme *nvme = obj;
@@ -142,6 +166,8 @@ static void nvme_register_nodes(void)
&(QOSGraphTestOptions) {
.edge.extra_device_opts = "pmrdev=pmr0"
});
+
+qos_add_test("reg-read", "nvme", nvmetest_reg_read_test, NULL);
}
libqos_init(nvme_register_nodes);
--
2.32.0
[PATCH v3 4/5] hw/nvme: fix mmio read
From: Klaus Jensen
The new PMR test unearthed a long-standing issue with MMIO reads on
big-endian hosts.
Fix this by unconditionally storing all controller registers in little
endian.
Cc: Gollu Appalanaidu
Reported-by: Peter Maydell
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 300 -
1 file changed, 173 insertions(+), 127 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 0449cc4dee9b..6d06ed990f2d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -439,10 +439,12 @@ static uint8_t nvme_sq_empty(NvmeSQueue *sq)
static void nvme_irq_check(NvmeCtrl *n)
{
+uint32_t intms = ldl_le_p(>bar.intms);
+
if (msix_enabled(&(n->parent_obj))) {
return;
}
-if (~n->bar.intms & n->irq_status) {
+if (~intms & n->irq_status) {
pci_irq_assert(>parent_obj);
} else {
pci_irq_deassert(>parent_obj);
@@ -1289,7 +1291,7 @@ static void nvme_post_cqes(void *opaque)
if (ret) {
trace_pci_nvme_err_addr_write(addr);
trace_pci_nvme_err_cfs();
-n->bar.csts = NVME_CSTS_FAILED;
+stl_le_p(>bar.csts, NVME_CSTS_FAILED);
break;
}
QTAILQ_REMOVE(>req_list, req, entry);
@@ -4022,7 +4024,7 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest
*req)
trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
return NVME_INVALID_QID | NVME_DNR;
}
-if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(>bar.cap {
trace_pci_nvme_err_invalid_create_sq_size(qsize);
return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
}
@@ -4208,7 +4210,7 @@ static uint16_t nvme_cmd_effects(NvmeCtrl *n, uint8_t
csi, uint32_t buf_len,
return NVME_INVALID_FIELD | NVME_DNR;
}
-switch (NVME_CC_CSS(n->bar.cc)) {
+switch (NVME_CC_CSS(ldl_le_p(>bar.cc))) {
case NVME_CC_CSS_NVM:
src_iocs = nvme_cse_iocs_nvm;
/* fall through */
@@ -4370,7 +4372,7 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest
*req)
trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
return NVME_INVALID_QID | NVME_DNR;
}
-if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
+if (unlikely(!qsize || qsize > NVME_CAP_MQES(ldq_le_p(>bar.cap {
trace_pci_nvme_err_invalid_create_cq_size(qsize);
return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
}
@@ -5163,17 +5165,19 @@ static void nvme_update_dmrsl(NvmeCtrl *n)
static void nvme_select_iocs_ns(NvmeCtrl *n, NvmeNamespace *ns)
{
+uint32_t cc = ldl_le_p(>bar.cc);
+
ns->iocs = nvme_cse_iocs_none;
switch (ns->csi) {
case NVME_CSI_NVM:
-if (NVME_CC_CSS(n->bar.cc) != NVME_CC_CSS_ADMIN_ONLY) {
+if (NVME_CC_CSS(cc) != NVME_CC_CSS_ADMIN_ONLY) {
ns->iocs = nvme_cse_iocs_nvm;
}
break;
case NVME_CSI_ZONED:
-if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_CSI) {
+if (NVME_CC_CSS(cc) == NVME_CC_CSS_CSI) {
ns->iocs = nvme_cse_iocs_zoned;
-} else if (NVME_CC_CSS(n->bar.cc) == NVME_CC_CSS_NVM) {
+} else if (NVME_CC_CSS(cc) == NVME_CC_CSS_NVM) {
ns->iocs = nvme_cse_iocs_nvm;
}
break;
@@ -5510,7 +5514,7 @@ static void nvme_process_sq(void *opaque)
if (nvme_addr_read(n, addr, (void *), sizeof(cmd))) {
trace_pci_nvme_err_addr_read(addr);
trace_pci_nvme_err_cfs();
-n->bar.csts = NVME_CSTS_FAILED;
+stl_le_p(>bar.csts, NVME_CSTS_FAILED);
break;
}
nvme_inc_sq_head(sq);
@@ -5565,8 +5569,6 @@ static void nvme_ctrl_reset(NvmeCtrl *n)
n->aer_queued = 0;
n->outstanding_aers = 0;
n->qs_created = false;
-
-n->bar.cc = 0;
}
static void nvme_ctrl_shutdown(NvmeCtrl *n)
@@ -5605,7 +5607,12 @@ static void nvme_select_iocs(NvmeCtrl *n)
static int nvme_start_ctrl(NvmeCtrl *n)
{
-uint32_t page_bits = NVME_CC_MPS(n->bar.cc) + 12;
+uint64_t cap = ldq_le_p(>bar.cap);
+uint32_t cc = ldl_le_p(>bar.cc);
+uint32_t aqa = ldl_le_p(>bar.aqa);
+uint64_t asq = ldq_le_p(>bar.asq);
+uint64_t acq = ldq_le_p(>bar.acq);
+uint32_t page_bits = NVME_CC_MPS(cc) + 12;
uint32_t page_size = 1 << page_bits;
if (unlikely(n->cq[0])) {
@@ -5616,73 +5623,72 @@ static int nvme_start_ctrl(NvmeCtrl *n)
trace_pci_nvme_err_startfail_sq();
return -1;
}
-if (unlikely(!n->bar.asq)) {
+if (unlikely(!asq)) {
trace_pci_nvme_err_startfail_nbarasq();
return -1;
}
-if (unlikely(!n->bar.acq)) {
+if (unlikely(!acq)) {
trace_pci_nvme_err_startfail_nbaracq();
return -1;
}
-if (unlikely(n->bar.asq & (page_size - 1))) {
-trace_pci_nvme_err_startfail_asq_misaligned(n->bar.asq);
+if (unlikely(asq & (page_size - 1))) {
+
[PATCH v3 3/5] hw/nvme: fix out-of-bounds reads
From: Klaus Jensen
Peter noticed that mmio access may read into the NvmeParams member in
the NvmeCtrl struct.
Fix the bounds check.
Reported-by: Peter Maydell
Signed-off-by: Klaus Jensen
---
hw/nvme/ctrl.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 8c305315f41c..0449cc4dee9b 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5968,23 +5968,26 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr
addr, unsigned size)
/* should RAZ, fall through for now */
}
-if (addr < sizeof(n->bar)) {
-/*
- * When PMRWBM bit 1 is set then read from
- * from PMRSTS should ensure prior writes
- * made it to persistent media
- */
-if (addr == NVME_REG_PMRSTS &&
-(NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
-memory_region_msync(>pmr.dev->mr, 0, n->pmr.dev->size);
-}
-memcpy(, ptr + addr, size);
-} else {
+if (addr > sizeof(n->bar) - size) {
NVME_GUEST_ERR(pci_nvme_ub_mmiord_invalid_ofs,
"MMIO read beyond last register,"
" offset=0x%"PRIx64", returning 0", addr);
+
+return 0;
}
+/*
+ * When PMRWBM bit 1 is set then read from
+ * from PMRSTS should ensure prior writes
+ * made it to persistent media
+ */
+if (addr == NVME_REG_PMRSTS &&
+(NVME_PMRCAP_PMRWBM(n->bar.pmrcap) & 0x02)) {
+memory_region_msync(>pmr.dev->mr, 0, n->pmr.dev->size);
+}
+
+memcpy(, ptr + addr, size);
+
return val;
}
--
2.32.0
[PATCH v3 2/5] hw/nvme: use symbolic names for registers
From: Klaus Jensen
Add the NvmeBarRegs enum and use these instead of explicit register
offsets.
Signed-off-by: Klaus Jensen
---
include/block/nvme.h | 29 -
hw/nvme/ctrl.c | 44 ++--
2 files changed, 50 insertions(+), 23 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 84053b68b987..77aae0117494 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -9,7 +9,7 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_tcc;
uint8_t rsvd24[4];
uint32_tcsts;
-uint32_tnssrc;
+uint32_tnssr;
uint32_taqa;
uint64_tasq;
uint64_tacq;
@@ -31,6 +31,33 @@ typedef struct QEMU_PACKED NvmeBar {
uint8_t css[484];
} NvmeBar;
+enum NvmeBarRegs {
+NVME_REG_CAP = offsetof(NvmeBar, cap),
+NVME_REG_VS = offsetof(NvmeBar, vs),
+NVME_REG_INTMS = offsetof(NvmeBar, intms),
+NVME_REG_INTMC = offsetof(NvmeBar, intmc),
+NVME_REG_CC = offsetof(NvmeBar, cc),
+NVME_REG_CSTS= offsetof(NvmeBar, csts),
+NVME_REG_NSSR= offsetof(NvmeBar, nssr),
+NVME_REG_AQA = offsetof(NvmeBar, aqa),
+NVME_REG_ASQ = offsetof(NvmeBar, asq),
+NVME_REG_ACQ = offsetof(NvmeBar, acq),
+NVME_REG_CMBLOC = offsetof(NvmeBar, cmbloc),
+NVME_REG_CMBSZ = offsetof(NvmeBar, cmbsz),
+NVME_REG_BPINFO = offsetof(NvmeBar, bpinfo),
+NVME_REG_BPRSEL = offsetof(NvmeBar, bprsel),
+NVME_REG_BPMBL = offsetof(NvmeBar, bpmbl),
+NVME_REG_CMBMSC = offsetof(NvmeBar, cmbmsc),
+NVME_REG_CMBSTS = offsetof(NvmeBar, cmbsts),
+NVME_REG_PMRCAP = offsetof(NvmeBar, pmrcap),
+NVME_REG_PMRCTL = offsetof(NvmeBar, pmrctl),
+NVME_REG_PMRSTS = offsetof(NvmeBar, pmrsts),
+NVME_REG_PMREBS = offsetof(NvmeBar, pmrebs),
+NVME_REG_PMRSWTP = offsetof(NvmeBar, pmrswtp),
+NVME_REG_PMRMSCL = offsetof(NvmeBar, pmrmscl),
+NVME_REG_PMRMSCU = offsetof(NvmeBar, pmrmscu),
+};
+
enum NvmeCapShift {
CAP_MQES_SHIFT = 0,
CAP_CQR_SHIFT = 16,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 28299c6f3764..8c305315f41c 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5740,7 +5740,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
}
switch (offset) {
-case 0xc: /* INTMS */
+case NVME_REG_INTMS:
if (unlikely(msix_enabled(&(n->parent_obj {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask set"
@@ -5752,7 +5752,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
trace_pci_nvme_mmio_intm_set(data & 0x, n->bar.intmc);
nvme_irq_check(n);
break;
-case 0x10: /* INTMC */
+case NVME_REG_INTMC:
if (unlikely(msix_enabled(&(n->parent_obj {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_intmask_with_msix,
"undefined access to interrupt mask clr"
@@ -5764,7 +5764,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
trace_pci_nvme_mmio_intm_clr(data & 0x, n->bar.intmc);
nvme_irq_check(n);
break;
-case 0x14: /* CC */
+case NVME_REG_CC:
trace_pci_nvme_mmio_cfg(data & 0x);
/* Windows first sends data, then sends enable bit */
if (!NVME_CC_EN(data) && !NVME_CC_EN(n->bar.cc) &&
@@ -5798,7 +5798,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
n->bar.cc = data;
}
break;
-case 0x1c: /* CSTS */
+case NVME_REG_CSTS:
if (data & (1 << 4)) {
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_ssreset_w1c_unsupported,
"attempted to W1C CSTS.NSSRO"
@@ -5809,7 +5809,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
" of controller status");
}
break;
-case 0x20: /* NSSR */
+case NVME_REG_NSSR:
if (data == 0x4e564d65) {
trace_pci_nvme_ub_mmiowr_ssreset_unsupported();
} else {
@@ -5817,38 +5817,38 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
break;
-case 0x24: /* AQA */
+case NVME_REG_AQA:
n->bar.aqa = data & 0x;
trace_pci_nvme_mmio_aqattr(data & 0x);
break;
-case 0x28: /* ASQ */
+case NVME_REG_ASQ:
n->bar.asq = size == 8 ? data :
(n->bar.asq & ~0xULL) | (data & 0x);
trace_pci_nvme_mmio_asqaddr(data);
break;
-case 0x2c: /* ASQ hi */
+case NVME_REG_ASQ + 4:
n->bar.asq = (n->bar.asq & 0x) | (data << 32);
trace_pci_nvme_mmio_asqaddr_hi(data, n->bar.asq);
break;
-case 0x30: /* ACQ */
+case NVME_REG_ACQ:
[PATCH v3 1/5] hw/nvme: split pmrmsc register into upper and lower
From: Klaus Jensen
The specification uses a set of 32 bit PMRMSCL and PMRMSCU registers to
make up the 64 bit logical PMRMSC register.
Make it so.
Signed-off-by: Klaus Jensen
---
include/block/nvme.h | 31 ---
hw/nvme/ctrl.c | 9 +
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 527105fafc0b..84053b68b987 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -26,7 +26,8 @@ typedef struct QEMU_PACKED NvmeBar {
uint32_tpmrsts;
uint32_tpmrebs;
uint32_tpmrswtp;
-uint64_tpmrmsc;
+uint32_tpmrmscl;
+uint32_tpmrmscu;
uint8_t css[484];
} NvmeBar;
@@ -475,25 +476,25 @@ enum NvmePmrswtpMask {
#define NVME_PMRSWTP_SET_PMRSWTV(pmrswtp, val) \
(pmrswtp |= (uint64_t)(val & PMRSWTP_PMRSWTV_MASK) <<
PMRSWTP_PMRSWTV_SHIFT)
-enum NvmePmrmscShift {
-PMRMSC_CMSE_SHIFT = 1,
-PMRMSC_CBA_SHIFT= 12,
+enum NvmePmrmsclShift {
+PMRMSCL_CMSE_SHIFT = 1,
+PMRMSCL_CBA_SHIFT= 12,
};
-enum NvmePmrmscMask {
-PMRMSC_CMSE_MASK = 0x1,
-PMRMSC_CBA_MASK= 0xf,
+enum NvmePmrmsclMask {
+PMRMSCL_CMSE_MASK = 0x1,
+PMRMSCL_CBA_MASK= 0xf,
};
-#define NVME_PMRMSC_CMSE(pmrmsc)\
-((pmrmsc >> PMRMSC_CMSE_SHIFT) & PMRMSC_CMSE_MASK)
-#define NVME_PMRMSC_CBA(pmrmsc) \
-((pmrmsc >> PMRMSC_CBA_SHIFT) & PMRMSC_CBA_MASK)
+#define NVME_PMRMSCL_CMSE(pmrmscl)\
+((pmrmscl >> PMRMSCL_CMSE_SHIFT) & PMRMSCL_CMSE_MASK)
+#define NVME_PMRMSCL_CBA(pmrmscl) \
+((pmrmscl >> PMRMSCL_CBA_SHIFT) & PMRMSCL_CBA_MASK)
-#define NVME_PMRMSC_SET_CMSE(pmrmsc, val) \
-(pmrmsc |= (uint64_t)(val & PMRMSC_CMSE_MASK) << PMRMSC_CMSE_SHIFT)
-#define NVME_PMRMSC_SET_CBA(pmrmsc, val) \
-(pmrmsc |= (uint64_t)(val & PMRMSC_CBA_MASK) << PMRMSC_CBA_SHIFT)
+#define NVME_PMRMSCL_SET_CMSE(pmrmscl, val) \
+(pmrmscl |= (uint32_t)(val & PMRMSCL_CMSE_MASK) << PMRMSCL_CMSE_SHIFT)
+#define NVME_PMRMSCL_SET_CBA(pmrmscl, val) \
+(pmrmscl |= (uint32_t)(val & PMRMSCL_CBA_MASK) << PMRMSCL_CBA_SHIFT)
enum NvmeSglDescriptorType {
NVME_SGL_DESCR_TYPE_DATA_BLOCK = 0x0,
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 2f0524e12a36..28299c6f3764 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -5916,11 +5916,12 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
-n->bar.pmrmsc = (n->bar.pmrmsc & ~0x) | (data & 0x);
+n->bar.pmrmscl = data & 0x;
n->pmr.cmse = false;
-if (NVME_PMRMSC_CMSE(n->bar.pmrmsc)) {
-hwaddr cba = NVME_PMRMSC_CBA(n->bar.pmrmsc) << PMRMSC_CBA_SHIFT;
+if (NVME_PMRMSCL_CMSE(n->bar.pmrmscl)) {
+hwaddr cba = n->bar.pmrmscu |
+(NVME_PMRMSCL_CBA(n->bar.pmrmscl) << PMRMSCL_CBA_SHIFT);
if (cba + int128_get64(n->pmr.dev->mr.size) < cba) {
NVME_PMRSTS_SET_CBAI(n->bar.pmrsts, 1);
return;
@@ -5936,7 +5937,7 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset,
uint64_t data,
return;
}
-n->bar.pmrmsc = (n->bar.pmrmsc & 0x) | (data << 32);
+n->bar.pmrmscu = data & 0x;
return;
default:
NVME_GUEST_ERR(pci_nvme_ub_mmiowr_invalid,
--
2.32.0
[PATCH v3 0/5] hw/nvme: fix mmio read
From: Klaus Jensen Fix mmio read issues on big-endian hosts. The core issue is that values in the BAR is not stored in little endian as required. Fix that and add a regression test for this. This required a bit of cleanup, so it blew up into a series. v2: * "hw/nvme: use symbolic names for registers" Use offsetof(NvmeBar, reg) instead of explicit offsets (Philippe) * "hw/nvme: fix mmio read" Use the st/ld API instead of cpu_to_X (Philippe) Klaus Jensen (5): hw/nvme: split pmrmsc register into upper and lower hw/nvme: use symbolic names for registers hw/nvme: fix out-of-bounds reads hw/nvme: fix mmio read tests/qtest/nvme-test: add mmio read test include/block/nvme.h| 60 +-- hw/nvme/ctrl.c | 362 +++- tests/qtest/nvme-test.c | 26 +++ 3 files changed, 276 insertions(+), 172 deletions(-) -- 2.32.0
