[PULL 1/2] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
Per the 'Physical Layer Simplified Specification Version 3.01',
Table 4-22: 'Block Oriented Write Protection Commands'
SEND_WRITE_PROT (CMD30)
If the card provides write protection features, this command asks
the card to send the status of the write protection bits [1].
[1] 32 write protection bits (representing 32 write protect groups
starting at the specified address) [...]
The last (least significant) bit of the protection bits corresponds
to the first addressed group. If the addresses of the last groups
are outside the valid range, then the corresponding write protection
bits shall be set to 0.
Split the if() statement (without changing the behaviour of the code)
to better position the description comment.
Reviewed-by: Alexander Bulekov
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20210802235524.3417739-2-f4...@amsat.org>
Reviewed-by: Peter Maydell
Tested-by: Alexander Bulekov
---
hw/sd/sd.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1f964e022b1..707dcc12a14 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
assert(wpnum < sd->wpgrps_size);
-if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
+if (addr >= sd->size) {
+/*
+ * If the addresses of the last groups are outside the valid range,
+ * then the corresponding write protection bits shall be set to 0.
+ */
+continue;
+}
+if (test_bit(wpnum, sd->wp_groups)) {
ret |= (1 << i);
}
}
--
2.31.1
[PULL 2/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30
OSS-Fuzz found sending illegal addresses when querying the write
protection bits triggers the assertion added in commit 84816fb63e5
("hw/sd/sdcard: Assert if accessing an illegal group"):
qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t
sd_wpbits(SDState *, uint64_t):
Assertion `wpnum < sd->wpgrps_size' failed.
#3 0x7f62a8b22c91 in __assert_fail
#4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
#5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
#6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
#7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
#8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
#9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
#10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5
It is legal for the CMD30 to query for out-of-range addresses.
Such invalid addresses are simply ignored in the response (write
protection bits set to 0).
In commit 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal
group") we misplaced the assertion *before* we test the address is
in range. Move it *after*.
Include the qtest reproducer provided by Alexander Bulekov:
$ make check-qtest-i386
...
Running test qtest-i386/fuzz-sdcard-test
qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum <
sd->wpgrps_size' failed.
Cc: qemu-sta...@nongnu.org
Reported-by: OSS-Fuzz (Issue 29225)
Suggested-by: Peter Maydell
Fixes: 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal group")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/495
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20210802235524.3417739-3-f4...@amsat.org>
Reviewed-by: Peter Maydell
Tested-by: Alexander Bulekov
---
hw/sd/sd.c | 2 +-
tests/qtest/fuzz-sdcard-test.c | 36 ++
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 707dcc12a14..bb5dbff68c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -821,7 +821,6 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
wpnum = sd_addr_to_wpnum(addr);
for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
-assert(wpnum < sd->wpgrps_size);
if (addr >= sd->size) {
/*
* If the addresses of the last groups are outside the valid range,
@@ -829,6 +828,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
*/
continue;
}
+assert(wpnum < sd->wpgrps_size);
if (test_bit(wpnum, sd->wp_groups)) {
ret |= (1 << i);
}
diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index 96602eac7e5..ae14305344a 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -52,6 +52,41 @@ static void oss_fuzz_29225(void)
qtest_quit(s);
}
+/*
+ * https://gitlab.com/qemu-project/qemu/-/issues/495
+ * Used to trigger:
+ * Assertion `wpnum < sd->wpgrps_size' failed.
+ */
+static void oss_fuzz_36217(void)
+{
+QTestState *s;
+
+s = qtest_init(" -display none -m 32 -nodefaults -nographic"
+ " -device sdhci-pci,sd-spec-version=3 "
+ "-device sd-card,drive=d0 "
+ "-drive if=none,index=0,file=null-co://,format=raw,id=d0");
+
+qtest_outl(s, 0xcf8, 0x80001010);
+qtest_outl(s, 0xcfc, 0xe000);
+qtest_outl(s, 0xcf8, 0x80001004);
+qtest_outw(s, 0xcfc, 0x02);
+qtest_bufwrite(s, 0xe02c, "\x05", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x37", 0x1);
+qtest_bufwrite(s, 0xe00a, "\x01", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x29", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x02", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x03", 0x1);
+qtest_bufwrite(s, 0xe005, "\x01", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x06", 0x1);
+qtest_bufwrite(s, 0xe00c, "\x05", 0x1);
+qtest_bufwrite(s, 0xe00e, "\x20", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x08", 0x1);
+qtest_bufwrite(s, 0xe00b, "\x3d", 0x1);
+qtest_bufwrite(s, 0xe00f, "\x1e", 0x1);
+
+qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -60,6 +95,7 @@ int main(int argc, char **argv)
if (strcmp(arch, "i386") == 0) {
qtest_add_func("fuzz/sdcard/oss_fuzz_29225", oss_fuzz_29225);
+qtest_add_func("fuzz/sdcard/oss_fuzz_36217", oss_fuzz_36217);
}
return g_test_run();
--
2.31.1
[PULL 0/2] SD/MMC patches for 2021-08-03
The following changes since commit acf8200722251a0a995cfa75fe5c15aea0886418: Merge remote-tracking branch 'remotes/mdroth/tags/qga-pull-2021-08-03-pull-tag' into staging (2021-08-03 14:48:57 +0100) are available in the Git repository at: https://github.com/philmd/qemu.git tags/sdmmc-20210803 for you to fetch changes up to 4ac0b72bae85cf94ae0e5153b9c2c288c71667d4: hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30 (2021-08-03 19:34:51 +0200) SD/MMC patches queue - sdcard: Fix assertion accessing out-of-range addresses with SEND_WRITE_PROT (CMD30) Philippe Mathieu-Daudé (2): hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30 hw/sd/sd.c | 9 - tests/qtest/fuzz-sdcard-test.c | 36 ++ 2 files changed, 44 insertions(+), 1 deletion(-) -- 2.31.1
Re: [PATCH-for-6.1 v2 0/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30
On 8/3/21 3:46 PM, Alexander Bulekov wrote: > On 210803 0155, Philippe Mathieu-Daudé wrote: >> Fix an assertion reported by OSS-Fuzz, add corresponding qtest. >> >> The change is (now) simple enough for the next rc. >> >> Since v1: >> - Simplified/corrected following Peter's suggestion >> >> Philippe Mathieu-Daudé (2): >> hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT >> hw/sd/sdcard: Fix assertion accessing out-of-range addresses with >> CMD30 >> > > Fuzzed this for 20 mins, based on the OSS-Fuzz corpus, without finding > anything. > > ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 -jobs=4 -workers=4 \ > -focus_function=sd_wpbits \ > ~/oss-fuzz/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/ > > Tested-by: Alexander Bulekov Thanks both! Queued on sdmmc-fixes.
Re: Failing iotest 206
Am 20.07.2021 um 10:32 hat Daniel P. Berrangé geschrieben:
> On Mon, Jul 19, 2021 at 08:12:58PM -0500, Eric Blake wrote:
> > On Mon, Jul 19, 2021 at 10:06:01AM +0200, Thomas Huth wrote:
> > > Hi,
> > >
> > > iotest 206 fails for me with:
> > >
> >
> > > --- 206.out
> > > +++ 206.out.bad
> > > @@ -99,55 +99,19 @@
> > >
> > > {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options":
> > > {"driver": "qcow2", "encrypt": {"cipher-alg": "twofish-128",
> > > "cipher-mode":
> > > "ctr", "format": "luks", "hash-alg": "sha1", "iter-time": 10, "ivgen-alg":
> > > "plain64", "ivgen-hash-alg": "md5", "key-secret": "keysec0"}, "file":
> > > {"driver": "file", "filename": "TEST_DIR/PID-t.qcow2"}, "size":
> > > 33554432}}}
> > > {"return": {}}
> > > +Job failed: Unsupported cipher algorithm twofish-128 with ctr mode
> > > {"execute": "job-dismiss", "arguments": {"id": "job0"}}
> > > {"return": {}}
> >
> > >
> > > Looks like it is missing a check for the availability of the corresponding
> > > crypto stuff? Does anybody got a clue how to fix this?
> >
> > What system is this on? Which crypto library versions are installed?
> > I suspect this is related to Dan's effort to speed up crypto by
> > favoring gnutls over nettle, where the switch in favored libraries
> > failed to account for whether twofish-128 is supported?
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg03886.html
>
> Yes, the gnutls provider doesn't support twofish. This doesn't matter
> in real world usage because no one is seriously going to ask for twofish
> instead of AES for luks encryption.
>
> I guess that test suite was simply trying to ask for some non-default
> values though.
Do we already have a patch somewhere that makes it use a different
value? Or if not, which value would be most likely to work everywhere?
Kevin
Re: [Question] qemu-img convert block alignment
On Fri, Apr 02, 2021 at 11:52:25AM +0800, Zhenyu Ye wrote:
> Hi all,
>
> commit 8dcd3c9b91 ("qemu-img: align result of is_allocated_sectors")
> introduces block alignment when doing qemu-img convert. However, the
> alignment is:
>
> s.alignment = MAX(pow2floor(s.min_sparse),
> DIV_ROUND_UP(out_bs->bl.request_alignment,
>BDRV_SECTOR_SIZE));
>
> (where the default s.min_sparse is 8)
> When the target device's bl.request_alignment is smaller than 4K, this
> will cause additional write-zero overhead and makes the size of target
> file larger.
>
> Is this as expected? Should we change the MAX() to MIN()?
Yes it is expected, and no we shouldn't change it. Even when a target
advertises a bl.request_alignment of 512, our goal is to avoid needing
read-modify-write cycles when that target is really on top of a 4k
sector disk. Writing extra 0s out to the 4k boundaries does not
change the fact that allocation is in 4k chunks anyways, regardless of
whether the disk supports smaller 512-byte reads.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[PULL 1/1] block: Fix in_flight leak in request padding error path
When bdrv_pad_request() fails in bdrv_co_preadv_part(), bs->in_flight
has been increased, but is never decreased again. This leads to a hang
when trying to drain the block node.
This bug was observed with Windows guests which issue a request that
fully uses IOV_MAX during installation, so that when padding is
necessary (O_DIRECT with a 4k sector size block device on the host),
adding another entry causes failure.
Call bdrv_dec_in_flight() to fix this. There is a larger problem to
solve here because this request shouldn't even fail, but Windows doesn't
seem to care and with this minimal fix the installation succeeds. So
given that we're already in freeze, let's take this minimal fix for 6.1.
Fixes: 98ca45494fcd6bf0336ecd559e440b6de6ea4cd3
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1972079
Reported-by: Qing Wang
Signed-off-by: Kevin Wolf
Message-Id: <20210727154923.91067-1-kw...@redhat.com>
Reviewed-by: Max Reitz
Signed-off-by: Kevin Wolf
---
block/io.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index e0a689c584..a19942718b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1841,7 +1841,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
ret = bdrv_pad_request(bs, &qiov, &qiov_offset, &offset, &bytes, &pad,
NULL);
if (ret < 0) {
-return ret;
+goto fail;
}
tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ);
@@ -1849,10 +1849,11 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
bs->bl.request_alignment,
qiov, qiov_offset, flags);
tracked_request_end(&req);
-bdrv_dec_in_flight(bs);
-
bdrv_padding_destroy(&pad);
+fail:
+bdrv_dec_in_flight(bs);
+
return ret;
}
--
2.31.1
[PULL 0/1] Block layer patches
The following changes since commit 7f1cab9c628a798ae2607940993771e6300e9e00: Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2021-08-02 17:21:50 +0100) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 87ab88025247b893aad5071fd38301b67be76d1a: block: Fix in_flight leak in request padding error path (2021-08-03 15:43:30 +0200) Block layer patches - Fix hang after request padding error (Windows + 512-on-4k emulation) Kevin Wolf (1): block: Fix in_flight leak in request padding error path block/io.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-)
Re: [PATCH for-6.1? v2 6/7] mirror: Check job_is_cancelled() earlier
Am 26.07.2021 um 16:46 hat Max Reitz geschrieben:
> We must check whether the job is force-cancelled early in our main loop,
> most importantly before any `continue` statement. For example, we used
> to have `continue`s before our current checking location that are
> triggered by `mirror_flush()` failing. So, if `mirror_flush()` kept
> failing, force-cancelling the job would not terminate it.
>
> A job being force-cancelled should be treated the same as the job having
> failed, so put the check in the same place where we check `s->ret < 0`.
>
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
> Signed-off-by: Max Reitz
> ---
> block/mirror.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 72e02fa34e..46d1a1e5a2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -993,7 +993,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
> mirror_wait_for_any_operation(s, true);
> }
>
> -if (s->ret < 0) {
> +if (s->ret < 0 || job_is_cancelled(&s->common.job)) {
> ret = s->ret;
> goto immediate_exit;
> }
> @@ -1078,8 +1078,6 @@ static int coroutine_fn mirror_run(Job *job, Error
> **errp)
> break;
> }
>
> -ret = 0;
> -
> if (job_is_ready(&s->common.job) && !should_complete) {
> delay_ns = (s->in_flight == 0 &&
> cnt == 0 ? BLOCK_JOB_SLICE_TIME : 0);
> @@ -1087,9 +1085,6 @@ static int coroutine_fn mirror_run(Job *job, Error
> **errp)
> trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job),
>delay_ns);
> job_sleep_ns(&s->common.job, delay_ns);
> -if (job_is_cancelled(&s->common.job)) {
> -break;
> -}
I think it was intentional that the check is here because it means
skipping the job_sleep_ns() and instead cancelling immediately, and we
probably still want that. Between your check above and here, the
coroutine can yield, so cancellation could have been newly requested.
So have the check in both places, I guess? And a comment to explain why
neither is redundant.
> s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> }
Kevin
Re: [PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel
Am 26.07.2021 um 16:46 hat Max Reitz geschrieben: > Hi, > > v1 cover letter: > https://lists.nongnu.org/archive/html/qemu-block/2021-07/msg00705.html > > Changes in v2: > - Added patch 2 (as suggested by Vladimir) > - Patch 4 (ex. 3): Rebase conflicts because of patch 2 > - Patch 5 (ex. 4): > - Rebase conflicts because of patch 2 > - Do not use job_cancel_requested() to determine how a soft-cancelled > job should be completed: A soft-cancelled job should end with > COMPLETED, not CANCELLED, and so job_is_cancelled() is the > appropriate condition there. Patches 1-4, preferably with the unrelated hunk split out from 3: Reviewed-by: Kevin Wolf
Re: [PATCH for-6.1? v2 5/7] job: Add job_cancel_requested()
Am 26.07.2021 um 16:46 hat Max Reitz geschrieben: > Most callers of job_is_cancelled() actually want to know whether the job > is on its way to immediate termination. For example, we refuse to pause > jobs that are cancelled; but this only makes sense for jobs that are > really actually cancelled. > > A mirror job that is cancelled during READY with force=false should > absolutely be allowed to pause. This "cancellation" (which is actually > a kind of completion) may take an indefinite amount of time, and so > should behave like any job during normal operation. For example, with > on-target-error=stop, the job should stop on write errors. (In > contrast, force-cancelled jobs should not get write errors, as they > should just terminate and not do further I/O.) > > Therefore, redefine job_is_cancelled() to only return true for jobs that > are force-cancelled (which as of HEAD^ means any job that interprets the > cancellation request as a request for immediate termination), and add > job_cancel_request() as the general variant, which returns true for any > jobs which have been requested to be cancelled, whether it be > immediately or after an arbitrarily long completion phase. > > Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462 > Signed-off-by: Max Reitz > --- > include/qemu/job.h | 8 +++- > block/mirror.c | 10 -- > job.c | 7 ++- > 3 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/include/qemu/job.h b/include/qemu/job.h > index 8aa90f7395..032edf3c5f 100644 > --- a/include/qemu/job.h > +++ b/include/qemu/job.h > @@ -436,9 +436,15 @@ const char *job_type_str(const Job *job); > /** Returns true if the job should not be visible to the management layer. */ > bool job_is_internal(Job *job); > > -/** Returns whether the job is scheduled for cancellation. */ > +/** Returns whether the job is being cancelled. */ > bool job_is_cancelled(Job *job); > > +/** > + * Returns whether the job is scheduled for cancellation (at an > + * indefinite point). > + */ > +bool job_cancel_requested(Job *job); I don't think non-force blockdev-cancel for mirror should actually be considered cancellation, so what is the question that this function answers? "Is this a cancelled job, or a mirror block job that is supposed to complete soon, but only if it doesn't switch over the users to the target on completion"? Is this ever a reasonable question to ask, except maybe inside the mirror implementation itself? job_complete() is the only function outside of mirror that seems to use it. But even there, it feels wrong to make a difference. Either we accept redundant completion requests, or we don't. It doesn't really matter how the job reconfigures the graph on completion. (Also, I feel this should really have been part of the state machine, but I'm not sure if we want to touch it now...) Kevin
Re: [PATCH for-6.1? v2 3/7] job: @force parameter for job_cancel_sync{,_all}()
Am 26.07.2021 um 16:46 hat Max Reitz geschrieben:
> Callers should be able to specify whether they want job_cancel_sync() to
> force-cancel the job or not.
>
> In fact, almost all invocations do not care about consistency of the
> result and just want the job to terminate as soon as possible, so they
> should pass force=true. The replication block driver is the exception.
>
> This changes some iotest outputs, because quitting qemu while a mirror
> job is active will now lead to it being cancelled instead of completed,
> which is what we want. (Cancelling a READY mirror job with force=false
> may take an indefinite amount of time, which we do not want when
> quitting. If users want consistent results, they must have all jobs be
> done before they quit qemu.)
>
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
> Signed-off-by: Max Reitz
> Reviewed-by: Vladimir Sementsov-Ogievskiy
> diff --git a/job.c b/job.c
> index e7a5d28854..9e971d64cf 100644
> --- a/job.c
> +++ b/job.c
> @@ -763,7 +763,12 @@ static void job_completed_txn_abort(Job *job)
> if (other_job != job) {
> ctx = other_job->aio_context;
> aio_context_acquire(ctx);
> -job_cancel_async(other_job, false);
> +/*
> + * This is a transaction: If one job failed, no result will
> matter.
> + * Therefore, pass force=true to terminate all other jobs as
> quickly
> + * as possible.
> + */
> +job_cancel_async(other_job, true);
> aio_context_release(ctx);
> }
> }
Sneaking in a hunk that is unrelated to what the commit message
promises? How naughty! :-)
(But I guess the change makes sense.)
Kevin
Re: [PATCH-for-6.1 v2 0/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30
On 210803 0155, Philippe Mathieu-Daudé wrote: > Fix an assertion reported by OSS-Fuzz, add corresponding qtest. > > The change is (now) simple enough for the next rc. > > Since v1: > - Simplified/corrected following Peter's suggestion > > Philippe Mathieu-Daudé (2): > hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT > hw/sd/sdcard: Fix assertion accessing out-of-range addresses with > CMD30 > Fuzzed this for 20 mins, based on the OSS-Fuzz corpus, without finding anything. ./qemu-fuzz-i386 --fuzz-target=generic-fuzz-sdhci-v3 -jobs=4 -workers=4 \ -focus_function=sd_wpbits \ ~/oss-fuzz/qemu_qemu-fuzz-i386-target-generic-fuzz-sdhci-v3/ Tested-by: Alexander Bulekov Thanks! > hw/sd/sd.c | 9 - > tests/qtest/fuzz-sdcard-test.c | 36 ++ > 2 files changed, 44 insertions(+), 1 deletion(-) > > -- > 2.31.1 >
[RFC PATCH : v3 2/2] Implementation of nvme-mi plugin in nvme-cli
From: mohit kapoor
Subject: [RFC PATCH : v3 2/2] Implementation of nvme-mi plugin in nvme-cli
The enclosed patch contains the implementation of a new
nvme mi(Management Interface) plugin. By adding the mi plugin into the
nvme-cli application, users have the ability to test the mi commands,
control and manage the nvme subsystem using nvme-cli application.
There are 3 components for the mi plugin:
1. mi plugin command layer, which is responsible for forming command packets
with nvme-mi header information and pass it on to the utility layer.
2. Utility layer, which is responsible for adding smbus/i2c header
information over nvme-mi command packet, communicate with HAL layer
for sending commands and receiving responses.
3. HAL (Hardware Abstraction Layer) layer communicates to the nvme subsystem
via sideband interface. The HAL layer is helpful in adding multiple sideband
hardware support in nvme-cli mi plugin. HAL makes use of structure with
function pointers for abstraction. Currently qemu-mi support is added in the
HAL layer as the default sideband interface. The mi plugin of nvme-cli
communicates with qemu-mi using i2c on bus 0, slave address as 0x15.
The prerequisite for executing via i2c interface is to add the qemu-mi
in the i2c bus using 'i2cdetect 0' command, which is part of i2ctools.
For executing mi commands in nvme-cli application, one needs to specify
following command format:
1. For complete list of commands : nvme mi help
2. For command help : nvme mi -h
3. For executing command : nvme mi
The mi plugin implementation is completely new and there are no changes to
other modules of nvme-cli, hence there will be no impact on any other
functionality of nvme-cli application. Currently the mi plugin contains
implementation of the mi commands of nvme-mi specification currently
supported by qemu-mi. Please review and suggest if any improvement can
be made to the design which has been followed.
v3
--rebased on master
diff --git a/qemu_nvme/nvme-cli/Makefile b/qemu_nvme/nvme-cli/Makefile
index 86eb7c6..3ea82dd 100644
--- a/qemu_nvme/nvme-cli/Makefile
+++ b/qemu_nvme/nvme-cli/Makefile
@@ -83,6 +83,13 @@ PLUGIN_OBJS := \
plugins/wdc/wdc-utils.o \
plugins/huawei/huawei-nvme.o\
plugins/netapp/netapp-nvme.o\
+ plugins/mi/util/hal/mi-nvme-hal-main.o \
+ plugins/mi/util/hal/mi-nvme-qemu/mi-nvme-qemu.o \
+ plugins/mi/util/mi-nvme-util-base.o \
+ plugins/mi/util/mi-nvme-util-crc.o \
+ plugins/mi/util/mi-nvme-util-tool.o \
+ plugins/mi/mi-nvme.o\
+ plugins/mi/mi-nvme-cmd.o\
plugins/toshiba/toshiba-nvme.o \
plugins/micron/micron-nvme.o\
plugins/seagate/seagate-nvme.o \
diff --git a/qemu_nvme/nvme-cli/plugins/mi/mi-nvme-cmd.c
b/qemu_nvme/nvme-cli/plugins/mi/mi-nvme-cmd.c
new file mode 100644
index 000..8852004
--- /dev/null
+++ b/qemu_nvme/nvme-cli/plugins/mi/mi-nvme-cmd.c
@@ -0,0 +1,126 @@
+/*
+ * Copyright (C) 2021 Samsung Electronics Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * ~~
+ * mi-nvme-cmd.c - Implementation of NVMe Management Interface commands in Nvme
+ *
+ * Developer: Mohit Kapoor
+ */
+
+#include "mi-nvme-cmd.h"
+
+uint32_t gettransmissionunitsize()
+{
+return MCTP_TRANS_UNIT_SIZE_VAL_DEF;
+}
+
+int executecommand(__u8 *cmdobj)
+{
+struct nvme_mi_message message;
+memset(&message, 0, sizeof(struct nvme_mi_message));
+nvme_mi_admin_message adminmessage;
+memset(&adminmessage, 0, sizeof(nvme_mi_admin_message));
+bool ret = false;
+uint32_t RequestDataSize = 0;
+struct gencmd_nvmemi *micmd;
+struct gencmd_nvmemi_admin *miadmincmd;
+uint32_t uiMCTP_TUS = gettransmissionunitsize();
+
+ret = initialize(uiMCTP_TUS);
+if (ret == false) {
+return -1;
+}
+
+streply.length = 0;
+streply.errorcode = 0;
+memset(streply.replybuf, 0, sizeof(streply.replybuf));
+memcpy(&message.msgPld, cmdobj, sizeof(struct nvme_mi_message) - 8);
+
+/*Check if the incoming command is an MI/MI-Admin Command*/
+if (message.msgPld.nvme_mi_message_header & 0x1000) {
+miadmincmd = (struct gencmd_nvmemi_admin *)cmdobj;
+memcpy(&adminmessage.msgPld, cmdobj, sizeof(nvme_mi_admin_message) -
8);
+
Re: [PATCH for-6.1? v2 5/7] job: Add job_cancel_requested()
02.08.2021 13:23, Max Reitz wrote:
On 27.07.21 17:47, Vladimir Sementsov-Ogievskiy wrote:
27.07.2021 18:39, Max Reitz wrote:
On 27.07.21 15:04, Vladimir Sementsov-Ogievskiy wrote:
26.07.2021 17:46, Max Reitz wrote:
Most callers of job_is_cancelled() actually want to know whether the job
is on its way to immediate termination. For example, we refuse to pause
jobs that are cancelled; but this only makes sense for jobs that are
really actually cancelled.
A mirror job that is cancelled during READY with force=false should
absolutely be allowed to pause. This "cancellation" (which is actually
a kind of completion) may take an indefinite amount of time, and so
should behave like any job during normal operation. For example, with
on-target-error=stop, the job should stop on write errors. (In
contrast, force-cancelled jobs should not get write errors, as they
should just terminate and not do further I/O.)
Therefore, redefine job_is_cancelled() to only return true for jobs that
are force-cancelled (which as of HEAD^ means any job that interprets the
cancellation request as a request for immediate termination), and add
job_cancel_request() as the general variant, which returns true for any
job_cancel_requested()
jobs which have been requested to be cancelled, whether it be
immediately or after an arbitrarily long completion phase.
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
Signed-off-by: Max Reitz
---
include/qemu/job.h | 8 +++-
block/mirror.c | 10 --
job.c | 7 ++-
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 8aa90f7395..032edf3c5f 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -436,9 +436,15 @@ const char *job_type_str(const Job *job);
/** Returns true if the job should not be visible to the management layer. */
bool job_is_internal(Job *job);
-/** Returns whether the job is scheduled for cancellation. */
+/** Returns whether the job is being cancelled. */
bool job_is_cancelled(Job *job);
+/**
+ * Returns whether the job is scheduled for cancellation (at an
+ * indefinite point).
+ */
+bool job_cancel_requested(Job *job);
+
/** Returns whether the job is in a completed state. */
bool job_is_completed(Job *job);
diff --git a/block/mirror.c b/block/mirror.c
index e93631a9f6..72e02fa34e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -936,7 +936,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
/* Transition to the READY state and wait for complete. */
job_transition_to_ready(&s->common.job);
s->actively_synced = true;
- while (!job_is_cancelled(&s->common.job) && !s->should_complete) {
+ while (!job_cancel_requested(&s->common.job) && !s->should_complete) {
job_yield(&s->common.job);
}
s->common.job.cancelled = false;
@@ -1043,7 +1043,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
}
should_complete = s->should_complete ||
- job_is_cancelled(&s->common.job);
+ job_cancel_requested(&s->common.job);
cnt = bdrv_get_dirty_count(s->dirty_bitmap);
}
@@ -1087,7 +1087,7 @@ static int coroutine_fn mirror_run(Job *job, Error
**errp)
trace_mirror_before_sleep(s, cnt, job_is_ready(&s->common.job),
delay_ns);
job_sleep_ns(&s->common.job, delay_ns);
- if (job_is_cancelled(&s->common.job) && s->common.job.force_cancel) {
+ if (job_is_cancelled(&s->common.job)) {
break;
}
s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
@@ -1099,9 +1099,7 @@ immediate_exit:
* or it was cancelled prematurely so that we do not guarantee that
* the target is a copy of the source.
*/
- assert(ret < 0 ||
- (s->common.job.force_cancel &&
- job_is_cancelled(&s->common.job)));
+ assert(ret < 0 || job_is_cancelled(&s->common.job));
(As a note, I hope this does the job regarding your suggestions for patch 4. :))
assert(need_drain);
mirror_wait_for_all_io(s);
}
diff --git a/job.c b/job.c
index e78d893a9c..dba17a680f 100644
--- a/job.c
+++ b/job.c
@@ -216,6 +216,11 @@ const char *job_type_str(const Job *job)
}
bool job_is_cancelled(Job *job)
+{
+ return job->cancelled && job->force_cancel;
can job->cancelled be false when job->force_cancel is true ? I think not and
worth an assertion here. Something like
if (job->force_cancel) {
assert(job->cancelled);
return true;
}
return false;
Sounds good, why not.
+}
+
+bool job_cancel_requested(Job *job)
{
return job->cancelled;
}
@@ -1015,7 +1020,7 @@ void job_complete(Job *job, Error **errp)
if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) {
return;
}
- if (job_is_cancelled(job) || !job->driver->
Re: [PATCH-for-6.1 v2 1/2] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
On Tue, 3 Aug 2021 at 00:55, Philippe Mathieu-Daudé wrote:
>
> Per the 'Physical Layer Simplified Specification Version 3.01',
> Table 4-22: 'Block Oriented Write Protection Commands'
>
> SEND_WRITE_PROT (CMD30)
>
> If the card provides write protection features, this command asks
> the card to send the status of the write protection bits [1].
>
> [1] 32 write protection bits (representing 32 write protect groups
> starting at the specified address) [...]
> The last (least significant) bit of the protection bits corresponds
> to the first addressed group. If the addresses of the last groups
> are outside the valid range, then the corresponding write protection
> bits shall be set to 0.
>
> Split the if() statement (without changing the behaviour of the code)
> to better position the description comment.
>
> Reviewed-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/sd/sd.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1f964e022b1..707dcc12a14 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>
> for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
> assert(wpnum < sd->wpgrps_size);
> -if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
> +if (addr >= sd->size) {
> +/*
> + * If the addresses of the last groups are outside the valid
> range,
> + * then the corresponding write protection bits shall be set to
> 0.
> + */
> +continue;
> +}
> +if (test_bit(wpnum, sd->wp_groups)) {
> ret |= (1 << i);
Reviewed-by: Peter Maydell
thanks
-- PMM
Re: [PATCH-for-6.1 v2 2/2] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30
On Tue, 3 Aug 2021 at 00:55, Philippe Mathieu-Daudé wrote:
>
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers the assertion added in commit 84816fb63e5
> ("hw/sd/sdcard: Assert if accessing an illegal group"):
>
> qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t
> sd_wpbits(SDState *, uint64_t):
> Assertion `wpnum < sd->wpgrps_size' failed.
> #3 0x7f62a8b22c91 in __assert_fail
> #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
> #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
> #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
> #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
> #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
> #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
> #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5
>
> It is legal for the CMD30 to query for out-of-range addresses.
> Such invalid addresses are simply ignored in the response (write
> protection bits set to 0).
>
> In commit 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal
> group") we misplaced the assertion *before* we test the address is
> in range. Move it *after*.
>
> Include the qtest reproducer provided by Alexander Bulekov:
>
> $ make check-qtest-i386
> ...
> Running test qtest-i386/fuzz-sdcard-test
> qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum <
> sd->wpgrps_size' failed.
>
> Cc: qemu-sta...@nongnu.org
> Reported-by: OSS-Fuzz (Issue 29225)
> Suggested-by: Peter Maydell
> Fixes: 84816fb63e5 ("hw/sd/sdcard: Assert if accessing an illegal group")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/495
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Peter Maydell
thanks
-- PMM
[RFC PATCH: v3 1/2] add mi device in qemu
From: padmakar
This patch contains the implementation of certain commands
of nvme-mi specification.The MI commands are useful to
manage/configure/monitor the device.Eventhough the MI commands
can be sent via the inband NVMe-MI send/recieve commands, the idea
here is to emulate the sideband interface for MI.
The changes here includes the interface for i2c/smbus
for nvme-mi protocol. We have used i2c address of 0x15
using which the guest VM can send and recieve the nvme-mi
commands. Since the nvme-mi device uses the I2C_SLAVE as
parent, we have used the send and recieve callbacks by
which the nvme-mi device will get the required notification.
With the callback approach, we have eliminated the use of
threads.
One needs to specify the following command in the launch to
specify the nvme-mi device, link to nvme and assign the i2c address.
<-device nvme-mi,nvme=nvme0,address=0x15>
This module makes use of the NvmeCtrl structure of the nvme module,
to fetch relevant information of the nvme device which are used in
some of the mi commands. Eventhough certain commands might require
modification to the nvme module, currently we have currently refrained
from making changes to the nvme module.
cmd-name cmd-type
*
read nvme-mi dsnvme-mi
configuration set nvme-mi
configuration get nvme-mi
vpd read nvme-mi
vpd write nvme-mi
controller Health Staus Poll nvme-mi
nvme subsystem health status poll nvme-mi
identify nvme-admin
get log page nvme-admin
get features nvme-admin
Signed-off-by: Padmakar Kalghatgi
Reviewed-by: Klaus Birkelund Jensen
Reviewed-by: Jaegyu Choi
v3
-- rebased on master
---
hw/nvme/meson.build | 2 +-
hw/nvme/nvme-mi.c | 650
hw/nvme/nvme-mi.h | 297
3 files changed, 948 insertions(+), 1 deletion(-)
create mode 100644 hw/nvme/nvme-mi.c
create mode 100644 hw/nvme/nvme-mi.h
diff --git a/hw/nvme/meson.build b/hw/nvme/meson.build
index 3cf4004..8dac50e 100644
--- a/hw/nvme/meson.build
+++ b/hw/nvme/meson.build
@@ -1 +1 @@
-softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c',
'ns.c', 'subsys.c'))
+softmmu_ss.add(when: 'CONFIG_NVME_PCI', if_true: files('ctrl.c', 'dif.c',
'ns.c', 'subsys.c', 'nvme-mi.c'))
diff --git a/hw/nvme/nvme-mi.c b/hw/nvme/nvme-mi.c
new file mode 100644
index 000..a90ce90
--- /dev/null
+++ b/hw/nvme/nvme-mi.c
@@ -0,0 +1,650 @@
+/*
+ * QEMU NVMe-MI Controller
+ *
+ * Copyright (c) 2021, Samsung Electronics co Ltd.
+ *
+ * Written by Padmakar Kalghatgi
+ *Arun Kumar Agasar
+ *Saurav Kumar
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+/**
+ * Reference Specs: http://www.nvmexpress.org,
+ *
+ *
+ * Usage
+ * -
+ * The nvme-mi device has to be used along with nvme device only
+ *
+ * Add options:
+ *-device nvme-mi,nvme=,address=0x15",
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+#include "hw/block/block.h"
+#include "hw/pci/msix.h"
+#include "nvme.h"
+#include "nvme-mi.h"
+#include "qemu/crc32c.h"
+
+#define NVME_TEMPERATURE 0x143
+#define NVME_TEMPERATURE_WARNING 0x157
+#define NVME_TEMPERATURE_CRITICAL 0x175
+
+static void nvme_mi_send_resp(NvmeMiCtrl *ctrl_mi, uint8_t *resp, uint32_t
size)
+{
+uint32_t crc_value = crc32c(0x, resp, size);
+size += 4;
+uint32_t offset = 0;
+uint32_t ofst = 0;
+uint32_t som = 1;
+uint32_t eom = 0;
+uint32_t pktseq = 0;
+uint32_t mtus = ctrl_mi->mctp_unit_size;
+while (size > 0) {
+uint32_t sizesent = size > mtus ? mtus : size;
+size -= sizesent;
+eom = size > 0 ? 0 : 1;
+g_autofree uint8_t *buf = (uint8_t *)g_malloc0(sizesent + 8);
+buf[2] = sizesent + 5;
+buf[7] = (som << 7) | (eom << 6) | (pktseq << 5);
+som = 0;
+memcpy(buf + 8, resp + offset, sizesent);
+offset += sizesent;
+if (size <= 0) {
+memcpy(buf + 8 + offset , &crc_value, sizeof(crc_value));
+}
+memcpy(ctrl_mi->misendrecv.sendrecvbuf + ofst, buf, sizesent + 8);
+ofst += sizesent + 8;
+}
+}
+
+static void nvme_mi_resp_hdr_init(NvmeMIResponse *resp, bool bAdminCommand)
+{
+resp->msg_header.msgtype = 4;
+resp->msg_header.ic = 1;
+resp->msg_header.csi = 0;
+resp->msg_header.reserved = 0;
+resp->msg_header.nmimt = bAdminCommand ? 2 : 1;
+resp->msg_header.ror = 1;
+resp->msg_header.reserved1 = 0;
+}
+static void nvme_mi_nvm_subsys_ds(NvmeMiCtrl *ctrl_mi, NvmeMIRequest *req)
+{
+NvmeMIResponse resp;
+NvmMiSubsysInfoDs ds;
+ds.nump = 1;
+ds.mjr = (ctrl_mi->n->bar.vs & 0xFF) >> 16;
+ds.mnr = (ctrl
