[PATCH] block/curl: use strlen instead of strchr
We already know where colon is, so no reason to search for it. Also,
avoid a code, which looks like we forget to check return value of
strchr() to NULL.
Suggested-by: Kevin Wolf
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
This replaces my patch
[PATCH] block/curl: explicitly assert that strchr returns non-NULL value
Supersedes: <20240627153059.589070-1-vsement...@yandex-team.ru>
block/curl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 419f7c89ef..d03bfe4817 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -219,7 +219,7 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t
nmemb, void *opaque)
&& g_ascii_strncasecmp(header, accept_ranges,
strlen(accept_ranges)) == 0) {
-char *p = strchr(header, ':') + 1;
+char *p = header + strlen(accept_ranges);
/* Skip whitespace between the header name and value. */
while (p < end && *p && g_ascii_isspace(*p)) {
--
2.34.1
Re: [PATCH] block/curl: explicitly assert that strchr returns non-NULL value
On 27.06.24 21:05, Kevin Wolf wrote:
Am 27.06.2024 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
strchr may return NULL if colon is not found. It seems clearer to
assert explicitly that we don't expect it here, than dereference 1 in
the next line.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/curl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/curl.c b/block/curl.c
index 419f7c89ef..ccfffd6c12 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t
nmemb, void *opaque)
&& g_ascii_strncasecmp(header, accept_ranges,
strlen(accept_ranges)) == 0) {
-char *p = strchr(header, ':') + 1;
+char *p = strchr(header, ':');
+assert(p != NULL);
+p += 1;
I'm not sure if this is actually much clearer because it doesn't say why
we don't expect NULL here. If you don't look at the context, it almost
looks like an assert() where proper error handling is needed. If you do,
then the original line is clear enough.
My first thought was that maybe what we want is a comment, but we
actually already know where the colon is. So how about this instead:
char *p = header + strlen(accept_ranges);
Oh, right. That's better.
/* Skip whitespace between the header name and value. */
while (p < end && *p && g_ascii_isspace(*p)) {
--
2.34.1
--
Best regards,
Vladimir
[PATCH v6 07/11] gdbstub: Make hex conversion function non-internal
Make gdb_hextomem non-internal so it's not confined to use only in gdbstub.c. Signed-off-by: Gustavo Romero Reviewed-by: Richard Henderson --- gdbstub/internals.h| 1 - include/gdbstub/commands.h | 6 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/gdbstub/internals.h b/gdbstub/internals.h index 34121dc61a..bf5a5c6302 100644 --- a/gdbstub/internals.h +++ b/gdbstub/internals.h @@ -107,7 +107,6 @@ static inline int tohex(int v) void gdb_put_strbuf(void); int gdb_put_packet_binary(const char *buf, int len, bool dump); -void gdb_hextomem(GByteArray *mem, const char *buf, int len); void gdb_memtohex(GString *buf, const uint8_t *mem, int len); void gdb_memtox(GString *buf, const char *mem, int len); void gdb_read_byte(uint8_t ch); diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h index 306dfdef97..e51f276b40 100644 --- a/include/gdbstub/commands.h +++ b/include/gdbstub/commands.h @@ -91,4 +91,10 @@ void gdb_extend_set_table(GdbCmdParseEntry *table, int size); */ void gdb_extend_qsupported_features(char *qsupported_features); +/** + * Convert a hex string to bytes. Conversion is done per byte, so 2 hex digits + * are converted to 1 byte. Invalid hex digits are treated as 0 digits. + */ +void gdb_hextomem(GByteArray *mem, const char *buf, int len); + #endif /* GDBSTUB_COMMANDS_H */ -- 2.34.1
[PATCH v6 11/11] tests/tcg/aarch64: Add MTE gdbstub tests
Add tests to exercise the MTE stubs. The tests will only run if a
version of GDB that supports MTE is available in the test environment.
Signed-off-by: Gustavo Romero
---
configure | 4 ++
tests/tcg/aarch64/Makefile.target | 14 +++-
tests/tcg/aarch64/gdbstub/test-mte.py | 86 +++
tests/tcg/aarch64/mte-8.c | 98 +++
4 files changed, 201 insertions(+), 1 deletion(-)
create mode 100644 tests/tcg/aarch64/gdbstub/test-mte.py
create mode 100644 tests/tcg/aarch64/mte-8.c
diff --git a/configure b/configure
index 5ad1674ca5..10f7e1259a 100755
--- a/configure
+++ b/configure
@@ -1673,6 +1673,10 @@ for target in $target_list; do
echo "GDB=$gdb_bin" >> $config_target_mak
fi
+ if test "${arch}" = "aarch64" && version_ge ${gdb_version##* } 15.0; then
+ echo "GDB_HAS_MTE=y" >> $config_target_mak
+ fi
+
echo "run-tcg-tests-$target: $qemu\$(EXESUF)" >> Makefile.prereqs
tcg_tests_targets="$tcg_tests_targets $target"
fi
diff --git a/tests/tcg/aarch64/Makefile.target
b/tests/tcg/aarch64/Makefile.target
index 70d728ae9a..f306e3d257 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -62,7 +62,7 @@ AARCH64_TESTS += bti-2
# MTE Tests
ifneq ($(CROSS_CC_HAS_ARMV8_MTE),)
-AARCH64_TESTS += mte-1 mte-2 mte-3 mte-4 mte-5 mte-6 mte-7
+AARCH64_TESTS += mte-1 mte-2 mte-3 mte-4 mte-5 mte-6 mte-7 mte-8
mte-%: CFLAGS += -march=armv8.5-a+memtag
endif
@@ -128,6 +128,18 @@ run-gdbstub-sve-ioctls: sve-ioctls
basic gdbstub SVE ZLEN support)
EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
+
+ifeq ($(GDB_HAS_MTE),y)
+run-gdbstub-mte: mte-8
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(GDB) \
+ --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+ --bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py, \
+ gdbstub MTE support)
+
+EXTRA_RUNS += run-gdbstub-mte
+endif
+
endif
endif
diff --git a/tests/tcg/aarch64/gdbstub/test-mte.py
b/tests/tcg/aarch64/gdbstub/test-mte.py
new file mode 100644
index 00..2db0663c1a
--- /dev/null
+++ b/tests/tcg/aarch64/gdbstub/test-mte.py
@@ -0,0 +1,86 @@
+from __future__ import print_function
+#
+# Test GDB memory-tag commands that exercise the stubs for the
qIsAddressTagged,
+# qMemTag, and QMemTag packets. Logical tag-only commands rely on local
+# operations, hence don't exercise any stub.
+#
+# The test consists in breaking just after a atag() call (which sets the
+# allocation tag -- see mte-8.c for details) and setting/getting tags in
+# different memory locations and ranges starting at the address of the array
+# 'a'.
+#
+# This is launched via tests/guest-debug/run-test.py
+#
+
+
+import gdb
+import re
+from test_gdbstub import main, report
+
+
+PATTERN_0 = "Memory tags for address 0x[0-9a-f]+ match \(0x[0-9a-f]+\)."
+PATTERN_1 = ".*(0x[0-9a-f]+)"
+
+
+def run_test():
+gdb.execute("break 95", False, True)
+gdb.execute("continue", False, True)
+try:
+# Test if we can check correctly that the allocation tag for
+# array 'a' matches the logical tag after atag() is called.
+co = gdb.execute("memory-tag check a", False, True)
+tags_match = re.findall(PATTERN_0, co, re.MULTILINE)
+if tags_match:
+report(True, f"{tags_match[0]}")
+else:
+report(False, "Logical and allocation tags don't match!")
+
+# Test allocation tag 'set and print' commands. Commands on logical
+# tags rely on local operation and so don't exercise any stub.
+
+# Set the allocation tag for the first granule (16 bytes) of
+# address starting at 'a' address to a known value, i.e. 0x04.
+gdb.execute("memory-tag set-allocation-tag a 1 04", False, True)
+
+# Then set the allocation tag for the second granule to a known
+# value, i.e. 0x06. This tests that contiguous tag granules are
+# set correct and don't run over each other.
+gdb.execute("memory-tag set-allocation-tag a+16 1 06", False, True)
+
+# Read the known values back and check if they remain the same.
+
+co = gdb.execute("memory-tag print-allocation-tag a", False, True)
+first_tag = re.match(PATTERN_1, co)[1]
+
+co = gdb.execute("memory-tag print-allocation-tag a+16", False, True)
+second_tag = re.match(PATTERN_1, co)[1]
+
+if first_tag == "0x4" and second_tag == "0x6":
+report(True, "Allocation tags are correctly set/printed.")
+else:
+report(False, "Can't set/print allocation tags!")
+
+# Now test fill pattern by setting a whole page with a pattern.
+gdb.execute("memory-tag set-allocation-tag a 4096 0a0b", False, True)
+
+# And read back the tags of the last two granules in page so
+# we also test if the pattern is set correctly up to the end of
+# the
[PATCH v6 08/11] gdbstub: Pass CPU context to command handler
Allow passing the current CPU context to command handlers via user_ctx
when the handler requires it.
Signed-off-by: Alex Bennée
Signed-off-by: Gustavo Romero
---
gdbstub/gdbstub.c | 7 ++-
include/gdbstub/commands.h | 3 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b1ca253f97..5c1612ed2a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -938,6 +938,7 @@ static bool process_string_cmd(const char *data,
for (i = 0; i < num_cmds; i++) {
const GdbCmdParseEntry *cmd = [i];
+void *user_ctx = NULL;
g_assert(cmd->handler && cmd->cmd);
if ((cmd->cmd_startswith && !startswith(data, cmd->cmd)) ||
@@ -952,8 +953,12 @@ static bool process_string_cmd(const char *data,
}
}
+if (cmd->need_cpu_context) {
+user_ctx = (void *)gdbserver_state.g_cpu;
+}
+
gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
-cmd->handler(params, NULL);
+cmd->handler(params, user_ctx);
return true;
}
diff --git a/include/gdbstub/commands.h b/include/gdbstub/commands.h
index e51f276b40..f3058f9dda 100644
--- a/include/gdbstub/commands.h
+++ b/include/gdbstub/commands.h
@@ -54,6 +54,8 @@ typedef union GdbCmdVariant {
* "stop reply" packet. The list of commands that accept such response is
* defined at the GDB Remote Serial Protocol documentation. See:
*
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
+ *
+ * @need_cpu_context: Pass current CPU context to command handler via user_ctx.
*/
typedef struct GdbCmdParseEntry {
GdbCmdHandler handler;
@@ -61,6 +63,7 @@ typedef struct GdbCmdParseEntry {
bool cmd_startswith;
const char *schema;
bool allow_stop_reply;
+bool need_cpu_context;
} GdbCmdParseEntry;
/**
--
2.34.1
[PATCH v6 04/11] target/arm: Fix exception case in allocation_tag_mem_probe
If page in 'ptr_access' is inaccessible and probe is 'true'
allocation_tag_mem_probe should not throw an exception, but currently it
does, so fix it.
Signed-off-by: Gustavo Romero
Reviewed-by: Alex Bennée
Reviewed-by: Richard Henderson
---
target/arm/tcg/mte_helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index 037ac6dd60..a50d576294 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -96,6 +96,9 @@ static uint8_t *allocation_tag_mem_probe(CPUARMState *env,
int ptr_mmu_idx,
assert(!(probe && ra));
if (!(flags & (ptr_access == MMU_DATA_STORE ? PAGE_WRITE_ORG :
PAGE_READ))) {
+if (probe) {
+return NULL;
+}
cpu_loop_exit_sigsegv(env_cpu(env), ptr, ptr_access,
!(flags & PAGE_VALID), ra);
}
--
2.34.1
[PATCH v6 09/11] gdbstub: Use true to set cmd_startswith
cmd_startswith is a boolean so use 'true' to set it instead of 1.
Signed-off-by: Gustavo Romero
---
gdbstub/gdbstub.c | 80 +++
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 5c1612ed2a..b9ad0a063e 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1433,26 +1433,26 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
{
.handler = handle_v_cont_query,
.cmd = "Cont?",
-.cmd_startswith = 1
+.cmd_startswith = true
},
{
.handler = handle_v_cont,
.cmd = "Cont",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.allow_stop_reply = true,
.schema = "s0"
},
{
.handler = handle_v_attach,
.cmd = "Attach;",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.allow_stop_reply = true,
.schema = "l0"
},
{
.handler = handle_v_kill,
.cmd = "Kill;",
-.cmd_startswith = 1
+.cmd_startswith = true
},
#ifdef CONFIG_USER_ONLY
/*
@@ -1462,25 +1462,25 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
{
.handler = gdb_handle_v_file_open,
.cmd = "File:open:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s,L,L0"
},
{
.handler = gdb_handle_v_file_close,
.cmd = "File:close:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l0"
},
{
.handler = gdb_handle_v_file_pread,
.cmd = "File:pread:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l,L,L0"
},
{
.handler = gdb_handle_v_file_readlink,
.cmd = "File:readlink:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s0"
},
#endif
@@ -1748,7 +1748,7 @@ static const GdbCmdParseEntry
gdb_gen_query_set_common_table[] = {
{
.handler = handle_set_qemu_sstep,
.cmd = "qemu.sstep=",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l0"
},
};
@@ -1804,7 +1804,7 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = handle_query_thread_extra,
.cmd = "ThreadExtraInfo,",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "t0"
},
#ifdef CONFIG_USER_ONLY
@@ -1816,14 +1816,14 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = gdb_handle_query_rcmd,
.cmd = "Rcmd,",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s0"
},
#endif
{
.handler = handle_query_supported,
.cmd = "Supported:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s0"
},
{
@@ -1834,7 +1834,7 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = handle_query_xfer_features,
.cmd = "Xfer:features:read:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s:l,l0"
},
#if defined(CONFIG_USER_ONLY)
@@ -1842,27 +1842,27 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = gdb_handle_query_xfer_auxv,
.cmd = "Xfer:auxv:read::",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l,l0"
},
{
.handler = gdb_handle_query_xfer_siginfo,
.cmd = "Xfer:siginfo:read::",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l,l0"
},
#endif
{
.handler = gdb_handle_query_xfer_exec_file,
.cmd = "Xfer:exec-file:read:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l:l,l0"
},
#endif
{
.handler = gdb_handle_query_attached,
.cmd = "Attached:",
-.cmd_startswith = 1
+.cmd_startswith = true
},
{
.handler = gdb_handle_query_attached,
@@ -1901,14 +1901,14 @@ static const GdbCmdParseEntry gdb_gen_set_table[] = {
{
.handler = handle_set_qemu_sstep,
.cmd = "qemu.sstep:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l0"
},
#ifndef CONFIG_USER_ONLY
{
.handler = gdb_handle_set_qemu_phy_mem_mode,
.cmd = "qemu.PhyMemMode:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "l0"
},
#endif
@@ -1916,7 +1916,7 @@ static const GdbCmdParseEntry gdb_gen_set_table[] = {
{
.handler = gdb_handle_set_catch_syscalls,
.cmd = "CatchSyscalls:",
-.cmd_startswith = 1,
+.cmd_startswith = true,
.schema = "s0",
},
#endif
@@ -2012,7 +2012,7 @@ static int gdb_handle_packet(const char *line_buf)
static const
[PATCH v6 06/11] target/arm: Factor out code for setting MTE TCF0 field
Factor out the code used for setting the MTE TCF0 field from the prctl
code into a convenient function. Other subsystems, like gdbstub, need to
set this field as well, so keep it as a separate function to avoid
duplication and ensure consistency in how this field is set across the
board.
Signed-off-by: Gustavo Romero
---
linux-user/aarch64/meson.build | 2 ++
linux-user/aarch64/mte_user_helper.c | 34
linux-user/aarch64/mte_user_helper.h | 25
linux-user/aarch64/target_prctl.h| 22 ++
4 files changed, 63 insertions(+), 20 deletions(-)
create mode 100644 linux-user/aarch64/mte_user_helper.c
create mode 100644 linux-user/aarch64/mte_user_helper.h
diff --git a/linux-user/aarch64/meson.build b/linux-user/aarch64/meson.build
index 248c578d15..f75bb3cd75 100644
--- a/linux-user/aarch64/meson.build
+++ b/linux-user/aarch64/meson.build
@@ -9,3 +9,5 @@ vdso_le_inc = gen_vdso.process('vdso-le.so',
extra_args: ['-r', '__kernel_rt_sigreturn'])
linux_user_ss.add(when: 'TARGET_AARCH64', if_true: [vdso_be_inc, vdso_le_inc])
+
+linux_user_ss.add(when: 'TARGET_AARCH64', if_true:
[files('mte_user_helper.c')])
diff --git a/linux-user/aarch64/mte_user_helper.c
b/linux-user/aarch64/mte_user_helper.c
new file mode 100644
index 00..8be6deaf03
--- /dev/null
+++ b/linux-user/aarch64/mte_user_helper.c
@@ -0,0 +1,34 @@
+/*
+ * ARM MemTag convenience functions.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include
+#include "mte_user_helper.h"
+
+void arm_set_mte_tcf0(CPUArchState *env, abi_long value)
+{
+/*
+ * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
+ *
+ * The kernel has a per-cpu configuration for the sysadmin,
+ * /sys/devices/system/cpu/cpu/mte_tcf_preferred,
+ * which qemu does not implement.
+ *
+ * Because there is no performance difference between the modes, and
+ * because SYNC is most useful for debugging MTE errors, choose SYNC
+ * as the preferred mode. With this preference, and the way the API
+ * uses only two bits, there is no way for the program to select
+ * ASYMM mode.
+ */
+unsigned tcf = 0;
+if (value & PR_MTE_TCF_SYNC) {
+tcf = 1;
+} else if (value & PR_MTE_TCF_ASYNC) {
+tcf = 2;
+}
+env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
+}
diff --git a/linux-user/aarch64/mte_user_helper.h
b/linux-user/aarch64/mte_user_helper.h
new file mode 100644
index 00..ee3f6b190a
--- /dev/null
+++ b/linux-user/aarch64/mte_user_helper.h
@@ -0,0 +1,25 @@
+/*
+ * ARM MemTag convenience functions.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef AARCH64_MTE_USER_HELPER_H
+#define AARCH64_MTE USER_HELPER_H
+
+#include "qemu/osdep.h"
+#include "qemu.h"
+
+/**
+ * arm_set_mte_tcf0 - Set TCF0 field in SCTLR_EL1 register
+ * @env: The CPU environment
+ * @value: The value to be set for the Tag Check Fault in EL0 field.
+ *
+ * Only SYNC and ASYNC modes can be selected. If ASYMM mode is given, the SYNC
+ * mode is selected instead. So, there is no way to set the ASYMM mode.
+ */
+void arm_set_mte_tcf0(CPUArchState *env, abi_long value);
+
+#endif /* AARCH64_MTE_USER_HELPER_H */
diff --git a/linux-user/aarch64/target_prctl.h
b/linux-user/aarch64/target_prctl.h
index aa8e203c15..ed75b9e4b5 100644
--- a/linux-user/aarch64/target_prctl.h
+++ b/linux-user/aarch64/target_prctl.h
@@ -7,6 +7,7 @@
#define AARCH64_TARGET_PRCTL_H
#include "target/arm/cpu-features.h"
+#include "mte_user_helper.h"
static abi_long do_prctl_sve_get_vl(CPUArchState *env)
{
@@ -173,26 +174,7 @@ static abi_long do_prctl_set_tagged_addr_ctrl(CPUArchState
*env, abi_long arg2)
env->tagged_addr_enable = arg2 & PR_TAGGED_ADDR_ENABLE;
if (cpu_isar_feature(aa64_mte, cpu)) {
-/*
- * Write PR_MTE_TCF to SCTLR_EL1[TCF0].
- *
- * The kernel has a per-cpu configuration for the sysadmin,
- * /sys/devices/system/cpu/cpu/mte_tcf_preferred,
- * which qemu does not implement.
- *
- * Because there is no performance difference between the modes, and
- * because SYNC is most useful for debugging MTE errors, choose SYNC
- * as the preferred mode. With this preference, and the way the API
- * uses only two bits, there is no way for the program to select
- * ASYMM mode.
- */
-unsigned tcf = 0;
-if (arg2 & PR_MTE_TCF_SYNC) {
-tcf = 1;
-} else if (arg2 & PR_MTE_TCF_ASYNC) {
-tcf = 2;
-}
-env->cp15.sctlr_el[1] = deposit64(env->cp15.sctlr_el[1], 38, 2, tcf);
+arm_set_mte_tcf0(env, arg2);
/*
* Write PR_MTE_TAG to GCR_EL1[Exclude].
--
2.34.1
[PATCH v6 03/11] gdbstub: Add support for target-specific stubs
Currently, it's not possible to have stubs specific to a given target,
even though there are GDB features which are target-specific, like, for
instance, memory tagging.
This commit introduces gdb_extend_qsupported_features,
gdb_extend_query_table, and gdb_extend_set_table functions as interfaces
to extend the qSupported string, the query handler table, and the set
handler table, allowing target-specific stub implementations.
Signed-off-by: Gustavo Romero
Reviewed-by: Alex Bennée
---
gdbstub/gdbstub.c | 102 ++---
include/gdbstub/commands.h | 22
2 files changed, 118 insertions(+), 6 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9ff2f4177d..b1ca253f97 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1609,6 +1609,20 @@ static void handle_query_thread_extra(GArray *params,
void *user_ctx)
gdb_put_strbuf();
}
+static char *extended_qsupported_features;
+void gdb_extend_qsupported_features(char *qsupported_features)
+{
+/*
+ * We don't support different sets of CPU gdb features on different CPUs
yet
+ * so assert the feature strings are the same on all CPUs, or is set only
+ * once (1 CPU).
+ */
+g_assert(extended_qsupported_features == NULL ||
+ g_strcmp0(extended_qsupported_features, qsupported_features) ==
0);
+
+extended_qsupported_features = qsupported_features;
+}
+
static void handle_query_supported(GArray *params, void *user_ctx)
{
CPUClass *cc;
@@ -1648,6 +1662,11 @@ static void handle_query_supported(GArray *params, void
*user_ctx)
}
g_string_append(gdbserver_state.str_buf, ";vContSupported+;multiprocess+");
+
+if (extended_qsupported_features) {
+g_string_append(gdbserver_state.str_buf, extended_qsupported_features);
+}
+
gdb_put_strbuf();
}
@@ -1729,6 +1748,41 @@ static const GdbCmdParseEntry
gdb_gen_query_set_common_table[] = {
},
};
+/* Compares if a set of command parsers is equal to another set of parsers. */
+static bool cmp_cmds(GdbCmdParseEntry *c, GdbCmdParseEntry *d, int size)
+{
+for (int i = 0; i < size; i++) {
+if (!(c[i].handler == d[i].handler &&
+g_strcmp0(c[i].cmd, d[i].cmd) == 0 &&
+c[i].cmd_startswith == d[i].cmd_startswith &&
+g_strcmp0(c[i].schema, d[i].schema) == 0)) {
+
+/* Sets are different. */
+return false;
+}
+}
+
+/* Sets are equal, i.e. contain the same command parsers. */
+return true;
+}
+
+static GdbCmdParseEntry *extended_query_table;
+static int extended_query_table_size;
+void gdb_extend_query_table(GdbCmdParseEntry *table, int size)
+{
+/*
+ * We don't support different sets of CPU gdb features on different CPUs
yet
+ * so assert query table is the same on all CPUs, or is set only once
+ * (1 CPU).
+ */
+g_assert(extended_query_table == NULL ||
+ (extended_query_table_size == size &&
+ cmp_cmds(extended_query_table, table, size)));
+
+extended_query_table = table;
+extended_query_table_size = size;
+}
+
static const GdbCmdParseEntry gdb_gen_query_table[] = {
{
.handler = handle_query_curr_tid,
@@ -1821,6 +1875,22 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
#endif
};
+static GdbCmdParseEntry *extended_set_table;
+static int extended_set_table_size;
+void gdb_extend_set_table(GdbCmdParseEntry *table, int size)
+{
+/*
+ * We don't support different sets of CPU gdb features on different CPUs
yet
+ * so assert set table is the same on all CPUs, or is set only once (1
CPU).
+ */
+g_assert(extended_set_table == NULL ||
+ (extended_set_table_size == size &&
+ cmp_cmds(extended_set_table, table, size)));
+
+extended_set_table = table;
+extended_set_table_size = size;
+}
+
static const GdbCmdParseEntry gdb_gen_set_table[] = {
/* Order is important if has same prefix */
{
@@ -1859,11 +1929,21 @@ static void handle_gen_query(GArray *params, void
*user_ctx)
return;
}
-if (!process_string_cmd(gdb_get_cmd_param(params, 0)->data,
-gdb_gen_query_table,
-ARRAY_SIZE(gdb_gen_query_table))) {
-gdb_put_packet("");
+if (process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ gdb_gen_query_table,
+ ARRAY_SIZE(gdb_gen_query_table))) {
+return;
+}
+
+if (extended_query_table &&
+process_string_cmd(gdb_get_cmd_param(params, 0)->data,
+ extended_query_table,
+ extended_query_table_size)) {
+return;
}
+
+/* Can't handle query, return Empty response. */
+gdb_put_packet("");
}
static void handle_gen_set(GArray *params, void *user_ctx)
@@ -1878,11 +1958,21 @@ static void handle_gen_set(GArray *params, void
*user_ctx)
[PATCH v6 05/11] target/arm: Make some MTE helpers widely available
Make the MTE helpers allocation_tag_mem_probe, load_tag1, and store_tag1
available to other subsystems.
Signed-off-by: Gustavo Romero
Reviewed-by: Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé
---
target/arm/tcg/mte_helper.c | 45 -
target/arm/tcg/mte_helper.h | 66 +
2 files changed, 73 insertions(+), 38 deletions(-)
create mode 100644 target/arm/tcg/mte_helper.h
diff --git a/target/arm/tcg/mte_helper.c b/target/arm/tcg/mte_helper.c
index a50d576294..9d2ba287ee 100644
--- a/target/arm/tcg/mte_helper.c
+++ b/target/arm/tcg/mte_helper.c
@@ -29,6 +29,7 @@
#include "hw/core/tcg-cpu-ops.h"
#include "qapi/error.h"
#include "qemu/guest-random.h"
+#include "mte_helper.h"
static int choose_nonexcluded_tag(int tag, int offset, uint16_t exclude)
@@ -50,42 +51,10 @@ static int choose_nonexcluded_tag(int tag, int offset,
uint16_t exclude)
return tag;
}
-/**
- * allocation_tag_mem_probe:
- * @env: the cpu environment
- * @ptr_mmu_idx: the addressing regime to use for the virtual address
- * @ptr: the virtual address for which to look up tag memory
- * @ptr_access: the access to use for the virtual address
- * @ptr_size: the number of bytes in the normal memory access
- * @tag_access: the access to use for the tag memory
- * @probe: true to merely probe, never taking an exception
- * @ra: the return address for exception handling
- *
- * Our tag memory is formatted as a sequence of little-endian nibbles.
- * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
- * tags, with the tag at [3:0] for the lower addr and the tag at [7:4]
- * for the higher addr.
- *
- * Here, resolve the physical address from the virtual address, and return
- * a pointer to the corresponding tag byte.
- *
- * If there is no tag storage corresponding to @ptr, return NULL.
- *
- * If the page is inaccessible for @ptr_access, or has a watchpoint, there are
- * three options:
- * (1) probe = true, ra = 0 : pure probe -- we return NULL if the page is not
- * accessible, and do not take watchpoint traps. The calling code must
- * handle those cases in the right priority compared to MTE traps.
- * (2) probe = false, ra = 0 : probe, no fault expected -- the caller
guarantees
- * that the page is going to be accessible. We will take watchpoint traps.
- * (3) probe = false, ra != 0 : non-probe -- we will take both memory access
- * traps and watchpoint traps.
- * (probe = true, ra != 0 is invalid and will assert.)
- */
-static uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
- uint64_t ptr, MMUAccessType
ptr_access,
- int ptr_size, MMUAccessType
tag_access,
- bool probe, uintptr_t ra)
+uint8_t *allocation_tag_mem_probe(CPUARMState *env, int ptr_mmu_idx,
+ uint64_t ptr, MMUAccessType ptr_access,
+ int ptr_size, MMUAccessType tag_access,
+ bool probe, uintptr_t ra)
{
#ifdef CONFIG_USER_ONLY
uint64_t clean_ptr = useronly_clean_ptr(ptr);
@@ -287,7 +256,7 @@ uint64_t HELPER(addsubg)(CPUARMState *env, uint64_t ptr,
return address_with_allocation_tag(ptr + offset, rtag);
}
-static int load_tag1(uint64_t ptr, uint8_t *mem)
+int load_tag1(uint64_t ptr, uint8_t *mem)
{
int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
return extract32(*mem, ofs, 4);
@@ -321,7 +290,7 @@ static void check_tag_aligned(CPUARMState *env, uint64_t
ptr, uintptr_t ra)
}
/* For use in a non-parallel context, store to the given nibble. */
-static void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
+void store_tag1(uint64_t ptr, uint8_t *mem, int tag)
{
int ofs = extract32(ptr, LOG2_TAG_GRANULE, 1) * 4;
*mem = deposit32(*mem, ofs, 4, tag);
diff --git a/target/arm/tcg/mte_helper.h b/target/arm/tcg/mte_helper.h
new file mode 100644
index 00..1f471fb69b
--- /dev/null
+++ b/target/arm/tcg/mte_helper.h
@@ -0,0 +1,66 @@
+/*
+ * ARM MemTag operation helpers.
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ *
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#ifndef TARGET_ARM_MTE_H
+#define TARGET_ARM_MTE_H
+
+#include "exec/mmu-access-type.h"
+
+/**
+ * allocation_tag_mem_probe:
+ * @env: the cpu environment
+ * @ptr_mmu_idx: the addressing regime to use for the virtual address
+ * @ptr: the virtual address for which to look up tag memory
+ * @ptr_access: the access to use for the virtual address
+ * @ptr_size: the number of bytes in the normal memory access
+ * @tag_access: the access to use for the tag memory
+ * @probe: true to merely probe, never taking an exception
+ * @ra: the return address for exception handling
+ *
+ * Our tag memory is formatted as a sequence of little-endian nibbles.
+ * That is, the byte at (addr >> (LOG2_TAG_GRANULE + 1)) contains two
+ *
[PATCH v6 10/11] gdbstub: Add support for MTE in user mode
This commit implements the stubs to handle the qIsAddressTagged,
qMemTag, and QMemTag GDB packets, allowing all GDB 'memory-tag'
subcommands to work with QEMU gdbstub on aarch64 user mode. It also
implements the get/set functions for the special GDB MTE register
'tag_ctl', used to control the MTE fault type at runtime.
Signed-off-by: Gustavo Romero
---
configs/targets/aarch64-linux-user.mak | 2 +-
gdb-xml/aarch64-mte.xml| 11 ++
target/arm/cpu.c | 1 +
target/arm/gdbstub.c | 46 +
target/arm/gdbstub64.c | 223 +
target/arm/internals.h | 6 +
6 files changed, 288 insertions(+), 1 deletion(-)
create mode 100644 gdb-xml/aarch64-mte.xml
diff --git a/configs/targets/aarch64-linux-user.mak
b/configs/targets/aarch64-linux-user.mak
index ba8bc5fe3f..8f0ed21d76 100644
--- a/configs/targets/aarch64-linux-user.mak
+++ b/configs/targets/aarch64-linux-user.mak
@@ -1,6 +1,6 @@
TARGET_ARCH=aarch64
TARGET_BASE_ARCH=arm
-TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml
gdb-xml/aarch64-pauth.xml
+TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml
gdb-xml/aarch64-pauth.xml gdb-xml/aarch64-mte.xml
TARGET_HAS_BFLT=y
CONFIG_SEMIHOSTING=y
CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
diff --git a/gdb-xml/aarch64-mte.xml b/gdb-xml/aarch64-mte.xml
new file mode 100644
index 00..4b70b4f17a
--- /dev/null
+++ b/gdb-xml/aarch64-mte.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b..14d4eca127 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2518,6 +2518,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
register_cp_regs_for_features(cpu);
arm_cpu_register_gdb_regs_for_features(cpu);
+arm_cpu_register_gdb_commands(cpu);
init_cpreg_list(cpu);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index a3bb73cfa7..c3a9b5eb1e 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -21,6 +21,7 @@
#include "cpu.h"
#include "exec/gdbstub.h"
#include "gdbstub/helpers.h"
+#include "gdbstub/commands.h"
#include "sysemu/tcg.h"
#include "internals.h"
#include "cpu-features.h"
@@ -474,6 +475,41 @@ static GDBFeature
*arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
#endif
#endif /* CONFIG_TCG */
+void arm_cpu_register_gdb_commands(ARMCPU *cpu)
+{
+GArray *query_table =
+g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+GArray *set_table =
+g_array_new(FALSE, FALSE, sizeof(GdbCmdParseEntry));
+GString *qsupported_features = g_string_new(NULL);
+
+if (arm_feature(>env, ARM_FEATURE_AARCH64)) {
+#ifdef TARGET_AARCH64
+aarch64_cpu_register_gdb_commands(cpu, qsupported_features,
query_table,
+ set_table);
+#endif
+}
+
+/* Set arch-specific handlers for 'q' commands. */
+if (query_table->len) {
+gdb_extend_query_table(_array_index(query_table,
+ GdbCmdParseEntry, 0),
+ query_table->len);
+}
+
+/* Set arch-specific handlers for 'Q' commands. */
+if (set_table->len) {
+gdb_extend_set_table(_array_index(set_table,
+ GdbCmdParseEntry, 0),
+ set_table->len);
+}
+
+/* Set arch-specific qSupported feature. */
+if (qsupported_features->len) {
+gdb_extend_qsupported_features(qsupported_features->str);
+}
+}
+
void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
{
CPUState *cs = CPU(cpu);
@@ -507,6 +543,16 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
gdb_find_static_feature("aarch64-pauth.xml"),
0);
}
+
+#ifdef CONFIG_USER_ONLY
+/* Memory Tagging Extension (MTE) 'tag_ctl' pseudo-register. */
+if (cpu_isar_feature(aa64_mte, cpu)) {
+gdb_register_coprocessor(cs, aarch64_gdb_get_tag_ctl_reg,
+ aarch64_gdb_set_tag_ctl_reg,
+
gdb_find_static_feature("aarch64-mte.xml"),
+ 0);
+}
+#endif
#endif
} else {
if (arm_feature(env, ARM_FEATURE_NEON)) {
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index caa31ff3fa..2e2bc2700b 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -21,6 +21,12 @@
#include "cpu.h"
#include "internals.h"
#include "gdbstub/helpers.h"
+#include "gdbstub/commands.h"
+#include "tcg/mte_helper.h"
+#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
+#include
+#include "mte_user_helper.h"
+#endif
int aarch64_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
@@ -381,3 +387,220 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs,
int
[PATCH v6 02/11] gdbstub: Move GdbCmdParseEntry into a new header file
Move GdbCmdParseEntry and its associated types into a separate header
file to allow the use of GdbCmdParseEntry and other gdbstub command
functions outside of gdbstub.c.
Since GdbCmdParseEntry and get_param are now public, kdoc
GdbCmdParseEntry and rename get_param to gdb_get_cmd_param.
This commit also makes gdb_put_packet public since is used in gdbstub
command handling.
Signed-off-by: Gustavo Romero
Reviewed-by: Alex Bennée
---
gdbstub/gdbstub.c | 134 ++---
gdbstub/internals.h| 22 --
gdbstub/syscalls.c | 7 +-
gdbstub/system.c | 7 +-
gdbstub/user-target.c | 25 +++
gdbstub/user.c | 7 +-
include/gdbstub/commands.h | 72
7 files changed, 146 insertions(+), 128 deletions(-)
create mode 100644 include/gdbstub/commands.h
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 37314b92e5..9ff2f4177d 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -30,6 +30,7 @@
#include "qemu/error-report.h"
#include "trace.h"
#include "exec/gdbstub.h"
+#include "gdbstub/commands.h"
#include "gdbstub/syscalls.h"
#ifdef CONFIG_USER_ONLY
#include "accel/tcg/vcpu-state.h"
@@ -920,43 +921,6 @@ static int cmd_parse_params(const char *data, const char
*schema,
return 0;
}
-typedef void (*GdbCmdHandler)(GArray *params, void *user_ctx);
-
-/*
- * cmd_startswith -> cmd is compared using startswith
- *
- * allow_stop_reply -> true iff the gdbstub can respond to this command with a
- * "stop reply" packet. The list of commands that accept such response is
- * defined at the GDB Remote Serial Protocol documentation. see:
- *
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
- *
- * schema definitions:
- * Each schema parameter entry consists of 2 chars,
- * the first char represents the parameter type handling
- * the second char represents the delimiter for the next parameter
- *
- * Currently supported schema types:
- * 'l' -> unsigned long (stored in .val_ul)
- * 'L' -> unsigned long long (stored in .val_ull)
- * 's' -> string (stored in .data)
- * 'o' -> single char (stored in .opcode)
- * 't' -> thread id (stored in .thread_id)
- * '?' -> skip according to delimiter
- *
- * Currently supported delimiters:
- * '?' -> Stop at any delimiter (",;:=\0")
- * '0' -> Stop at "\0"
- * '.' -> Skip 1 char unless reached "\0"
- * Any other value is treated as the delimiter value itself
- */
-typedef struct GdbCmdParseEntry {
-GdbCmdHandler handler;
-const char *cmd;
-bool cmd_startswith;
-const char *schema;
-bool allow_stop_reply;
-} GdbCmdParseEntry;
-
static inline int startswith(const char *string, const char *pattern)
{
return !strncmp(string, pattern, strlen(pattern));
@@ -1023,7 +987,7 @@ static void handle_detach(GArray *params, void *user_ctx)
return;
}
-pid = get_param(params, 0)->val_ul;
+pid = gdb_get_cmd_param(params, 0)->val_ul;
}
#ifdef CONFIG_USER_ONLY
@@ -1061,13 +1025,13 @@ static void handle_thread_alive(GArray *params, void
*user_ctx)
return;
}
-if (get_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
+if (gdb_get_cmd_param(params, 0)->thread_id.kind == GDB_READ_THREAD_ERR) {
gdb_put_packet("E22");
return;
}
-cpu = gdb_get_cpu(get_param(params, 0)->thread_id.pid,
- get_param(params, 0)->thread_id.tid);
+cpu = gdb_get_cpu(gdb_get_cmd_param(params, 0)->thread_id.pid,
+ gdb_get_cmd_param(params, 0)->thread_id.tid);
if (!cpu) {
gdb_put_packet("E22");
return;
@@ -1079,7 +1043,7 @@ static void handle_thread_alive(GArray *params, void
*user_ctx)
static void handle_continue(GArray *params, void *user_ctx)
{
if (params->len) {
-gdb_set_cpu_pc(get_param(params, 0)->val_ull);
+gdb_set_cpu_pc(gdb_get_cmd_param(params, 0)->val_ull);
}
gdbserver_state.signal = 0;
@@ -1095,7 +1059,7 @@ static void handle_cont_with_sig(GArray *params, void
*user_ctx)
* omit the addr parameter
*/
if (params->len) {
-signal = get_param(params, 0)->val_ul;
+signal = gdb_get_cmd_param(params, 0)->val_ul;
}
gdbserver_state.signal = gdb_signal_to_target(signal);
@@ -1115,18 +1079,18 @@ static void handle_set_thread(GArray *params, void
*user_ctx)
return;
}
-if (get_param(params, 1)->thread_id.kind == GDB_READ_THREAD_ERR) {
+if (gdb_get_cmd_param(params, 1)->thread_id.kind == GDB_READ_THREAD_ERR) {
gdb_put_packet("E22");
return;
}
-if (get_param(params, 1)->thread_id.kind != GDB_ONE_THREAD) {
+if (gdb_get_cmd_param(params, 1)->thread_id.kind != GDB_ONE_THREAD) {
gdb_put_packet("OK");
return;
}
-pid = get_param(params, 1)->thread_id.pid;
-tid = get_param(params, 1)->thread_id.tid;
+
Re: [PATCH v5 6/9] target/arm: Factor out code for setting MTE TCF0 field
Hi Phil, On 6/27/24 3:05 AM, Philippe Mathieu-Daudé wrote: On 27/6/24 06:13, Gustavo Romero wrote: Factor out the code used for setting the MTE TCF0 field from the prctl code into a convenient function. Other subsystems, like gdbstub, need to set this field as well, so keep it as a separate function to avoid duplication and ensure consistency in how this field is set across the board. Signed-off-by: Gustavo Romero --- linux-user/aarch64/meson.build | 2 ++ linux-user/aarch64/mte_user_helper.c | 34 linux-user/aarch64/mte_user_helper.h | 25 linux-user/aarch64/target_prctl.h | 22 ++ 4 files changed, 63 insertions(+), 20 deletions(-) create mode 100644 linux-user/aarch64/mte_user_helper.c create mode 100644 linux-user/aarch64/mte_user_helper.h diff --git a/linux-user/aarch64/mte_user_helper.h b/linux-user/aarch64/mte_user_helper.h new file mode 100644 index 00..ee3f6b190a --- /dev/null +++ b/linux-user/aarch64/mte_user_helper.h @@ -0,0 +1,25 @@ +/* + * ARM MemTag convenience functions. + * + * This code is licensed under the GNU GPL v2 or later. + * + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#ifndef AARCH64_MTE_USER_HELPER_H +#define AARCH64_MTE USER_HELPER_H + +#include "qemu/osdep.h" https://www.qemu.org/docs/master/devel/style.html#include-directives Do not include “qemu/osdep.h” from header files since the .c file will have already included it. +#include "qemu.h" "qemu.h" shouldn't be required neither. If I remove qemu/osdep.h CPUArchState can't resolved. If I remove qemu.h then abi_long can't be resolved. I'm in a tight corner here. Cheers, Gustavo Conditional to removing both lines: Reviewed-by: Philippe Mathieu-Daudé
[PATCH v6 01/11] gdbstub: Clean up process_string_cmd
Change 'process_string_cmd' to return true on success and false on
failure, instead of 0 and -1.
Signed-off-by: Gustavo Romero
Reviewed-by: Alex Bennée
Reviewed-by: Richard Henderson
---
gdbstub/gdbstub.c | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b3574997ea..37314b92e5 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -962,14 +962,14 @@ static inline int startswith(const char *string, const
char *pattern)
return !strncmp(string, pattern, strlen(pattern));
}
-static int process_string_cmd(const char *data,
- const GdbCmdParseEntry *cmds, int num_cmds)
+static bool process_string_cmd(const char *data,
+ const GdbCmdParseEntry *cmds, int num_cmds)
{
int i;
g_autoptr(GArray) params = g_array_new(false, true, sizeof(GdbCmdVariant));
if (!cmds) {
-return -1;
+return false;
}
for (i = 0; i < num_cmds; i++) {
@@ -984,16 +984,16 @@ static int process_string_cmd(const char *data,
if (cmd->schema) {
if (cmd_parse_params([strlen(cmd->cmd)],
cmd->schema, params)) {
-return -1;
+return false;
}
}
gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, NULL);
-return 0;
+return true;
}
-return -1;
+return false;
}
static void run_cmd_parser(const char *data, const GdbCmdParseEntry *cmd)
@@ -1007,7 +1007,7 @@ static void run_cmd_parser(const char *data, const
GdbCmdParseEntry *cmd)
/* In case there was an error during the command parsing we must
* send a NULL packet to indicate the command is not supported */
-if (process_string_cmd(data, cmd, 1)) {
+if (!process_string_cmd(data, cmd, 1)) {
gdb_put_packet("");
}
}
@@ -1523,9 +1523,9 @@ static void handle_v_commands(GArray *params, void
*user_ctx)
return;
}
-if (process_string_cmd(get_param(params, 0)->data,
- gdb_v_commands_table,
- ARRAY_SIZE(gdb_v_commands_table))) {
+if (!process_string_cmd(get_param(params, 0)->data,
+gdb_v_commands_table,
+ARRAY_SIZE(gdb_v_commands_table))) {
gdb_put_packet("");
}
}
@@ -1889,15 +1889,15 @@ static void handle_gen_query(GArray *params, void
*user_ctx)
return;
}
-if (!process_string_cmd(get_param(params, 0)->data,
-gdb_gen_query_set_common_table,
-ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+if (process_string_cmd(get_param(params, 0)->data,
+ gdb_gen_query_set_common_table,
+ ARRAY_SIZE(gdb_gen_query_set_common_table))) {
return;
}
-if (process_string_cmd(get_param(params, 0)->data,
- gdb_gen_query_table,
- ARRAY_SIZE(gdb_gen_query_table))) {
+if (!process_string_cmd(get_param(params, 0)->data,
+gdb_gen_query_table,
+ARRAY_SIZE(gdb_gen_query_table))) {
gdb_put_packet("");
}
}
@@ -1908,13 +1908,13 @@ static void handle_gen_set(GArray *params, void
*user_ctx)
return;
}
-if (!process_string_cmd(get_param(params, 0)->data,
-gdb_gen_query_set_common_table,
-ARRAY_SIZE(gdb_gen_query_set_common_table))) {
+if (process_string_cmd(get_param(params, 0)->data,
+ gdb_gen_query_set_common_table,
+ ARRAY_SIZE(gdb_gen_query_set_common_table))) {
return;
}
-if (process_string_cmd(get_param(params, 0)->data,
+if (!process_string_cmd(get_param(params, 0)->data,
gdb_gen_set_table,
ARRAY_SIZE(gdb_gen_set_table))) {
gdb_put_packet("");
--
2.34.1
[PATCH v6 00/11] Add MTE stubs for aarch64 user mode
This patchset adds the stubs necessary to support GDB memory tagging
commands on QEMU aarch64 user mode.
These new stubs handle the qIsAddressTagged, qMemTag, and QMemTag
packets, which allow GDB memory tagging subcommands 'check',
'print-allocation-tag', and 'set-allocation-tag' to work. The remaining
memory tagging commands ('print-logical-tag' and 'with-logical-tag')
will also work, but they don't rely on any stub because they perform
local operations.
Since the memory tagging stubs are not common to all architectures, this
patchset also introduces three functions: gdb_extend_qsupported_features,
gdb_extend_query_table, and gdb_extend_set_table. These functions can be
used to extend the target-specific 'qSupported' feature string and the
handlers for the 'q' (query) and 'Q' (set) packets. These new functions
are used to add the MTE stubs for the aarch64 gdbstub.
Note that this patchset requires a GDB that supports the
qIsAddressTagged packet (recently added to GDB), so the gdbstub MTE
tests introduced by it must be run using GDB's master branch, since the
GDB in the distros hasn't picked up the change yet.
Once GDB is built and installed locally, the tests can be exercised,
this way:
$ make run-tcg-tests-aarch64-linux-user -j 32
after running QEMU's configure script with the --gdb=
option to specify the proper GDB binary.
v2:
- Addressed comments from Richard, Phil, and Alex
- Made the series more granular by splitting it into more patches
- Moved gdbstub command-specific structs and functions into a new header,
gdbstub/commands.h
- Fixed exception in allocation_tag_mem_probe()
- Used MTE helpers ({store,load}_tag1 and allocation_tag_mem_probe) in the MTE
stubs
- Factored out MTE code to set TCF0, avoiding duplication (both prctl and
gdbstub code use it)
- Hoisted sscanf() out of loop in handle_Q_memtag stub and use gdb_hextomem
instead
- Rebased this series on Alex's gdb/next branch
v3:
- Moved stubs to gdbstub64.c
- Fixed build for BSD target
- Fixed license tags in the new header files
- Use of only function prototypes in mte_helpers.h
- Added prefix to arm_set_mte_tcf0 and marked it inline
- Moved target/arm/mte.h -> target/arm/tcg/mte_user_helper.h
- Cleaned up leftover in mte-8 test
v4:
- Don't run tests when GDB < 15 (i.e., lacks most recent MTE changes) (Alex's
review)
- Added g_assert()s in new gdb_extend_* API functions to ensure all CPUs have
the same features/query/set tables (Alex's review)
- Move load_tag1 and store_tag1 to mte_helpers.h marking them as "static
inline" (Richard's review)
- Move mte_user_helper.h to linux-user/aarch64/ and include it from
gdbstub64.c (Richard's review)
v5:
- Made load_tag1, store_tag1, and arm_set_mte_tcf0 non-inlined for code
modularity (Phil's review)
- MTE tests: GDB_HAS_MTE is now only set for aarch64 targets (Alex's
suggestion)
v6:
- Remove dead code (Phil's review)
- Pass CPU context via user_ctx (Phil and Alex's review)
- Cleanup: in gdbstub.c use 'true' to set startswith instead of 1
Cheers,
Gustavo
Gustavo Romero (11):
gdbstub: Clean up process_string_cmd
gdbstub: Move GdbCmdParseEntry into a new header file
gdbstub: Add support for target-specific stubs
target/arm: Fix exception case in allocation_tag_mem_probe
target/arm: Make some MTE helpers widely available
target/arm: Factor out code for setting MTE TCF0 field
gdbstub: Make hex conversion function non-internal
gdbstub: Pass CPU context to command handler
gdbstub: Use true to set cmd_startswith
gdbstub: Add support for MTE in user mode
tests/tcg/aarch64: Add MTE gdbstub tests
configs/targets/aarch64-linux-user.mak | 2 +-
configure | 4 +
gdb-xml/aarch64-mte.xml| 11 +
gdbstub/gdbstub.c | 341 +++--
gdbstub/internals.h| 23 --
gdbstub/syscalls.c | 7 +-
gdbstub/system.c | 7 +-
gdbstub/user-target.c | 25 +-
gdbstub/user.c | 7 +-
include/gdbstub/commands.h | 103
linux-user/aarch64/meson.build | 2 +
linux-user/aarch64/mte_user_helper.c | 34 +++
linux-user/aarch64/mte_user_helper.h | 25 ++
linux-user/aarch64/target_prctl.h | 22 +-
target/arm/cpu.c | 1 +
target/arm/gdbstub.c | 46
target/arm/gdbstub64.c | 223
target/arm/internals.h | 6 +
target/arm/tcg/mte_helper.c| 48 +---
target/arm/tcg/mte_helper.h| 66 +
tests/tcg/aarch64/Makefile.target | 14 +-
tests/tcg/aarch64/gdbstub/test-mte.py | 86 +++
tests/tcg/aarch64/mte-8.c | 98 +++
23 files changed, 956 insertions(+), 245 deletions(-)
create mode 100644 gdb-xml/aarch64-mte.xml
create mode 100644 include/gdbstub/commands.h
create mode 100644
Re: [PATCH v5 7/9] gdbstub: Make get cpu and hex conversion functions non-internal
Hi Phil, Alex,
On 6/27/24 9:26 AM, Philippe Mathieu-Daudé wrote:
On 27/6/24 13:05, Alex Bennée wrote:
Philippe Mathieu-Daudé writes:
On 27/6/24 06:13, Gustavo Romero wrote:
Make the gdb_first_attached_cpu and gdb_hextomem non-internal so they
are not confined to use only in gdbstub.c.
Signed-off-by: Gustavo Romero
Reviewed-by: Richard Henderson
---
gdbstub/internals.h | 2 --
include/exec/gdbstub.h | 5 +
include/gdbstub/commands.h | 6 ++
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 1bd2c4ec2a..77e5ec9a5b 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -135,4 +135,9 @@ void gdb_set_stop_cpu(CPUState *cpu);
/* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
extern const GDBFeature gdb_static_features[];
+/**
+ * Return the first attached CPU
+ */
+CPUState *gdb_first_attached_cpu(void);
Alex, it seems dubious to expose the API like that.
IMHO GdbCmdHandler should take a GDBRegisterState argument,
then this would become:
CPUState *gdb_first_attached_cpu(GDBRegisterState *s);
Maybe instead of exposing this we can use user_ctx for something? If we
look at handle_set_reg/handle_get_reg we can see that passes down
gdbserver_state.g_cpu down to the eventual helpers. We could define
something like:
--8<---cut here---start->8---
fixups to avoid get_first_cpu()
5 files changed, 25 insertions(+), 18 deletions(-)
gdbstub/internals.h | 1 +
include/exec/gdbstub.h | 5 -
include/gdbstub/commands.h | 3 +++
gdbstub/gdbstub.c | 7 ++-
target/arm/gdbstub64.c | 27 +++
@@ -54,6 +54,8 @@ typedef union GdbCmdVariant {
* "stop reply" packet. The list of commands that accept such response is
* defined at the GDB Remote Serial Protocol documentation. See:
*
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
+ *
+ * @need_cpu_context: pass current CPU to command via user_ctx.
*/
typedef struct GdbCmdParseEntry {
GdbCmdHandler handler;
@@ -61,6 +63,7 @@ typedef struct GdbCmdParseEntry {
bool cmd_startswith;
const char *schema;
bool allow_stop_reply;
+ bool need_cpu_context;
} GdbCmdParseEntry;
#define get_cmd_parsers(p) (_array_index(p, GdbCmdParseEntry, 0))
modified gdbstub/gdbstub.c
@@ -938,6 +938,7 @@ static bool process_string_cmd(const char *data,
for (i = 0; i < num_cmds; i++) {
const GdbCmdParseEntry *cmd = [i];
+ void *user_ctx = NULL;
g_assert(cmd->handler && cmd->cmd);
if ((cmd->cmd_startswith && !startswith(data, cmd->cmd)) ||
@@ -952,8 +953,12 @@ static bool process_string_cmd(const char *data,
}
}
+ if (cmd->need_cpu_context) {
+ user_ctx = (void *) gdbserver_state.g_cpu;
LGTM.
Thanks for the suggestion. I added it to v6.
Cheers,
Gustavo
Re: [PATCH v5 2/9] gdbstub: Move GdbCmdParseEntry into a new header file
Hi Phil, On 6/27/24 3:02 AM, Philippe Mathieu-Daudé wrote: On 27/6/24 06:13, Gustavo Romero wrote: Move GdbCmdParseEntry and its associated types into a separate header file to allow the use of GdbCmdParseEntry and other gdbstub command functions outside of gdbstub.c. Since GdbCmdParseEntry and get_param are now public, kdoc GdbCmdParseEntry and rename get_param to gdb_get_cmd_param. This commit also makes gdb_put_packet public since is used in gdbstub command handling. Signed-off-by: Gustavo Romero Reviewed-by: Alex Bennée --- gdbstub/gdbstub.c | 134 ++--- gdbstub/internals.h | 22 -- gdbstub/syscalls.c | 7 +- gdbstub/system.c | 7 +- gdbstub/user-target.c | 25 +++ gdbstub/user.c | 7 +- include/gdbstub/commands.h | 74 7 files changed, 148 insertions(+), 128 deletions(-) create mode 100644 include/gdbstub/commands.h +#define get_cmd_parsers(p) (_array_index(p, GdbCmdParseEntry, 0)) Dead code AFAICT. Yes, that's a leftover. Removed in v6. Thanks. Cheers, Gustavo
Re: [PATCH] system/physmem: Fix reference to dump-guest-core
On 28/6/24 06:05, Akihiko Odaki wrote: dump_guest_core is exposed as dump-guest-core with QOM. Signed-off-by: Akihiko Odaki --- system/physmem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH 09/12] hw/sd/sdcard: Remove noise from sd_acmd_name()
These ACMD names weren't really useful, "UNKNOWN_ACMD" is simpler.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 12 +---
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7f93d363c7..19322c558f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -259,23 +259,13 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
{
-static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
-[14] = "DPS_spec", [15] = "DPS_spec",
-[16] = "DPS_spec",
-[18] = "SECU_spec",
-[52] = "SECU_spec", [53] = "SECU_spec",
-[54] = "SECU_spec",
-[56] = "SECU_spec", [57] = "SECU_spec",
-[58] = "SECU_spec", [59] = "SECU_spec",
-};
const SDProto *sdp = sd->proto;
if (sdp->acmd[cmd].handler) {
-assert(!acmd_abbrev[cmd]);
return sdp->acmd[cmd].name;
}
-return acmd_abbrev[cmd] ? acmd_abbrev[cmd] : "UNKNOWN_ACMD";
+return "UNKNOWN_ACMD";
}
static uint8_t sd_get_dat_lines(SDState *sd)
--
2.41.0
[PATCH 10/12] hw/sd/sdcard: Remove noise from sd_cmd_name()
These CMD names weren't really useful, "UNKNOWN_CMD" is simpler.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 19322c558f..0a7b422b2c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -241,12 +241,7 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
{
static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
[18]= "READ_MULTIPLE_BLOCK",
-[21]= "DPS_spec",
[25]= "WRITE_MULTIPLE_BLOCK",
-[26]= "MANUF_RSVD",
-[40]= "DPS_spec",
-[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
-[62]= "MANUF_RSVD", [63]= "MANUF_RSVD",
};
const SDProto *sdp = sd->proto;
--
2.41.0
[PATCH 12/12] hw/sd/sdcard: Trace length of data read on DAT lines
Some commands expect less than BLOCK_LENGTH.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 4 ++--
hw/sd/trace-events | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 64621d4340..a0da06e017 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2197,8 +2197,8 @@ uint8_t sd_read_byte(SDState *sd)
io_len = sd_blk_len(sd);
trace_sdcard_read_data(sd->proto->name,
- sd->last_cmd_name,
- sd->current_cmd, sd->data_offset, io_len);
+ sd->last_cmd_name, sd->current_cmd,
+ sd->data_offset, sd->data_size, io_len);
switch (sd->current_cmd) {
case 6: /* CMD6: SWITCH_FUNCTION */
case 9: /* CMD9: SEND_CSD */
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 6a51b0e906..5dfe6be7b7 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -55,7 +55,7 @@ sdcard_req_addr(uint32_t req_arg, uint64_t addr) "req 0x%"
PRIx32 " addr 0x%" PR
sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd,
uint32_t offset, uint8_t value) "%s %20s/ CMD%02d ofs %"PRIu32" value 0x%02x"
-sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd,
uint32_t offset, uint32_t length) "%s %20s/ CMD%02d ofs %"PRIu32" len %" PRIu32
+sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd,
uint32_t offset, uint64_t size, uint32_t blklen) "%s %20s/ CMD%02d ofs
%"PRIu32" size %"PRIu64" blklen %" PRIu32
sdcard_set_voltage(uint16_t millivolts) "%u mV"
# pxa2xx_mmci.c
--
2.41.0
[PATCH 08/12] hw/sd/sdcard: Remove sd_none enum from sd_cmd_type_t
All handlers using the 'sd_none' enum got converted,
remove it.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/sd/sd.h | 1 -
hw/sd/sd.c | 7 +--
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 29c76935a0..c1a35ab420 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -76,7 +76,6 @@ typedef enum {
} sd_uhs_mode_t;
typedef enum {
-sd_none = 0,
sd_spi,
sd_bc, /* broadcast -- no response */
sd_bcr,/* broadcast with response */
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6a9d611429..7f93d363c7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -526,17 +526,12 @@ static void sd_set_rca(SDState *sd, uint16_t value)
static uint16_t sd_req_get_rca(SDState *s, SDRequest req)
{
switch (s->proto->cmd[req.cmd].type) {
-case sd_none:
-/* Called from legacy code not ported to SDProto array */
-assert(!s->proto->cmd[req.cmd].handler);
-/* fall-through */
case sd_ac:
case sd_adtc:
return req.arg >> 16;
case sd_spi:
-g_assert_not_reached();
default:
-return 0;
+g_assert_not_reached();
}
}
--
2.41.0
[PATCH 07/12] hw/sd/sdcard: Add sd_acmd_SEND_SCR handler (ACMD51)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a27a7e0f24..6a9d611429 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -263,7 +263,6 @@ static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
[18] = "SECU_spec",
-[51] = "SEND_SCR",
[52] = "SECU_spec", [53] = "SECU_spec",
[54] = "SECU_spec",
[56] = "SECU_spec", [57] = "SECU_spec",
@@ -1758,6 +1757,12 @@ static sd_rsp_type_t sd_acmd_SET_CLR_CARD_DETECT(SDState
*sd, SDRequest req)
return sd_r1;
}
+/* ACMD51 */
+static sd_rsp_type_t sd_acmd_SEND_SCR(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_sendingdata(sd, req, 0, sd->scr, sizeof(sd->scr));
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1866,16 +1871,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 51: /* ACMD51: SEND_SCR */
-switch (sd->state) {
-case sd_transfer_state:
-return sd_cmd_to_sendingdata(sd, req, 0, sd->scr, sizeof(sd->scr));
-
-default:
-break;
-}
-break;
-
case 18:/* Reserved for SD security applications */
case 25:
case 26:
@@ -2327,6 +2322,7 @@ static const SDProto sd_proto_spi = {
[23] = {8, sd_spi, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
[42] = {8, sd_spi, "SET_CLR_CARD_DETECT",
sd_acmd_SET_CLR_CARD_DETECT},
+[51] = {8, sd_spi, "SEND_SCR", sd_acmd_SEND_SCR},
},
};
@@ -2388,6 +2384,7 @@ static const SDProto sd_proto_sd = {
[23] = {8, sd_ac, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
[41] = {8, sd_bcr, "SD_APP_OP_COND", sd_acmd_SD_APP_OP_COND},
[42] = {8, sd_ac, "SET_CLR_CARD_DETECT",
sd_acmd_SET_CLR_CARD_DETECT},
+[51] = {8, sd_adtc, "SEND_SCR", sd_acmd_SEND_SCR},
},
};
--
2.41.0
[PATCH 11/12] hw/sd/sdcard: Remove default case in read/write on DAT lines
All read/write on DAT lines are explicitly handled. Reaching this point would be a programming error: replace by an assertion. Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/sd.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 0a7b422b2c..64621d4340 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -1951,7 +1951,6 @@ int sd_do_command(SDState *sd, SDRequest *req, /* Valid command, we can update the 'state before command' bits. * (Do this now so they appear in r1 responses.) */ -sd->current_cmd = req->cmd; sd->card_status = FIELD_DP32(sd->card_status, CSR, CURRENT_STATE, last_state); } @@ -2016,6 +2015,8 @@ send_response: qemu_hexdump(stderr, "Response", response, rsplen); #endif +sd->current_cmd = rtype == sd_illegal ? 0 : req->cmd; + return rsplen; } @@ -2171,8 +2172,7 @@ void sd_write_byte(SDState *sd, uint8_t value) break; default: -qemu_log_mask(LOG_GUEST_ERROR, "%s: unknown command\n", __func__); -break; +g_assert_not_reached(); } } @@ -2238,8 +2238,7 @@ uint8_t sd_read_byte(SDState *sd) break; default: -qemu_log_mask(LOG_GUEST_ERROR, "%s: unknown command\n", __func__); -return 0x00; +g_assert_not_reached(); } return ret; -- 2.41.0
[PATCH 06/12] hw/sd/sdcard: Add sd_acmd_SET_CLR_CARD_DETECT handler (ACMD42)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 167e1c517a..a27a7e0f24 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -263,7 +263,6 @@ static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
[18] = "SECU_spec",
-[42] = "SET_CLR_CARD_DETECT",
[51] = "SEND_SCR",
[52] = "SECU_spec", [53] = "SECU_spec",
[54] = "SECU_spec",
@@ -1748,6 +1747,17 @@ static sd_rsp_type_t sd_acmd_SD_APP_OP_COND(SDState *sd,
SDRequest req)
return sd_r3;
}
+/* ACMD42 */
+static sd_rsp_type_t sd_acmd_SET_CLR_CARD_DETECT(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+/* Bringing in the 50KOhm pull-up resistor... Done. */
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1856,17 +1866,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 42: /* ACMD42: SET_CLR_CARD_DETECT */
-switch (sd->state) {
-case sd_transfer_state:
-/* Bringing in the 50KOhm pull-up resistor... Done. */
-return sd_r1;
-
-default:
-break;
-}
-break;
-
case 51: /* ACMD51: SEND_SCR */
switch (sd->state) {
case sd_transfer_state:
@@ -2327,6 +2326,7 @@ static const SDProto sd_proto_spi = {
[22] = {8, sd_spi, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
[23] = {8, sd_spi, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
+[42] = {8, sd_spi, "SET_CLR_CARD_DETECT",
sd_acmd_SET_CLR_CARD_DETECT},
},
};
@@ -2387,6 +2387,7 @@ static const SDProto sd_proto_sd = {
[22] = {8, sd_adtc, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
[23] = {8, sd_ac, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
[41] = {8, sd_bcr, "SD_APP_OP_COND", sd_acmd_SD_APP_OP_COND},
+[42] = {8, sd_ac, "SET_CLR_CARD_DETECT",
sd_acmd_SET_CLR_CARD_DETECT},
},
};
--
2.41.0
[PATCH 05/12] hw/sd/sdcard: Add sd_acmd_SD_APP_OP_COND handler (ACMD41)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 82 ++
1 file changed, 45 insertions(+), 37 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index cd207a3090..167e1c517a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1704,6 +1704,50 @@ static sd_rsp_type_t
sd_acmd_SET_WR_BLK_ERASE_COUNT(SDState *sd, SDRequest req)
return sd_r1;
}
+/* ACMD41 */
+static sd_rsp_type_t sd_acmd_SD_APP_OP_COND(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_idle_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+/*
+ * If it's the first ACMD41 since reset, we need to decide
+ * whether to power up. If this is not an enquiry ACMD41,
+ * we immediately report power on and proceed below to the
+ * ready state, but if it is, we set a timer to model a
+ * delay for power up. This works around a bug in EDK2
+ * UEFI, which sends an initial enquiry ACMD41, but
+ * assumes that the card is in ready state as soon as it
+ * sees the power up bit set.
+ */
+if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
+if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
+timer_del(sd->ocr_power_timer);
+sd_ocr_powerup(sd);
+} else {
+trace_sdcard_inquiry_cmd41();
+if (!timer_pending(sd->ocr_power_timer)) {
+timer_mod_ns(sd->ocr_power_timer,
+ (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+ + OCR_POWER_DELAY_NS));
+}
+}
+}
+
+if (FIELD_EX32(sd->ocr & req.arg, OCR, VDD_VOLTAGE_WINDOW)) {
+/*
+ * We accept any voltage. 1 V is nothing.
+ *
+ * Once we're powered up, we advance straight to ready state
+ * unless it's an enquiry ACMD41 (bits 23:0 == 0).
+ */
+sd->state = sd_ready_state;
+}
+
+return sd_r3;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1812,43 +1856,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 41: /* ACMD41: SD_APP_OP_COND */
-if (sd->state != sd_idle_state) {
-break;
-}
-/* If it's the first ACMD41 since reset, we need to decide
- * whether to power up. If this is not an enquiry ACMD41,
- * we immediately report power on and proceed below to the
- * ready state, but if it is, we set a timer to model a
- * delay for power up. This works around a bug in EDK2
- * UEFI, which sends an initial enquiry ACMD41, but
- * assumes that the card is in ready state as soon as it
- * sees the power up bit set. */
-if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
-if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
-timer_del(sd->ocr_power_timer);
-sd_ocr_powerup(sd);
-} else {
-trace_sdcard_inquiry_cmd41();
-if (!timer_pending(sd->ocr_power_timer)) {
-timer_mod_ns(sd->ocr_power_timer,
- (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
- + OCR_POWER_DELAY_NS));
-}
-}
-}
-
-if (FIELD_EX32(sd->ocr & req.arg, OCR, VDD_VOLTAGE_WINDOW)) {
-/* We accept any voltage. 1 V is nothing.
- *
- * Once we're powered up, we advance straight to ready state
- * unless it's an enquiry ACMD41 (bits 23:0 == 0).
- */
-sd->state = sd_ready_state;
-}
-
-return sd_r3;
-
case 42: /* ACMD42: SET_CLR_CARD_DETECT */
switch (sd->state) {
case sd_transfer_state:
@@ -2379,6 +2386,7 @@ static const SDProto sd_proto_sd = {
[13] = {8, sd_adtc, "SD_STATUS", sd_acmd_SD_STATUS},
[22] = {8, sd_adtc, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
[23] = {8, sd_ac, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
+[41] = {8, sd_bcr, "SD_APP_OP_COND", sd_acmd_SD_APP_OP_COND},
},
};
--
2.41.0
[PATCH 04/12] hw/sd/sdcard: Add sd_acmd_SET_WR_BLK_ERASE_COUNT handler (ACMD23)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9d66c3715a..cd207a3090 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -263,7 +263,6 @@ static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
[18] = "SECU_spec",
-[23] = "SET_WR_BLK_ERASE_COUNT",
[42] = "SET_CLR_CARD_DETECT",
[51] = "SEND_SCR",
[52] = "SECU_spec", [53] = "SECU_spec",
@@ -1696,6 +1695,15 @@ static sd_rsp_type_t sd_acmd_SEND_NUM_WR_BLOCKS(SDState
*sd, SDRequest req)
>blk_written, sizeof(sd->blk_written));
}
+/* ACMD23 */
+static sd_rsp_type_t sd_acmd_SET_WR_BLK_ERASE_COUNT(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1804,16 +1812,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 23: /* ACMD23: SET_WR_BLK_ERASE_COUNT */
-switch (sd->state) {
-case sd_transfer_state:
-return sd_r1;
-
-default:
-break;
-}
-break;
-
case 41: /* ACMD41: SD_APP_OP_COND */
if (sd->state != sd_idle_state) {
break;
@@ -2320,6 +2318,7 @@ static const SDProto sd_proto_spi = {
.acmd = {
[13] = {8, sd_spi, "SD_STATUS", sd_acmd_SD_STATUS},
[22] = {8, sd_spi, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
+[23] = {8, sd_spi, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
},
};
@@ -2379,6 +2378,7 @@ static const SDProto sd_proto_sd = {
[6] = {8, sd_ac, "SET_BUS_WIDTH", sd_acmd_SET_BUS_WIDTH},
[13] = {8, sd_adtc, "SD_STATUS", sd_acmd_SD_STATUS},
[22] = {8, sd_adtc, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
+[23] = {8, sd_ac, "SET_WR_BLK_ERASE_COUNT",
sd_acmd_SET_WR_BLK_ERASE_COUNT},
},
};
--
2.41.0
[PATCH 03/12] hw/sd/sdcard: Add sd_acmd_SEND_NUM_WR_BLOCKS handler (ACMD22)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5323a42df2..9d66c3715a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -263,7 +263,7 @@ static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
[18] = "SECU_spec",
-[22] = "SEND_NUM_WR_BLOCKS",[23] = "SET_WR_BLK_ERASE_COUNT",
+[23] = "SET_WR_BLK_ERASE_COUNT",
[42] = "SET_CLR_CARD_DETECT",
[51] = "SEND_SCR",
[52] = "SECU_spec", [53] = "SECU_spec",
@@ -1689,6 +1689,13 @@ static sd_rsp_type_t sd_acmd_SD_STATUS(SDState *sd,
SDRequest req)
sd->sd_status, sizeof(sd->sd_status));
}
+/* ACMD22 */
+static sd_rsp_type_t sd_acmd_SEND_NUM_WR_BLOCKS(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_sendingdata(sd, req, 0,
+ >blk_written, sizeof(sd->blk_written));
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1797,18 +1804,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 22: /* ACMD22: SEND_NUM_WR_BLOCKS */
-switch (sd->state) {
-case sd_transfer_state:
-return sd_cmd_to_sendingdata(sd, req, 0,
- >blk_written,
- sizeof(sd->blk_written));
-
-default:
-break;
-}
-break;
-
case 23: /* ACMD23: SET_WR_BLK_ERASE_COUNT */
switch (sd->state) {
case sd_transfer_state:
@@ -2324,6 +2319,7 @@ static const SDProto sd_proto_spi = {
},
.acmd = {
[13] = {8, sd_spi, "SD_STATUS", sd_acmd_SD_STATUS},
+[22] = {8, sd_spi, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
},
};
@@ -2382,6 +2378,7 @@ static const SDProto sd_proto_sd = {
.acmd = {
[6] = {8, sd_ac, "SET_BUS_WIDTH", sd_acmd_SET_BUS_WIDTH},
[13] = {8, sd_adtc, "SD_STATUS", sd_acmd_SD_STATUS},
+[22] = {8, sd_adtc, "SEND_NUM_WR_BLOCKS", sd_acmd_SEND_NUM_WR_BLOCKS},
},
};
--
2.41.0
[PATCH 02/12] hw/sd/sdcard: Add sd_acmd_SD_STATUS handler (ACMD13)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 22 +-
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0310a5a3a1..5323a42df2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -260,7 +260,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
{
static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
-[13] = "SD_STATUS",
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
[18] = "SECU_spec",
@@ -1683,6 +1682,13 @@ static sd_rsp_type_t sd_acmd_SET_BUS_WIDTH(SDState *sd,
SDRequest req)
return sd_r1;
}
+/* ACMD13 */
+static sd_rsp_type_t sd_acmd_SD_STATUS(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_sendingdata(sd, req, 0,
+ sd->sd_status, sizeof(sd->sd_status));
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1791,18 +1797,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 13: /* ACMD13: SD_STATUS */
-switch (sd->state) {
-case sd_transfer_state:
-return sd_cmd_to_sendingdata(sd, req, 0,
- sd->sd_status,
- sizeof(sd->sd_status));
-
-default:
-break;
-}
-break;
-
case 22: /* ACMD22: SEND_NUM_WR_BLOCKS */
switch (sd->state) {
case sd_transfer_state:
@@ -2329,6 +2323,7 @@ static const SDProto sd_proto_spi = {
[59] = {0, sd_spi, "CRC_ON_OFF", spi_cmd_CRC_ON_OFF},
},
.acmd = {
+[13] = {8, sd_spi, "SD_STATUS", sd_acmd_SD_STATUS},
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
},
};
@@ -2386,6 +2381,7 @@ static const SDProto sd_proto_sd = {
},
.acmd = {
[6] = {8, sd_ac, "SET_BUS_WIDTH", sd_acmd_SET_BUS_WIDTH},
+[13] = {8, sd_adtc, "SD_STATUS", sd_acmd_SD_STATUS},
},
};
--
2.41.0
[PATCH] system/physmem: Fix reference to dump-guest-core
dump_guest_core is exposed as dump-guest-core with QOM.
Signed-off-by: Akihiko Odaki
---
system/physmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/system/physmem.c b/system/physmem.c
index b7847db1a2c5..effa97184f25 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1521,7 +1521,7 @@ static void qemu_ram_setup_dump(void *addr, ram_addr_t
size)
if (ret) {
perror("qemu_madvise");
fprintf(stderr, "madvise doesn't support MADV_DONTDUMP, "
-"but dump_guest_core=off specified\n");
+"but dump-guest-core=off specified\n");
}
}
}
---
base-commit: 046a64b9801343e2e89eef10c7a48eec8d8c0d4f
change-id: 20240614-dump-7b73ab18c31a
Best regards,
--
Akihiko Odaki
[PATCH v2] target/loongarch: Remove avail_64 in trans_srai_w() and simplify it
Since srai.w is a valid instruction on la32, remove the avail_64 check
and simplify trans_srai_w().
Fixes: c0c0461e3a06 ("target/loongarch: Add avail_64 to check la64-only
instructions")
Reviewed-by: Richard Henderson
Signed-off-by: Feiyang Chen
---
target/loongarch/tcg/insn_trans/trans_shift.c.inc | 15 +++
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/target/loongarch/tcg/insn_trans/trans_shift.c.inc
b/target/loongarch/tcg/insn_trans/trans_shift.c.inc
index 2f4bd6ff28..377307785a 100644
--- a/target/loongarch/tcg/insn_trans/trans_shift.c.inc
+++ b/target/loongarch/tcg/insn_trans/trans_shift.c.inc
@@ -67,19 +67,9 @@ static void gen_rotr_d(TCGv dest, TCGv src1, TCGv src2)
tcg_gen_rotr_tl(dest, src1, t0);
}
-static bool trans_srai_w(DisasContext *ctx, arg_srai_w *a)
+static void gen_sari_w(TCGv dest, TCGv src1, target_long imm)
{
-TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
-TCGv src1 = gpr_src(ctx, a->rj, EXT_ZERO);
-
-if (!avail_64(ctx)) {
-return false;
-}
-
-tcg_gen_sextract_tl(dest, src1, a->imm, 32 - a->imm);
-gen_set_gpr(a->rd, dest, EXT_NONE);
-
-return true;
+tcg_gen_sextract_tl(dest, src1, imm, 32 - imm);
}
TRANS(sll_w, ALL, gen_rrr, EXT_ZERO, EXT_NONE, EXT_SIGN, gen_sll_w)
@@ -94,6 +84,7 @@ TRANS(slli_w, ALL, gen_rri_c, EXT_NONE, EXT_SIGN,
tcg_gen_shli_tl)
TRANS(slli_d, 64, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_shli_tl)
TRANS(srli_w, ALL, gen_rri_c, EXT_ZERO, EXT_SIGN, tcg_gen_shri_tl)
TRANS(srli_d, 64, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_shri_tl)
+TRANS(srai_w, ALL, gen_rri_c, EXT_NONE, EXT_NONE, gen_sari_w)
TRANS(srai_d, 64, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_sari_tl)
TRANS(rotri_w, 64, gen_rri_v, EXT_NONE, EXT_NONE, gen_rotr_w)
TRANS(rotri_d, 64, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_rotri_tl)
--
2.34.1
Re: [v2 1/1] hw/i386/acpi-build: add OSHP method support for SHPC driver load
> > > that OS cannot get control of SHPC hotplug and hotplug device to > > > the PCI bridge will fail when we use SHPC Native type: > > > > > > [3.336059] shpchp :00:03.0: Requesting control of SHPC hotplug via > > > OSHP (\_SB_.PCI0.S28_) > > > [3.337408] shpchp :00:03.0: Requesting control of SHPC hotplug via > > > OSHP (\_SB_.PCI0) > > > [3.338710] shpchp :00:03.0: Cannot get control of SHPC hotplug > > > > > > Add OSHP method support for transfer control to the operating system, > > > after this SHPC driver will be loaded success and the hotplug device to > > > the PCI bridge will success when we use SHPC Native type. > > > > > > [1.703975] shpchp :00:03.0: Requesting control of SHPC hotplug via > > > OSHP (\_SB_.PCI0.S18_) > > > [1.704934] shpchp :00:03.0: Requesting control of SHPC hotplug via > > > OSHP (\_SB_.PCI0) > > > [1.705855] shpchp :00:03.0: Gained control of SHPC hotplug > > > (\_SB_.PCI0) > > > [1.707054] shpchp :00:03.0: HPC vendor_id 1b36 device_id 1 ss_vid 0 > > > ss_did 0 > > > > please describe in commit message reproducer > > (aka QEMU CLI and guest OS and if necessary other details) > > qemu-system-x86_64 -machine pc-q35-9.0 sorry, it is -machine pc-i440fx-9.0 > ... > -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off >
Re: [v2 1/1] hw/i386/acpi-build: add OSHP method support for SHPC driver load
> > that OS cannot get control of SHPC hotplug and hotplug device to
> > the PCI bridge will fail when we use SHPC Native type:
> >
> > [3.336059] shpchp :00:03.0: Requesting control of SHPC hotplug via
> >OSHP (\_SB_.PCI0.S28_)
> > [3.337408] shpchp :00:03.0: Requesting control of SHPC hotplug via
> >OSHP (\_SB_.PCI0)
> > [3.338710] shpchp :00:03.0: Cannot get control of SHPC hotplug
> >
> > Add OSHP method support for transfer control to the operating system,
> > after this SHPC driver will be loaded success and the hotplug device to
> > the PCI bridge will success when we use SHPC Native type.
> >
> > [1.703975] shpchp :00:03.0: Requesting control of SHPC hotplug via
> >OSHP (\_SB_.PCI0.S18_)
> > [1.704934] shpchp :00:03.0: Requesting control of SHPC hotplug via
> >OSHP (\_SB_.PCI0)
> > [1.705855] shpchp :00:03.0: Gained control of SHPC hotplug
> >(\_SB_.PCI0)
> > [1.707054] shpchp :00:03.0: HPC vendor_id 1b36 device_id 1 ss_vid 0
> >ss_did 0
>
> please describe in commit message reproducer
> (aka QEMU CLI and guest OS and if necessary other details)
qemu-system-x86_64 -machine pc-q35-9.0
...
-global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=off
guest OS: centos7/ubuntu22.04
I will add it in the next version.
> > +/*
> > + * PCI Firmware Specification 3.0
> > + * 4.8. The OSHP Control Method
> > + */
> > +static Aml *build_oshp_method(void)
> > +{
> > + Aml *method;
> > +
> > + /*
> > + * We don't use ACPI to control the SHPC, so just return
> > + * success is enough.
> > + */
> > + method = aml_method("OSHP", 0, AML_NOTSERIALIZED);
> > + aml_append(method, aml_return(aml_int(0x0)));
> > + return method;
> > +}
> > +
> > static void
> > build_dsdt(GArray *table_data, BIOSLinker *linker,
> > AcpiPmInfo *pm, AcpiMiscInfo *misc,
> > @@ -1452,6 +1469,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > aml_append(dev, aml_name_decl("_UID",
> >aml_int(pcmc->pci_root_uid)));
> > aml_append(dev, aml_pci_edsm());
> > + aml_append(dev, build_oshp_method());
>
> it's global and what will happen if we have ACPI PCI hotplug enabled
> and guest calls this NOP method?
ths OS get the control of SHPC hotplug and SHPC driver load fail later.
[6.170345] shpchp :00:03.0: Requesting control of SHPC hotplug via OSHP
(\_SB_.PCI0.S18_)
[6.171962] shpchp :00:03.0: Requesting control of SHPC hotplug via OSHP
(\_SB_.PCI0)
[6.173556] shpchp :00:03.0: Gained control of SHPC hotplug (\_SB_.PCI0)
[6.175144] shpchp :00:03.0: HPC vendor_id 1b36 device_id 1 ss_vid 0
ss_did 0
[6.196153] shpchp :00:03.0: irq 24 for MSI/MSI-X
[6.197211] shpchp :00:03.0: pci_hp_register failed with error -16
[6.198272] shpchp :00:03.0: Slot initialization failed
this looks more suitable.
+if (!pm->pcihp_bridge_en) {
+aml_append(dev, build_i440fx_oshp_method());
+}
>
> > aml_append(sb_scope, dev);
> > aml_append(dsdt, sb_scope);
> >
> > @@ -1586,6 +1604,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, build_q35_osc_method(true));
> > } else {
> > aml_append(dev, aml_name_decl("_HID",
> >aml_eisaid("PNP0A03")));
> > + aml_append(dev, build_oshp_method());
> > }
> >
> > if (numa_node != NUMA_NODE_UNASSIGNED) {
Hot plug/unplug a device using SHPC will take more time than ACPI PCI hotplug,
because
after pressing the button, it can be cancelled within 5 seconds in SHPC driver.
If I want to use ACPI PCI hotplug in the pxb bridge, what else need to be done?
thanks.
RE: [PATCH v3] hw/core/loader: allow loading larger ROMs
> -Original Message-
> From: Gregor Haas
> Sent: Friday, June 28, 2024 9:51 AM
> To: qemu-devel@nongnu.org
> Cc: Yao, Xingtao/姚 幸涛 ; Gregor Haas
>
> Subject: [PATCH v3] hw/core/loader: allow loading larger ROMs
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read(). Also, the ROM size is now determined using the
> get_image_size() function rather than using manual lseek().
>
> Signed-off-by: Gregor Haas
> ---
> hw/core/loader.c | 17 ++---
> 1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..c2c61158f1 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1076,7 +1076,6 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> ssize_t rc;
> -int fd = -1;
> char devpath[100];
>
> if (as && mr) {
> @@ -1094,19 +1093,12 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> rom->path = g_strdup(file);
> }
>
> -fd = open(rom->path, O_RDONLY | O_BINARY);
> -if (fd == -1) {
> -fprintf(stderr, "Could not open option rom '%s': %s\n",
> -rom->path, strerror(errno));
> -goto err;
> -}
> -
> if (fw_dir) {
> rom->fw_dir = g_strdup(fw_dir);
> rom->fw_file = g_strdup(file);
> }
> rom->addr = addr;
> -rom->romsize = lseek(fd, 0, SEEK_END);
> +rom->romsize = get_image_size(rom->path);
> if (rom->romsize == -1) {
> fprintf(stderr, "rom: file %-20s: get size error: %s\n",
> rom->name, strerror(errno));
> @@ -1115,14 +1107,12 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
>
> rom->datasize = rom->romsize;
> rom->data = g_malloc0(rom->datasize);
> -lseek(fd, 0, SEEK_SET);
> -rc = read(fd, rom->data, rom->datasize);
> +rc = load_image_size(rom->path, rom->data, rom->datasize);
> if (rc != rom->datasize) {
> fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected
> %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> -close(fd);
> rom_insert(rom);
> if (rom->fw_file && fw_cfg) {
> const char *basename;
> @@ -1159,9 +1149,6 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> return 0;
>
> err:
> -if (fd != -1)
> -close(fd);
> -
> rom_free(rom);
> return -1;
> }
> --
> 2.45.2
Reviewed-by: Xingtao Yao
[PATCH] util/cpuinfo-ppc: Add FreeBSD support
util/cpuinfo-ppc: Add FreeBSD support Signed-off-by: Brad Smith --- With corrected sign-off. Also this was based on the tcg-next branch. util/cpuinfo-ppc.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/util/cpuinfo-ppc.c b/util/cpuinfo-ppc.c index 47af55aa0c..0ad634b46f 100644 --- a/util/cpuinfo-ppc.c +++ b/util/cpuinfo-ppc.c @@ -14,6 +14,11 @@ # include "elf.h" # endif #endif +#ifdef __FreeBSD__ +# include +# define PPC_FEATURE2_ARCH_3_1 0 +# define PPC_FEATURE2_VEC_CRYPTO PPC_FEATURE2_HAS_VEC_CRYPTO +#endif unsigned cpuinfo; @@ -28,7 +33,7 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) info = CPUINFO_ALWAYS; -#ifdef CONFIG_LINUX +#if defined(CONFIG_LINUX) || defined(__FreeBSD__) unsigned long hwcap = qemu_getauxval(AT_HWCAP); unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2); -- 2.45.2
[PATCH] util/cpuinfo-ppc: Add FreeBSD support
util/cpuinfo-ppc: Add FreeBSD support Signed-off-by: default avatarBrad Smith --- util/cpuinfo-ppc.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/util/cpuinfo-ppc.c b/util/cpuinfo-ppc.c index 47af55aa0c..0ad634b46f 100644 --- a/util/cpuinfo-ppc.c +++ b/util/cpuinfo-ppc.c @@ -14,6 +14,11 @@ # include "elf.h" # endif #endif +#ifdef __FreeBSD__ +# include +# define PPC_FEATURE2_ARCH_3_1 0 +# define PPC_FEATURE2_VEC_CRYPTO PPC_FEATURE2_HAS_VEC_CRYPTO +#endif unsigned cpuinfo; @@ -28,7 +33,7 @@ unsigned __attribute__((constructor)) cpuinfo_init(void) info = CPUINFO_ALWAYS; -#ifdef CONFIG_LINUX +#if defined(CONFIG_LINUX) || defined(__FreeBSD__) unsigned long hwcap = qemu_getauxval(AT_HWCAP); unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2); -- 2.45.2
[PATCH v3] hw/core/loader: allow loading larger ROMs
The read() syscall is not guaranteed to return all data from a file. The
default ROM loader implementation currently does not take this into account,
instead failing if all bytes are not read at once. This change loads the ROM
using load_image_size() instead, which correctly reads all data using
multiple calls to read(). Also, the ROM size is now determined using the
get_image_size() function rather than using manual lseek().
Signed-off-by: Gregor Haas
---
hw/core/loader.c | 17 ++---
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2f8105d7de..c2c61158f1 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1076,7 +1076,6 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
Rom *rom;
ssize_t rc;
-int fd = -1;
char devpath[100];
if (as && mr) {
@@ -1094,19 +1093,12 @@ ssize_t rom_add_file(const char *file, const char
*fw_dir,
rom->path = g_strdup(file);
}
-fd = open(rom->path, O_RDONLY | O_BINARY);
-if (fd == -1) {
-fprintf(stderr, "Could not open option rom '%s': %s\n",
-rom->path, strerror(errno));
-goto err;
-}
-
if (fw_dir) {
rom->fw_dir = g_strdup(fw_dir);
rom->fw_file = g_strdup(file);
}
rom->addr = addr;
-rom->romsize = lseek(fd, 0, SEEK_END);
+rom->romsize = get_image_size(rom->path);
if (rom->romsize == -1) {
fprintf(stderr, "rom: file %-20s: get size error: %s\n",
rom->name, strerror(errno));
@@ -1115,14 +1107,12 @@ ssize_t rom_add_file(const char *file, const char
*fw_dir,
rom->datasize = rom->romsize;
rom->data = g_malloc0(rom->datasize);
-lseek(fd, 0, SEEK_SET);
-rc = read(fd, rom->data, rom->datasize);
+rc = load_image_size(rom->path, rom->data, rom->datasize);
if (rc != rom->datasize) {
fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
rom->name, rc, rom->datasize);
goto err;
}
-close(fd);
rom_insert(rom);
if (rom->fw_file && fw_cfg) {
const char *basename;
@@ -1159,9 +1149,6 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
return 0;
err:
-if (fd != -1)
-close(fd);
-
rom_free(rom);
return -1;
}
--
2.45.2
RE: [PATCH v2] hw/core/loader: allow loading larger ROMs
> -Original Message-
> From: Gregor Haas
> Sent: Friday, June 28, 2024 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: Yao, Xingtao/姚 幸涛 ; Gregor Haas
>
> Subject: [PATCH v2] hw/core/loader: allow loading larger ROMs
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read().
>
> Signed-off-by: Gregor Haas
> ---
> hw/core/loader.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..8216781a75 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1115,14 +1115,13 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
>
> rom->datasize = rom->romsize;
> rom->data = g_malloc0(rom->datasize);
> -lseek(fd, 0, SEEK_SET);
> -rc = read(fd, rom->data, rom->datasize);
> +close(fd);
> +rc = load_image_size(rom->path, rom->data, rom->datasize);
LGTM.
I think we may get romsize by get_image_size() and delete the redundant code
like below:
-fd = open(rom->path, O_RDONLY | O_BINARY);
-if (fd == -1) {
-fprintf(stderr, "Could not open option rom '%s': %s\n",
-rom->path, strerror(errno));
+rom->romesize = get_image_size(rom->path);
+if (rom->romsize == -1) {
+fprintf(stderr, "rom: file %-20s: get size error: %s\n",
+rom->name, strerror(errno));
goto err;
}
@@ -1106,16 +1106,9 @@ ssize_t rom_add_file(const char *file, const char
*fw_dir,
rom->fw_file = g_strdup(file);
}
rom->addr = addr;
-rom->romsize = lseek(fd, 0, SEEK_END);
-if (rom->romsize == -1) {
-fprintf(stderr, "rom: file %-20s: get size error: %s\n",
-rom->name, strerror(errno));
-goto err;
-}
rom->datasize = rom->romsize;
rom->data = g_malloc0(rom->datasize);
-lseek(fd, 0, SEEK_SET);
> if (rc != rom->datasize) {
> fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected
> %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> -close(fd);
> rom_insert(rom);
> if (rom->fw_file && fw_cfg) {
> const char *basename;
> --
> 2.45.2
[PATCH v2] hw/core/loader: allow loading larger ROMs
The read() syscall is not guaranteed to return all data from a file. The
default ROM loader implementation currently does not take this into account,
instead failing if all bytes are not read at once. This change loads the ROM
using load_image_size() instead, which correctly reads all data using
multiple calls to read().
Signed-off-by: Gregor Haas
---
hw/core/loader.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2f8105d7de..8216781a75 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1115,14 +1115,13 @@ ssize_t rom_add_file(const char *file, const char
*fw_dir,
rom->datasize = rom->romsize;
rom->data = g_malloc0(rom->datasize);
-lseek(fd, 0, SEEK_SET);
-rc = read(fd, rom->data, rom->datasize);
+close(fd);
+rc = load_image_size(rom->path, rom->data, rom->datasize);
if (rc != rom->datasize) {
fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
rom->name, rc, rom->datasize);
goto err;
}
-close(fd);
rom_insert(rom);
if (rom->fw_file && fw_cfg) {
const char *basename;
--
2.45.2
Re: [PATCH] hw/core/loader: allow loading larger ROMs
Hi Xingtao,
Thank you for reproducing this -- I agree with your conclusion and will
send a v2 patchset momentarily.
Thank you,
Gregor
On Thu, Jun 27, 2024 at 5:44 PM Xingtao Yao (Fujitsu) <
yaoxt.f...@fujitsu.com> wrote:
> Hi, Gregor
>
>
>
> >rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
> >qemu-system-riscv64: could not load firmware 'fw_payload.bin'
>
> Thanks, I was able to reproduce the problem when the images size is
> larger than 2147479552.
>
>
>
> I found that in my test environment, the maximum value returned by a read
> operation is 2147479552,
>
> which was affected by the operating system.
>
>
>
> We can find this limitation in the man page:
>
> NOTES
>
>The types size_t and ssize_t are, respectively, unsigned and
> signed integer data types specified by POSIX.1.
>
>
>
>On Linux, read() (and similar system calls) will transfer at most
> 0x7000 (2,147,479,552) bytes, returning the number of bytes actually
> transferred. (This is true on both
>
>32-bit and 64-bit systems.)
>
>
>
>
>
> > +do {
>
> > +rc = read(fd, >data[sz], rom->datasize);
>
> > +if (rc == -1) {
>
> > +fprintf(stderr, "rom: file %-20s: read error: %s\n",
>
> > +rom->name, strerror(errno));
>
> > +goto err;
>
> > +}
>
> > +sz += rc;
>
> > +} while (sz != rom->datasize);
>
> I think we can use load_image_size() instead.
>
>
>
>
>
>
>
>
>
> *From:* Gregor Haas
> *Sent:* Friday, June 28, 2024 1:35 AM
> *To:* Yao, Xingtao/姚 幸涛
> *Cc:* qemu-devel@nongnu.org; phi...@linaro.org;
> richard.hender...@linaro.org
> *Subject:* Re: [PATCH] hw/core/loader: allow loading larger ROMs
>
>
>
> Hi Xingtao,
>
> > Can you reproduce this issue?
> Absolutely! I encountered this when trying to load an OpenSBI payload
> firmware using the bios option for the QEMU RISC-V virt board. These
> payload firmwares bundle the entire next boot stage, which in my case is a
> build of the Linux kernel (which is a standard configuration, supported by
> tools such as Buildroot [1]). My kernel (configured with the default 64-bit
> RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI
> firmware of final size 10M. Then, I run the following QEMU command:
>
> qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin
>
> and get the following output:
>
> rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
> qemu-system-riscv64: could not load firmware 'fw_payload.bin'
>
> This is from my development machine, running Arch Linux with kernel 6.9.6
> and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make
> a minimal reproducer for this, or if you need any more information.
>
> Thanks,
> Gregor
>
> [1]
> https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95
>
>
>
> On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) <
> yaoxt.f...@fujitsu.com> wrote:
>
> Hi, Gregor
> >
> > The read() syscall is not guaranteed to return all data from a file. The
> > default ROM loader implementation currently does not take this into
> account,
> > instead failing if all bytes are not read at once. This change wraps the
> > read() syscall in a do/while loop to ensure all bytes of the ROM are
> read.
> Can you reproduce this issue?
>
> Thanks
> Xingtao
>
>
RE: [PATCH] hw/core/loader: allow loading larger ROMs
Hi, Gregor
>rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
>qemu-system-riscv64: could not load firmware 'fw_payload.bin'
Thanks, I was able to reproduce the problem when the images size is larger than
2147479552.
I found that in my test environment, the maximum value returned by a read
operation is 2147479552,
which was affected by the operating system.
We can find this limitation in the man page:
NOTES
The types size_t and ssize_t are, respectively, unsigned and signed
integer data types specified by POSIX.1.
On Linux, read() (and similar system calls) will transfer at most
0x7000 (2,147,479,552) bytes, returning the number of bytes actually
transferred. (This is true on both
32-bit and 64-bit systems.)
> +do {
> +rc = read(fd, >data[sz], rom->datasize);
> +if (rc == -1) {
> +fprintf(stderr, "rom: file %-20s: read error: %s\n",
> +rom->name, strerror(errno));
> +goto err;
> +}
> +sz += rc;
> +} while (sz != rom->datasize);
I think we can use load_image_size() instead.
From: Gregor Haas
Sent: Friday, June 28, 2024 1:35 AM
To: Yao, Xingtao/姚 幸涛
Cc: qemu-devel@nongnu.org; phi...@linaro.org; richard.hender...@linaro.org
Subject: Re: [PATCH] hw/core/loader: allow loading larger ROMs
Hi Xingtao,
> Can you reproduce this issue?
Absolutely! I encountered this when trying to load an OpenSBI payload
firmware using the bios option for the QEMU RISC-V virt board. These
payload firmwares bundle the entire next boot stage, which in my case is a
build of the Linux kernel (which is a standard configuration, supported by
tools such as Buildroot [1]). My kernel (configured with the default 64-bit
RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI
firmware of final size 10M. Then, I run the following QEMU command:
qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin
and get the following output:
rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
qemu-system-riscv64: could not load firmware 'fw_payload.bin'
This is from my development machine, running Arch Linux with kernel 6.9.6
and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make
a minimal reproducer for this, or if you need any more information.
Thanks,
Gregor
[1]
https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95
On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu)
mailto:yaoxt.f...@fujitsu.com>> wrote:
Hi, Gregor
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change wraps the
> read() syscall in a do/while loop to ensure all bytes of the ROM are read.
Can you reproduce this issue?
Thanks
Xingtao
Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency
On 6/25/24 11:08, Manos Pitsidianakis wrote:
On Tue, 25 Jun 2024 19:00, Zhao Liu wrote:
[snip]
This is for future-proofing the Rust integration in general. I
haven't been
able to compile under macos yet because bindgen cannot find the system clang
header. I also don't have a windows pc to test it on. But it should work
theoretically under all three.
Yes, they should work. EMM, but there is no particular need for them at
the moment, so just to be safe, we can put these two platforms on hold
for now, and they can be easily added when the tests are covered.
A TODO can remind support for them.
I'm still trying to figure out why bindgen doesn't find the /Library/***
include paths.. it's frustrating! I will remove them if I don't succeed
and also no one volunteers to attempt a windows build. :)
I'm currently doing it, and managed to run until bindgen step. Same
problem that you found on MacOS, it can't locate some headers
(strings.h, included from osdep.h). I'll try to dig into this, but if
you found a solution already, you're welcome to share it.
'gcc | grep' command you used should work, but should be adapted because
windows paths start with C:/ instead of /.
[snip]
Could we decouple 'optimization' with cargo's opt-level (maybe in the future)?
Or we could add new option to specify custom rust opt-level, which will
set the environment varialbe CARGO_PROFILE__OPT_LEVEL and
override the default opt-level in Cargo.toml.
optimization here refers to LLVM's opt flags directly, so I think it makes
sense to follow the project-wide compiler settings?
Oh, yes. But I think the choice of debug or release is best based on the
debug info. What about this?
if get_option('debug')
rs_build_type = 'debug'
else
rs_build_type = 'release'
endif
get_option('debug') would apply -g flag, and that flag is mapped to debuginfo=2,
which is the default debuginfo level for debug version.
But wait, I just noticed rs_build_type should be 'dev' not 'debug'. You
can build with optimizations and keep debuginfo if you override the
'release' profile or use a different profile altogether.
I will correct the optimization and debuginfo mappings in the next
version.
It's a totally new file, and it is used to derive other files. These
are
all significant changes. At least for 2024, I think you can declare
copyright here. :-)
Heh yes :)
Thanks,
Zhao
[PATCH v3 4/4] tests/migration: Add integration test for 'qatzip' compression method
From: Bryan Zhang
Adds an integration test for 'qatzip'.
Signed-off-by: Bryan Zhang
Signed-off-by: Hao Xiang
Signed-off-by: Yichen Wang
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 571fc1334c..cc4a971e63 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -32,6 +32,10 @@
# endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
+#ifdef CONFIG_QATZIP
+#include
+#endif /* CONFIG_QATZIP */
+
/* For dirty ring test; so far only x86_64 is supported */
#if defined(__linux__) && defined(HOST_X86_64)
#include "linux/kvm.h"
@@ -2992,6 +2996,22 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
+#ifdef CONFIG_QATZIP
+static void *
+test_migrate_precopy_tcp_multifd_qatzip_start(QTestState *from,
+ QTestState *to)
+{
+migrate_set_parameter_int(from, "multifd-qatzip-level", 2);
+migrate_set_parameter_int(to, "multifd-qatzip-level", 2);
+
+/* SW fallback is disabled by default, so enable it for testing. */
+migrate_set_parameter_bool(from, "multifd-qatzip-sw-fallback", true);
+migrate_set_parameter_bool(to, "multifd-qatzip-sw-fallback", true);
+
+return test_migrate_precopy_tcp_multifd_start_common(from, to, "qatzip");
+}
+#endif
+
#ifdef CONFIG_QPL
static void *
test_migrate_precopy_tcp_multifd_qpl_start(QTestState *from,
@@ -3089,6 +3109,17 @@ static void test_multifd_tcp_zstd(void)
}
#endif
+#ifdef CONFIG_QATZIP
+static void test_multifd_tcp_qatzip(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_qatzip_start,
+};
+test_precopy_common();
+}
+#endif
+
#ifdef CONFIG_QPL
static void test_multifd_tcp_qpl(void)
{
@@ -4002,6 +4033,10 @@ int main(int argc, char **argv)
migration_test_add("/migration/multifd/tcp/plain/zstd",
test_multifd_tcp_zstd);
#endif
+#ifdef CONFIG_QATZIP
+migration_test_add("/migration/multifd/tcp/plain/qatzip",
+test_multifd_tcp_qatzip);
+#endif
#ifdef CONFIG_QPL
migration_test_add("/migration/multifd/tcp/plain/qpl",
test_multifd_tcp_qpl);
--
Yichen Wang
[PATCH v3 0/4] Implement using Intel QAT to offload ZLIB
v3: - Rebase changes on top of master - Merge two patches per Fabiano Rosas's comment - Add versions into comments and documentations v2: - Rebase changes on top of recent multifd code changes. - Use QATzip API 'qzMalloc' and 'qzFree' to allocate QAT buffers. - Remove parameter tuning and use QATzip's defaults for better performance. - Add parameter to enable QAT software fallback. v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-12/msg03761.html * Performance We present updated performance results. For circumstantial reasons, v1 presented performance on a low-bandwidth (1Gbps) network. Here, we present updated results with a similar setup as before but with two main differences: 1. Our machines have a ~50Gbps connection, tested using 'iperf3'. 2. We had a bug in our memory allocation causing us to only use ~1/2 of the VM's RAM. Now we properly allocate and fill nearly all of the VM's RAM. Thus, the test setup is as follows: We perform multifd live migration over TCP using a VM with 64GB memory. We prepare the machine's memory by powering it on, allocating a large amount of memory (60GB) as a single buffer, and filling the buffer with the repeated contents of the Silesia corpus[0]. This is in lieu of a more realistic memory snapshot, which proved troublesome to acquire. We analyze CPU usage by averaging the output of 'top' every second during migration. This is admittedly imprecise, but we feel that it accurately portrays the different degrees of CPU usage of varying compression methods. We present the latency, throughput, and CPU usage results for all of the compression methods, with varying numbers of multifd threads (4, 8, and 16). [0] The Silesia corpus can be accessed here: https://sun.aei.polsl.pl//~sdeor/index.php?page=silesia ** Results 4 multifd threads: |---|---||-|-| |method |time(sec) |throughput(mbps)|send cpu%|recv cpu%| |---|---||-|-| |qatzip | 23.13 | 8749.94|117.50 |186.49 | |---|---||-|-| |zlib |254.35 | 771.87|388.20 |144.40 | |---|---||-|-| |zstd | 54.52 | 3442.59|414.59 |149.77 | |---|---||-|-| |none | 12.45 |43739.60|159.71 |204.96 | |---|---||-|-| 8 multifd threads: |---|---||-|-| |method |time(sec) |throughput(mbps)|send cpu%|recv cpu%| |---|---||-|-| |qatzip | 16.91 |12306.52|186.37 |391.84 | |---|---||-|-| |zlib |130.11 | 1508.89|753.86 |289.35 | |---|---||-|-| |zstd | 27.57 | 6823.23|786.83 |303.80 | |---|---||-|-| |none | 11.82 |46072.63|163.74 |238.56 | |---|---||-|-| 16 multifd threads: |---|---||-|-| |method |time(sec) |throughput(mbps)|send cpu%|recv cpu%| |---|---||-|-| |qatzip |18.64 |11044.52| 573.61 |437.65 | |---|---||-|-| |zlib |66.43 | 2955.79|1469.68 |567.47 | |---|---||-|-| |zstd |14.17 |13290.66|1504.08 |615.33 | |---|---||-|-| |none |16.82 |32363.26| 180.74 |217.17 | |---|---||-|-| ** Observations - In general, not using compression outperforms using compression in a non-network-bound environment. - 'qatzip' outperforms other compression workers with 4 and 8 workers, achieving a ~91% latency reduction over 'zlib' with 4 workers, and a ~58% latency reduction over 'zstd' with 4 workers. - 'qatzip' maintains comparable performance with 'zstd' at 16 workers, showing a ~32% increase in latency. This performance difference becomes more noticeable with more workers, as CPU compression is highly parallelizable. - 'qatzip' compression uses considerably less CPU than other compression methods. At 8 workers, 'qatzip' demonstrates a
[PATCH v3 3/4] migration: Introduce 'qatzip' compression method
From: Bryan Zhang
Adds support for 'qatzip' as an option for the multifd compression
method parameter, and implements using QAT for 'qatzip' compression and
decompression.
Signed-off-by: Bryan Zhang
Signed-off-by: Hao Xiang
Signed-off-by: Yichen Wang
---
hw/core/qdev-properties-system.c | 6 +-
migration/meson.build| 1 +
migration/multifd-qatzip.c | 382 +++
migration/multifd.h | 1 +
qapi/migration.json | 3 +
tests/qtest/meson.build | 4 +
6 files changed, 396 insertions(+), 1 deletion(-)
create mode 100644 migration/multifd-qatzip.c
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index f13350b4fb..eb50d6ec5b 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -659,7 +659,11 @@ const PropertyInfo qdev_prop_fdc_drive_type = {
const PropertyInfo qdev_prop_multifd_compression = {
.name = "MultiFDCompression",
.description = "multifd_compression values, "
- "none/zlib/zstd/qpl/uadk",
+ "none/zlib/zstd/qpl/uadk"
+#ifdef CONFIG_QATZIP
+ "/qatzip"
+#endif
+ ,
.enum_table = _lookup,
.get = qdev_propinfo_get_enum,
.set = qdev_propinfo_set_enum,
diff --git a/migration/meson.build b/migration/meson.build
index 5ce2acb41e..c9454c26ae 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -41,6 +41,7 @@ system_ss.add(when: rdma, if_true: files('rdma.c'))
system_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
system_ss.add(when: qpl, if_true: files('multifd-qpl.c'))
system_ss.add(when: uadk, if_true: files('multifd-uadk.c'))
+system_ss.add(when: qatzip, if_true: files('multifd-qatzip.c'))
specific_ss.add(when: 'CONFIG_SYSTEM_ONLY',
if_true: files('ram.c',
diff --git a/migration/multifd-qatzip.c b/migration/multifd-qatzip.c
new file mode 100644
index 00..19e54889dc
--- /dev/null
+++ b/migration/multifd-qatzip.c
@@ -0,0 +1,382 @@
+/*
+ * Multifd QATzip compression implementation
+ *
+ * Copyright (c) Bytedance
+ *
+ * Authors:
+ * Bryan Zhang
+ * Hao Xiang
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "exec/ramblock.h"
+#include "exec/target_page.h"
+#include "qapi/error.h"
+#include "migration.h"
+#include "options.h"
+#include "multifd.h"
+#include
+
+struct qatzip_data {
+/*
+ * Unique session for use with QATzip API
+ */
+QzSession_T sess;
+
+/*
+ * For compression: Buffer for pages to compress
+ * For decompression: Buffer for data to decompress
+ */
+uint8_t *in_buf;
+uint32_t in_len;
+
+/*
+ * For compression: Output buffer of compressed data
+ * For decompression: Output buffer of decompressed data
+ */
+uint8_t *out_buf;
+uint32_t out_len;
+};
+
+/**
+ * qatzip_send_setup: Set up QATzip session and private buffers.
+ *
+ * @param pMultifd channel params
+ * @param errp Pointer to error, which will be set in case of error
+ * @return 0 on success, -1 on error (and *errp will be set)
+ */
+static int qatzip_send_setup(MultiFDSendParams *p, Error **errp)
+{
+struct qatzip_data *q;
+QzSessionParamsDeflate_T params;
+const char *err_msg;
+int ret;
+int sw_fallback;
+
+q = g_new0(struct qatzip_data, 1);
+p->compress_data = q;
+
+sw_fallback = 0;
+if (migrate_multifd_qatzip_sw_fallback()) {
+sw_fallback = 1;
+}
+
+ret = qzInit(>sess, sw_fallback);
+if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+err_msg = "qzInit failed";
+goto err_free_q;
+}
+
+ret = qzGetDefaultsDeflate();
+if (ret != QZ_OK) {
+err_msg = "qzGetDefaultsDeflate failed";
+goto err_close;
+}
+
+/* Make sure to use configured QATzip compression level. */
+params.common_params.comp_lvl = migrate_multifd_qatzip_level();
+
+ret = qzSetupSessionDeflate(>sess, );
+if (ret != QZ_OK && ret != QZ_DUPLICATE) {
+err_msg = "qzSetupSessionDeflate failed";
+goto err_close;
+}
+
+/* TODO Add support for larger packets. */
+if (MULTIFD_PACKET_SIZE > UINT32_MAX) {
+err_msg = "packet size too large for QAT";
+goto err_close;
+}
+
+q->in_len = MULTIFD_PACKET_SIZE;
+q->in_buf = qzMalloc(q->in_len, 0, PINNED_MEM);
+if (!q->in_buf) {
+err_msg = "qzMalloc failed";
+goto err_close;
+}
+
+q->out_len = qzMaxCompressedLength(MULTIFD_PACKET_SIZE, >sess);
+q->out_buf = qzMalloc(q->out_len, 0, PINNED_MEM);
+if (!q->out_buf) {
+err_msg = "qzMalloc failed";
+goto err_free_inbuf;
+}
+
+return 0;
+
+err_free_inbuf:
+qzFree(q->in_buf);
+err_close:
+qzClose(>sess);
+err_free_q:
+g_free(q);
+error_setg(errp, "multifd %u: %s",
[PATCH v3 1/4] meson: Introduce 'qatzip' feature to the build system
From: Bryan Zhang
Add a 'qatzip' feature, which is automatically disabled, and which
depends on the QATzip library if enabled.
Signed-off-by: Bryan Zhang
Signed-off-by: Hao Xiang
Signed-off-by: Yichen Wang
---
meson.build | 10 ++
meson_options.txt | 2 ++
scripts/meson-buildoptions.sh | 6 ++
3 files changed, 18 insertions(+)
diff --git a/meson.build b/meson.build
index 97e00d6f59..009f07f506 100644
--- a/meson.build
+++ b/meson.build
@@ -1219,6 +1219,14 @@ if not get_option('uadk').auto() or have_system
uadk = declare_dependency(dependencies: [libwd, libwd_comp])
endif
endif
+
+qatzip = not_found
+if get_option('qatzip').enabled()
+ qatzip = dependency('qatzip', version: '>=1.1.2',
+ required: get_option('qatzip'),
+ method: 'pkg-config')
+endif
+
virgl = not_found
have_vhost_user_gpu = have_tools and host_os == 'linux' and pixman.found()
@@ -2353,6 +2361,7 @@ config_host_data.set('CONFIG_STATX_MNT_ID',
has_statx_mnt_id)
config_host_data.set('CONFIG_ZSTD', zstd.found())
config_host_data.set('CONFIG_QPL', qpl.found())
config_host_data.set('CONFIG_UADK', uadk.found())
+config_host_data.set('CONFIG_QATZIP', qatzip.found())
config_host_data.set('CONFIG_FUSE', fuse.found())
config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
config_host_data.set('CONFIG_SPICE_PROTOCOL', spice_protocol.found())
@@ -4468,6 +4477,7 @@ summary_info += {'lzfse support': liblzfse}
summary_info += {'zstd support': zstd}
summary_info += {'Query Processing Library support': qpl}
summary_info += {'UADK Library support': uadk}
+summary_info += {'qatzip support':qatzip}
summary_info += {'NUMA host support': numa}
summary_info += {'capstone': capstone}
summary_info += {'libpmem support': libpmem}
diff --git a/meson_options.txt b/meson_options.txt
index 7a79dd8970..3670e5058b 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -263,6 +263,8 @@ option('qpl', type : 'feature', value : 'auto',
description: 'Query Processing Library support')
option('uadk', type : 'feature', value : 'auto',
description: 'UADK Library support')
+option('qatzip', type: 'feature', value: 'disabled',
+ description: 'QATzip compression support')
option('fuse', type: 'feature', value: 'auto',
description: 'FUSE block device export')
option('fuse_lseek', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 58d49a447d..226605249e 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -162,6 +162,8 @@ meson_options_help() {
printf "%s\n" ' pixman pixman support'
printf "%s\n" ' plugins TCG plugins via shared library loading'
printf "%s\n" ' png PNG support with libpng'
+ printf "%s\n" ' pvrdma Enable PVRDMA support'
+ printf "%s\n" ' qatzip QATzip compression support'
printf "%s\n" ' qcow1 qcow1 image format support'
printf "%s\n" ' qed qed image format support'
printf "%s\n" ' qga-vss build QGA VSS support (broken with MinGW)'
@@ -428,6 +430,10 @@ _meson_option_parse() {
--enable-png) printf "%s" -Dpng=enabled ;;
--disable-png) printf "%s" -Dpng=disabled ;;
--prefix=*) quote_sh "-Dprefix=$2" ;;
+--enable-pvrdma) printf "%s" -Dpvrdma=enabled ;;
+--disable-pvrdma) printf "%s" -Dpvrdma=disabled ;;
+--enable-qatzip) printf "%s" -Dqatzip=enabled ;;
+--disable-qatzip) printf "%s" -Dqatzip=disabled ;;
--enable-qcow1) printf "%s" -Dqcow1=enabled ;;
--disable-qcow1) printf "%s" -Dqcow1=disabled ;;
--enable-qed) printf "%s" -Dqed=enabled ;;
--
Yichen Wang
[PATCH v3 2/4] migration: Add migration parameters for QATzip
From: Bryan Zhang
Adds support for migration parameters to control QATzip compression
level and to enable/disable software fallback when QAT hardware is
unavailable. This is a preparatory commit for a subsequent commit that
will actually use QATzip compression.
Signed-off-by: Bryan Zhang
Signed-off-by: Hao Xiang
Signed-off-by: Yichen Wang
---
migration/migration-hmp-cmds.c | 8 +
migration/options.c| 57 ++
migration/options.h| 2 ++
qapi/migration.json| 35 +
4 files changed, 102 insertions(+)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index 7d608d26e1..664e2390a3 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -576,6 +576,14 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict
*qdict)
p->has_multifd_zlib_level = true;
visit_type_uint8(v, param, >multifd_zlib_level, );
break;
+case MIGRATION_PARAMETER_MULTIFD_QATZIP_LEVEL:
+p->has_multifd_qatzip_level = true;
+visit_type_uint8(v, param, >multifd_qatzip_level, );
+break;
+case MIGRATION_PARAMETER_MULTIFD_QATZIP_SW_FALLBACK:
+p->has_multifd_qatzip_sw_fallback = true;
+visit_type_bool(v, param, >multifd_qatzip_sw_fallback, );
+break;
case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
p->has_multifd_zstd_level = true;
visit_type_uint8(v, param, >multifd_zstd_level, );
diff --git a/migration/options.c b/migration/options.c
index 645f55003d..334d70fb6d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -55,6 +55,15 @@
#define DEFAULT_MIGRATE_MULTIFD_COMPRESSION MULTIFD_COMPRESSION_NONE
/* 0: means nocompress, 1: best speed, ... 9: best compress ratio */
#define DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL 1
+/*
+ * 1: best speed, ... 9: best compress ratio
+ * There is some nuance here. Refer to QATzip documentation to understand
+ * the mapping of QATzip levels to standard deflate levels.
+ */
+#define DEFAULT_MIGRATE_MULTIFD_QATZIP_LEVEL 1
+/* QATzip's SW fallback implementation is extremely slow, so avoid fallback */
+#define DEFAULT_MIGRATE_MULTIFD_QATZIP_SW_FALLBACK false
+
/* 0: means nocompress, 1: best speed, ... 20: best compress ratio */
#define DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL 1
@@ -123,6 +132,12 @@ Property migration_properties[] = {
DEFINE_PROP_UINT8("multifd-zlib-level", MigrationState,
parameters.multifd_zlib_level,
DEFAULT_MIGRATE_MULTIFD_ZLIB_LEVEL),
+DEFINE_PROP_UINT8("multifd-qatzip-level", MigrationState,
+ parameters.multifd_qatzip_level,
+ DEFAULT_MIGRATE_MULTIFD_QATZIP_LEVEL),
+DEFINE_PROP_BOOL("multifd-qatzip-sw-fallback", MigrationState,
+ parameters.multifd_qatzip_sw_fallback,
+ DEFAULT_MIGRATE_MULTIFD_QATZIP_SW_FALLBACK),
DEFINE_PROP_UINT8("multifd-zstd-level", MigrationState,
parameters.multifd_zstd_level,
DEFAULT_MIGRATE_MULTIFD_ZSTD_LEVEL),
@@ -787,6 +802,20 @@ int migrate_multifd_zlib_level(void)
return s->parameters.multifd_zlib_level;
}
+int migrate_multifd_qatzip_level(void)
+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.multifd_qatzip_level;
+}
+
+bool migrate_multifd_qatzip_sw_fallback(void)
+{
+MigrationState *s = migrate_get_current();
+
+return s->parameters.multifd_qatzip_sw_fallback;
+}
+
int migrate_multifd_zstd_level(void)
{
MigrationState *s = migrate_get_current();
@@ -892,6 +921,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error
**errp)
params->multifd_compression = s->parameters.multifd_compression;
params->has_multifd_zlib_level = true;
params->multifd_zlib_level = s->parameters.multifd_zlib_level;
+params->has_multifd_qatzip_level = true;
+params->multifd_qatzip_level = s->parameters.multifd_qatzip_level;
+params->has_multifd_qatzip_sw_fallback = true;
+params->multifd_qatzip_sw_fallback =
+s->parameters.multifd_qatzip_sw_fallback;
params->has_multifd_zstd_level = true;
params->multifd_zstd_level = s->parameters.multifd_zstd_level;
params->has_xbzrle_cache_size = true;
@@ -946,6 +980,8 @@ void migrate_params_init(MigrationParameters *params)
params->has_multifd_channels = true;
params->has_multifd_compression = true;
params->has_multifd_zlib_level = true;
+params->has_multifd_qatzip_level = true;
+params->has_multifd_qatzip_sw_fallback = true;
params->has_multifd_zstd_level = true;
params->has_xbzrle_cache_size = true;
params->has_max_postcopy_bandwidth = true;
@@ -1038,6 +1074,14 @@ bool migrate_params_check(MigrationParameters *params,
Error **errp)
return false;
}
+if (params->has_multifd_qatzip_level &&
+((params->multifd_qatzip_level > 9) ||
+
Re: standardizing i2c device ids
On Sat, Jun 22, 2024 at 1:43 AM Markus Armbruster wrote: > Patrick Leis writes: > > > Corey and Peter, > > > > My team builds lots of configurations for Qemu boards, and one pain point > > has been that the qom path for a device depends on the device insertion > > order, child[0], child[1] and the like. > > Yes. > > Discussed in my "Dynamic & heterogeneous machines, initial > configuration: problems" memo, under "Problem 4: The > /machine/unattached/ orphanage". > > Copy of the "Problem 4" part appended for your your convenience. Full > memo archived at > https://lore.kernel.org/qemu-devel/87o7d1i7ky@pond.sub.org/ > > > I noticed that the qdev paths > for > > devices also exist by their device id property. By default, this ends up > > being the device type name. > > Which kind of devices? > > There are onboard devices and user-created devices. > > A user-created device's QOM path is "/machine/peripheral/ID" when it was > created with a qdev ID, and "/machine/peripheral-anon/device[N]" (where > N counts up from zero) when it was created without a qdev ID. N depends > on creation order, which is under the user's control. Users can and > should avoid relying on their order by supplying an ID. > > An onboard device's QOM path is chosen by board code. For instance, q35 > puts the mch device at "/machine/q35/mch". However, if the board code > neglects to put the device anywhere, the system puts it at > "/machine/unattached/device[N]" (where N counts up from zero). N > depends on creation order. > > N can change at the drop of a hat. Whether "device[N]" is a stable > interface is unclear. It would clearly be a bad one. > > If (part of) your problem is "/machine/peripheral-anon/device[N]", > supply IDs to bypass it. > > If (part of) your problem is "/machine/unattached/device[N]", all I can > offer is the proper solution: fix the board code to put the device in > its proper place instead of abandoning it to the "/machine/unattached/" > orphanage. > > > I was wondering if it made sense to override > > this with the device type plus the smbus address? I did something > similar > > with the i2c mux device, to resolve part of this issue. > > I doubt it. > > Questions? > > > > = Problem 4: The /machine/unattached/ orphanage = > > Is it okay for a QOM object to have no parent? > > An object without a parent is not part of the composition tree; it has > no canonical path, and object_get_canonical_path() returns null. > > Such objects can behave in wonky ways. For instance, > object_property_set_link() treats a target object without a parent as > null. If a linked object somehow loses its parent, > object_property_get_link() will return null even though the underlying C > pointer still points to the poor orphan. > > This strongly suggests QOM was designed with the assumption that objects > always have a parent, except during initialization (before they are > connected to anything) and finalization (when no longer connected to > anything). object_property_try_add_child()'s contract seems to confirm > this: > > * Child properties form the composition tree. All objects need to be a > child > * of another object. Objects can only be a child of one object. > > Some functions to create objects take the new object's parent as a > parameter. Example: object_new_with_props(), object_new_with_propv(), > clock_new(), ... > > Others set a fixed parent. For instance, we always add character > backends to "/chardevs/", objects created with object-add in > "/objects/", devices created with device_add in "/machine/peripheral/" > (with ID) or "/machine/peripheral-anon/" (without ID), ... > > There are also functions that don't set a parent: object_new(), > object_new_with_class(), qdev_new(), qdev_try_new(), ... Setting a > parent is the callers job then. Invites misuse. I'm aware of one > instance: @current_migration remains without a parent forever. > > Not all callers care to set a parent themselves. Instead, they rely on > the "/machine/unattached/" orphanage: > > * qdev_connect_gpio_out_named() needs the input pin to have a parent. > If it lacks one, it gets added to "/machine/unattached/" with a > made-up name. > > * device_set_realized() ensures realized devices have a parent by adding > devices lacking one to "/machine/unattached/" with a made-up name. > > * portio_list_add() adds a memory region. If the caller doesn't specify > the parent, "/machine/unattached/" is assumed. > > * memory_region_init() adds a memory region, and may set the parent. If > the caller requests setting a parent without specifying one, > "/machine/unattached/" is assumed. > > * qemu_create_machine() adds the main system bus to > "/machine/unattached/". > > Except for the last one, the child names depend on execution order. For > instance, device_set_realized() uses "device[N]", where N counts up from > zero. > > These brittle, made-up
Re: [PATCH v2 2/7] plugins: save value during memory accesses
On 6/27/24 12:25, Pierrick Bouvier wrote: On 6/27/24 12:10, Richard Henderson wrote: On 6/26/24 16:37, Pierrick Bouvier wrote: Different code paths handle memory accesses: - tcg generated code - load/store helpers - atomic helpers This value is saved in cpu->plugin_state. Atomic operations are doing read/write at the same time, so we generate two memory callbacks instead of one, to allow plugins to access distinct values. Signed-off-by: Pierrick Bouvier --- accel/tcg/atomic_template.h | 66 include/qemu/plugin.h | 8 plugins/core.c | 7 tcg/tcg-op-ldst.c | 72 +++ accel/tcg/atomic_common.c.inc | 13 ++- accel/tcg/ldst_common.c.inc | 38 +++--- 6 files changed, 173 insertions(+), 31 deletions(-) diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h index 1dc2151dafd..830e4f16069 100644 --- a/accel/tcg/atomic_template.h +++ b/accel/tcg/atomic_template.h @@ -53,6 +53,14 @@ # error unsupported data size #endif +#if DATA_SIZE == 16 +# define UPPER_MEMORY_VALUE(val) int128_gethi(val) +# define LOWER_MEMORY_VALUE(val) int128_getlo(val) +#else +# define UPPER_MEMORY_VALUE(val) 0 +# define LOWER_MEMORY_VALUE(val) val +#endif + #if DATA_SIZE >= 4 # define ABI_TYPE DATA_TYPE #else @@ -83,7 +91,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr addr, ret = qatomic_cmpxchg__nocheck(haddr, cmpv, newv); #endif ATOMIC_MMU_CLEANUP; - atomic_trace_rmw_post(env, addr, oi); + atomic_trace_rmw_post(env, addr, + UPPER_MEMORY_VALUE(ret), + LOWER_MEMORY_VALUE(ret), + UPPER_MEMORY_VALUE(newv), + LOWER_MEMORY_VALUE(newv), + oi); Just a nit, but tcg is consistent in using little-endian argument ordering for values passed by parts. I would prefer we continue with that. Didn't notice that, but I'll stick to this. Any preference on the naming as well while I'm at it? (low/hi vs upper/lower)? I guess we mostly use high/low, hi/lo, and variations thereof elsewhere as well. I don't see any uses of upper/lower. At some point we may well support 32 byte acceses, for better guest vector support. Do we have a plan for this beyond "add more fields here"? For now, I sticked to native tcg ops (up to 128 bits), with this simple solution. Do you think tcg core will be extended to support more, or will helper simply load/store four 128bits words, emitting four callbacks as well? I assume we'll support a 256-bit (non-atomic) memory operation. That avoids some of the "probe for write, perform stores after we know it's safe" sort of affair. I don't think I'll do it while i686 is still a supported host though. If you have a better idea, I'm open to implement an alternative, but didn't want to think too much ahead. Fair. r~
Re: [PATCH v2 2/7] plugins: save value during memory accesses
On 6/27/24 12:10, Richard Henderson wrote:
On 6/26/24 16:37, Pierrick Bouvier wrote:
Different code paths handle memory accesses:
- tcg generated code
- load/store helpers
- atomic helpers
This value is saved in cpu->plugin_state.
Atomic operations are doing read/write at the same time, so we generate
two memory callbacks instead of one, to allow plugins to access distinct
values.
Signed-off-by: Pierrick Bouvier
---
accel/tcg/atomic_template.h | 66
include/qemu/plugin.h | 8
plugins/core.c| 7
tcg/tcg-op-ldst.c | 72 +++
accel/tcg/atomic_common.c.inc | 13 ++-
accel/tcg/ldst_common.c.inc | 38 +++---
6 files changed, 173 insertions(+), 31 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 1dc2151dafd..830e4f16069 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -53,6 +53,14 @@
# error unsupported data size
#endif
+#if DATA_SIZE == 16
+# define UPPER_MEMORY_VALUE(val) int128_gethi(val)
+# define LOWER_MEMORY_VALUE(val) int128_getlo(val)
+#else
+# define UPPER_MEMORY_VALUE(val) 0
+# define LOWER_MEMORY_VALUE(val) val
+#endif
+
#if DATA_SIZE >= 4
# define ABI_TYPE DATA_TYPE
#else
@@ -83,7 +91,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr
addr,
ret = qatomic_cmpxchg__nocheck(haddr, cmpv, newv);
#endif
ATOMIC_MMU_CLEANUP;
-atomic_trace_rmw_post(env, addr, oi);
+atomic_trace_rmw_post(env, addr,
+ UPPER_MEMORY_VALUE(ret),
+ LOWER_MEMORY_VALUE(ret),
+ UPPER_MEMORY_VALUE(newv),
+ LOWER_MEMORY_VALUE(newv),
+ oi);
Just a nit, but tcg is consistent in using little-endian argument ordering for
values
passed by parts. I would prefer we continue with that.
Didn't notice that, but I'll stick to this. Any preference on the naming
as well while I'm at it? (low/hi vs upper/lower)?
@@ -142,9 +142,13 @@ struct qemu_plugin_tb {
/**
* struct CPUPluginState - per-CPU state for plugins
* @event_mask: plugin event bitmap. Modified only via async work.
+ * @mem_value_upper_bits: 64 upper bits of latest accessed mem value.
+ * @mem_value_lower_bits: 64 lower bits of latest accessed mem value.
*/
struct CPUPluginState {
DECLARE_BITMAP(event_mask, QEMU_PLUGIN_EV_MAX);
+uint64_t mem_value_upper_bits;
+uint64_t mem_value_lower_bits;
};
At some point we may well support 32 byte acceses, for better guest vector
support. Do we
have a plan for this beyond "add more fields here"?
For now, I sticked to native tcg ops (up to 128 bits), with this simple
solution. Do you think tcg core will be extended to support more, or
will helper simply load/store four 128bits words, emitting four
callbacks as well?
If you have a better idea, I'm open to implement an alternative, but
didn't want to think too much ahead.
r~
Re: [PATCH v2 2/7] plugins: save value during memory accesses
On 6/26/24 16:37, Pierrick Bouvier wrote:
Different code paths handle memory accesses:
- tcg generated code
- load/store helpers
- atomic helpers
This value is saved in cpu->plugin_state.
Atomic operations are doing read/write at the same time, so we generate
two memory callbacks instead of one, to allow plugins to access distinct
values.
Signed-off-by: Pierrick Bouvier
---
accel/tcg/atomic_template.h | 66
include/qemu/plugin.h | 8
plugins/core.c| 7
tcg/tcg-op-ldst.c | 72 +++
accel/tcg/atomic_common.c.inc | 13 ++-
accel/tcg/ldst_common.c.inc | 38 +++---
6 files changed, 173 insertions(+), 31 deletions(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 1dc2151dafd..830e4f16069 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -53,6 +53,14 @@
# error unsupported data size
#endif
+#if DATA_SIZE == 16
+# define UPPER_MEMORY_VALUE(val) int128_gethi(val)
+# define LOWER_MEMORY_VALUE(val) int128_getlo(val)
+#else
+# define UPPER_MEMORY_VALUE(val) 0
+# define LOWER_MEMORY_VALUE(val) val
+#endif
+
#if DATA_SIZE >= 4
# define ABI_TYPE DATA_TYPE
#else
@@ -83,7 +91,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr
addr,
ret = qatomic_cmpxchg__nocheck(haddr, cmpv, newv);
#endif
ATOMIC_MMU_CLEANUP;
-atomic_trace_rmw_post(env, addr, oi);
+atomic_trace_rmw_post(env, addr,
+ UPPER_MEMORY_VALUE(ret),
+ LOWER_MEMORY_VALUE(ret),
+ UPPER_MEMORY_VALUE(newv),
+ LOWER_MEMORY_VALUE(newv),
+ oi);
Just a nit, but tcg is consistent in using little-endian argument ordering for values
passed by parts. I would prefer we continue with that.
@@ -142,9 +142,13 @@ struct qemu_plugin_tb {
/**
* struct CPUPluginState - per-CPU state for plugins
* @event_mask: plugin event bitmap. Modified only via async work.
+ * @mem_value_upper_bits: 64 upper bits of latest accessed mem value.
+ * @mem_value_lower_bits: 64 lower bits of latest accessed mem value.
*/
struct CPUPluginState {
DECLARE_BITMAP(event_mask, QEMU_PLUGIN_EV_MAX);
+uint64_t mem_value_upper_bits;
+uint64_t mem_value_lower_bits;
};
At some point we may well support 32 byte acceses, for better guest vector support. Do we
have a plan for this beyond "add more fields here"?
r~
Re: [PATCH v2 6/7] tests/plugin/mem: add option to print memory accesses
On 6/26/24 23:04, Xingtao Yao (Fujitsu) wrote:
-Original Message-
From: Pierrick Bouvier
Sent: Thursday, June 27, 2024 1:29 PM
To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org
Cc: Alexandre Iooss ; Philippe Mathieu-Daudé
; Mahmoud Mandour ; Paolo
Bonzini ; Eduardo Habkost ;
Richard Henderson ; Alex Bennée
Subject: Re: [PATCH v2 6/7] tests/plugin/mem: add option to print memory
accesses
Hi Xingtao,
On 6/26/24 20:17, Xingtao Yao (Fujitsu) wrote:
Hi, Pierrick
+static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
meminfo,
+ uint64_t vaddr, void *udata)
+{
+unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
+const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" :
"load";
+uint64_t upper = qemu_plugin_mem_get_value_upper_bits(meminfo);
+uint64_t lower = qemu_plugin_mem_get_value_lower_bits(meminfo);
+const char *sym = udata ? udata : "";
+g_autoptr(GString) out = g_string_new("");
+g_string_printf(out, "access: 0x%.0"PRIx64"%"PRIx64",%d,%s,%s\n",
+upper, lower, size, type, sym);
+qemu_plugin_outs(out->str);
+}
I think it may be helpful to output the GVA and GPA, can you append these
information?
You mean virtual and physical addresses?
Yes. currently we only known the memory value, appending these info may help us
to trace the
memory access.
Sounds good, I'll add this in v3.
Thanks
Xingtao
[PATCH 2/2] block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked
Upstream clang 18 (and backports to clang 17 in Fedora and RHEL)
implemented support for __attribute__((cleanup())) in its Thread Safety
Analysis, so we can now actually have a proper implementation of
WITH_GRAPH_RDLOCK_GUARD() that understands when we acquire and when we
release the lock.
-Wthread-safety is now only enabled if the compiler is new enough to
understand this pattern. In theory, we could have used some #ifdefs to
keep the existing basic checks on old compilers, but as long as someone
runs a newer compiler (and our CI does), we will catch locking problems,
so it's probably not worth keeping multiple implementations for this.
The implementation can't use g_autoptr any more because the glib macros
define wrapper functions that don't have the right TSA attributes, so
the compiler would complain about them. Just use the cleanup attribute
directly instead.
Signed-off-by: Kevin Wolf
---
include/block/graph-lock.h | 21 ++---
meson.build| 14 +-
2 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index d7545e82d0..dc8d949184 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -209,31 +209,38 @@ typedef struct GraphLockable { } GraphLockable;
* unlocked. TSA_ASSERT_SHARED() makes sure that the following calls know that
* we hold the lock while unlocking is left unchecked.
*/
-static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
coroutine_fn
+static inline GraphLockable * TSA_ACQUIRE_SHARED(graph_lock) coroutine_fn
graph_lockable_auto_lock(GraphLockable *x)
{
bdrv_graph_co_rdlock();
return x;
}
-static inline void TSA_NO_TSA coroutine_fn
-graph_lockable_auto_unlock(GraphLockable *x)
+static inline void TSA_RELEASE_SHARED(graph_lock) coroutine_fn
+graph_lockable_auto_unlock(GraphLockable **x)
{
bdrv_graph_co_rdunlock();
}
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(GraphLockable, graph_lockable_auto_unlock)
+#define GRAPH_AUTO_UNLOCK __attribute__((cleanup(graph_lockable_auto_unlock)))
+/*
+ * @var is only used to break the loop after the first iteration.
+ * @unlock_var can't be unlocked and then set to NULL because TSA wants the
lock
+ * to be held at the start of every iteration of the loop.
+ */
#define WITH_GRAPH_RDLOCK_GUARD_(var) \
-for (g_autoptr(GraphLockable) var = graph_lockable_auto_lock(GML_OBJ_()); \
+for (GraphLockable *unlock_var GRAPH_AUTO_UNLOCK =\
+graph_lockable_auto_lock(GML_OBJ_()), \
+*var = unlock_var;\
var; \
- graph_lockable_auto_unlock(var), var = NULL)
+ var = NULL)
#define WITH_GRAPH_RDLOCK_GUARD() \
WITH_GRAPH_RDLOCK_GUARD_(glue(graph_lockable_auto, __COUNTER__))
#define GRAPH_RDLOCK_GUARD(x) \
-g_autoptr(GraphLockable)\
+GraphLockable * GRAPH_AUTO_UNLOCK \
glue(graph_lockable_auto, __COUNTER__) G_GNUC_UNUSED = \
graph_lockable_auto_lock(GML_OBJ_())
diff --git a/meson.build b/meson.build
index 97e00d6f59..b1d5ce5f1d 100644
--- a/meson.build
+++ b/meson.build
@@ -624,7 +624,19 @@ warn_flags = [
]
if host_os != 'darwin'
- warn_flags += ['-Wthread-safety']
+ tsa_has_cleanup = cc.compiles('''
+struct __attribute__((capability("mutex"))) mutex {};
+void lock(struct mutex *m) __attribute__((acquire_capability(m)));
+void unlock(struct mutex *m) __attribute__((release_capability(m)));
+
+void test(void) {
+ struct mutex __attribute__((cleanup(unlock))) m;
+ lock();
+}
+ ''', args: ['-Wthread-safety', '-Werror'])
+ if tsa_has_cleanup
+warn_flags += ['-Wthread-safety']
+ endif
endif
# Set up C++ compiler flags
--
2.45.2
[PATCH 1/2] block-copy: Fix missing graph lock
The graph lock needs to be held when calling bdrv_co_pdiscard(). Fix
block_copy_task_entry() to take it for the call.
WITH_GRAPH_RDLOCK_GUARD() was implemented in a weak way because of
limitations in clang's Thread Safety Analysis at the time, so that it
only asserts that the lock is held (which allows calling functions that
require the lock), but we never deal with the unlocking (so even after
the scope of the guard, the compiler assumes that the lock is still
held). This is why the compiler didn't catch this locking error.
Signed-off-by: Kevin Wolf
---
block/block-copy.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index 7e3b378528..cc618e4561 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -595,7 +595,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
if (s->discard_source && ret == 0) {
int64_t nbytes =
MIN(t->req.offset + t->req.bytes, s->len) - t->req.offset;
-bdrv_co_pdiscard(s->source, t->req.offset, nbytes);
+WITH_GRAPH_RDLOCK_GUARD() {
+bdrv_co_pdiscard(s->source, t->req.offset, nbytes);
+}
}
return ret;
--
2.45.2
[PATCH 0/2] block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked
Newer clang versions allow us to check scoped guards more thoroughly. Surprisingly, we only seem to have missed one instance with the old incomplete checks. Kevin Wolf (2): block-copy: Fix missing graph lock block/graph-lock: Make WITH_GRAPH_RDLOCK_GUARD() fully checked include/block/graph-lock.h | 21 ++--- block/block-copy.c | 4 +++- meson.build| 14 +- 3 files changed, 30 insertions(+), 9 deletions(-) -- 2.45.2
Re: [PATCH] block/curl: explicitly assert that strchr returns non-NULL value
Am 27.06.2024 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> strchr may return NULL if colon is not found. It seems clearer to
> assert explicitly that we don't expect it here, than dereference 1 in
> the next line.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/curl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 419f7c89ef..ccfffd6c12 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size,
> size_t nmemb, void *opaque)
> && g_ascii_strncasecmp(header, accept_ranges,
> strlen(accept_ranges)) == 0) {
>
> -char *p = strchr(header, ':') + 1;
> +char *p = strchr(header, ':');
> +assert(p != NULL);
> +p += 1;
I'm not sure if this is actually much clearer because it doesn't say why
we don't expect NULL here. If you don't look at the context, it almost
looks like an assert() where proper error handling is needed. If you do,
then the original line is clear enough.
My first thought was that maybe what we want is a comment, but we
actually already know where the colon is. So how about this instead:
char *p = header + strlen(accept_ranges);
Kevin
> /* Skip whitespace between the header name and value. */
> while (p < end && *p && g_ascii_isspace(*p)) {
> --
> 2.34.1
>
[PATCH 1/3] util/cpuinfo-riscv: Support host/cpuinfo.h for riscv
Move detection code out of tcg, similar to other hosts.
Signed-off-by: Richard Henderson
---
host/include/riscv/host/cpuinfo.h | 23 +
tcg/riscv/tcg-target.h| 46 -
util/cpuinfo-riscv.c | 85 +++
tcg/riscv/tcg-target.c.inc| 84 +++---
util/meson.build | 2 +
5 files changed, 139 insertions(+), 101 deletions(-)
create mode 100644 host/include/riscv/host/cpuinfo.h
create mode 100644 util/cpuinfo-riscv.c
diff --git a/host/include/riscv/host/cpuinfo.h
b/host/include/riscv/host/cpuinfo.h
new file mode 100644
index 00..2b00660e36
--- /dev/null
+++ b/host/include/riscv/host/cpuinfo.h
@@ -0,0 +1,23 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ * Host specific cpu identification for RISC-V.
+ */
+
+#ifndef HOST_CPUINFO_H
+#define HOST_CPUINFO_H
+
+#define CPUINFO_ALWAYS (1u << 0) /* so cpuinfo is nonzero */
+#define CPUINFO_ZBA (1u << 1)
+#define CPUINFO_ZBB (1u << 2)
+#define CPUINFO_ZICOND (1u << 3)
+
+/* Initialized with a constructor. */
+extern unsigned cpuinfo;
+
+/*
+ * We cannot rely on constructor ordering, so other constructors must
+ * use the function interface rather than the variable above.
+ */
+unsigned cpuinfo_init(void);
+
+#endif /* HOST_CPUINFO_H */
diff --git a/tcg/riscv/tcg-target.h b/tcg/riscv/tcg-target.h
index 2c1b680b93..1a347eaf6e 100644
--- a/tcg/riscv/tcg-target.h
+++ b/tcg/riscv/tcg-target.h
@@ -25,6 +25,8 @@
#ifndef RISCV_TCG_TARGET_H
#define RISCV_TCG_TARGET_H
+#include "host/cpuinfo.h"
+
#define TCG_TARGET_INSN_UNIT_SIZE 4
#define TCG_TARGET_NB_REGS 32
#define MAX_CODE_GEN_BUFFER_SIZE ((size_t)-1)
@@ -80,18 +82,12 @@ typedef enum {
#define TCG_TARGET_CALL_ARG_I128TCG_CALL_ARG_NORMAL
#define TCG_TARGET_CALL_RET_I128TCG_CALL_RET_NORMAL
-#if defined(__riscv_arch_test) && defined(__riscv_zbb)
-# define have_zbb true
-#else
-extern bool have_zbb;
-#endif
-
/* optional instructions */
#define TCG_TARGET_HAS_negsetcond_i32 1
#define TCG_TARGET_HAS_div_i32 1
#define TCG_TARGET_HAS_rem_i32 1
#define TCG_TARGET_HAS_div2_i32 0
-#define TCG_TARGET_HAS_rot_i32 have_zbb
+#define TCG_TARGET_HAS_rot_i32 (cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_deposit_i32 0
#define TCG_TARGET_HAS_extract_i32 0
#define TCG_TARGET_HAS_sextract_i32 0
@@ -106,17 +102,17 @@ extern bool have_zbb;
#define TCG_TARGET_HAS_ext16s_i32 1
#define TCG_TARGET_HAS_ext8u_i321
#define TCG_TARGET_HAS_ext16u_i32 1
-#define TCG_TARGET_HAS_bswap16_i32 have_zbb
-#define TCG_TARGET_HAS_bswap32_i32 have_zbb
+#define TCG_TARGET_HAS_bswap16_i32 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_bswap32_i32 (cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_not_i32 1
-#define TCG_TARGET_HAS_andc_i32 have_zbb
-#define TCG_TARGET_HAS_orc_i32 have_zbb
-#define TCG_TARGET_HAS_eqv_i32 have_zbb
+#define TCG_TARGET_HAS_andc_i32 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_orc_i32 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_eqv_i32 (cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_nand_i32 0
#define TCG_TARGET_HAS_nor_i32 0
-#define TCG_TARGET_HAS_clz_i32 have_zbb
-#define TCG_TARGET_HAS_ctz_i32 have_zbb
-#define TCG_TARGET_HAS_ctpop_i32have_zbb
+#define TCG_TARGET_HAS_clz_i32 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_ctz_i32 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_ctpop_i32(cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_brcond2 1
#define TCG_TARGET_HAS_setcond2 1
#define TCG_TARGET_HAS_qemu_st8_i32 0
@@ -125,7 +121,7 @@ extern bool have_zbb;
#define TCG_TARGET_HAS_div_i64 1
#define TCG_TARGET_HAS_rem_i64 1
#define TCG_TARGET_HAS_div2_i64 0
-#define TCG_TARGET_HAS_rot_i64 have_zbb
+#define TCG_TARGET_HAS_rot_i64 (cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_deposit_i64 0
#define TCG_TARGET_HAS_extract_i64 0
#define TCG_TARGET_HAS_sextract_i64 0
@@ -137,18 +133,18 @@ extern bool have_zbb;
#define TCG_TARGET_HAS_ext8u_i641
#define TCG_TARGET_HAS_ext16u_i64 1
#define TCG_TARGET_HAS_ext32u_i64 1
-#define TCG_TARGET_HAS_bswap16_i64 have_zbb
-#define TCG_TARGET_HAS_bswap32_i64 have_zbb
-#define TCG_TARGET_HAS_bswap64_i64 have_zbb
+#define TCG_TARGET_HAS_bswap16_i64 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_bswap32_i64 (cpuinfo & CPUINFO_ZBB)
+#define TCG_TARGET_HAS_bswap64_i64 (cpuinfo & CPUINFO_ZBB)
#define TCG_TARGET_HAS_not_i64 1
-#define TCG_TARGET_HAS_andc_i64 have_zbb
-#define TCG_TARGET_HAS_orc_i64 have_zbb
-#define TCG_TARGET_HAS_eqv_i64 have_zbb
+#define TCG_TARGET_HAS_andc_i64
[PATCH 3/3] util/cpuinfo-riscv: Use linux __riscv_hwprobe syscall
With recent linux kernels, there is a syscall to probe for various
ISA extensions. These bits were phased in over several kernel
releases, so we still require checks for symbol availability.
Signed-off-by: Richard Henderson
---
meson.build | 6 ++
util/cpuinfo-riscv.c | 26 ++
2 files changed, 32 insertions(+)
diff --git a/meson.build b/meson.build
index 97e00d6f59..58afd0125d 100644
--- a/meson.build
+++ b/meson.build
@@ -2837,6 +2837,12 @@ have_cpuid_h = cc.links('''
}''')
config_host_data.set('CONFIG_CPUID_H', have_cpuid_h)
+# Don't bother to advertise asm/hwprobe.h for old versions that do
+# not contain RISCV_HWPROBE_EXT_ZBA.
+config_host_data.set('CONFIG_ASM_HWPROBE_H',
+ cc.has_header_symbol('asm/hwprobe.h',
+ 'RISCV_HWPROBE_EXT_ZBA'))
+
config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
.require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable
AVX2') \
.require(cc.links('''
diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c
index abf799794f..cf59ce83a3 100644
--- a/util/cpuinfo-riscv.c
+++ b/util/cpuinfo-riscv.c
@@ -6,6 +6,11 @@
#include "qemu/osdep.h"
#include "host/cpuinfo.h"
+#ifdef CONFIG_ASM_HWPROBE_H
+#include
+#include
+#endif
+
unsigned cpuinfo;
static volatile sig_atomic_t got_sigill;
@@ -47,6 +52,27 @@ unsigned __attribute__((constructor)) cpuinfo_init(void)
#endif
left &= ~info;
+#ifdef CONFIG_ASM_HWPROBE_H
+if (left) {
+/*
+ * TODO: glibc 2.40 will introduce , which
+ * provides __riscv_hwprobe and __riscv_hwprobe_one,
+ * which is a slightly cleaner interface.
+ */
+struct riscv_hwprobe pair = { .key = RISCV_HWPROBE_KEY_IMA_EXT_0 };
+if (syscall(__NR_riscv_hwprobe, , 1, 0, NULL, 0) == 0
+&& pair.key >= 0) {
+info |= pair.value & RISCV_HWPROBE_EXT_ZBA ? CPUINFO_ZBA : 0;
+info |= pair.value & RISCV_HWPROBE_EXT_ZBB ? CPUINFO_ZBB : 0;
+left &= ~(CPUINFO_ZBA | CPUINFO_ZBB);
+#ifdef RISCV_HWPROBE_EXT_ZICOND
+info |= pair.value & RISCV_HWPROBE_EXT_ZICOND ? CPUINFO_ZICOND : 0;
+left &= ~CPUINFO_ZICOND;
+#endif
+}
+}
+#endif /* CONFIG_ASM_HWPROBE_H */
+
if (left) {
struct sigaction sa_old, sa_new;
--
2.34.1
[PATCH 2/3] util/cpuinfo-riscv: Support OpenBSD signal frame
Reported-by: Brad Smith
Signed-off-by: Richard Henderson
---
util/cpuinfo-riscv.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/util/cpuinfo-riscv.c b/util/cpuinfo-riscv.c
index 6b97100620..abf799794f 100644
--- a/util/cpuinfo-riscv.c
+++ b/util/cpuinfo-riscv.c
@@ -13,7 +13,14 @@ static void sigill_handler(int signo, siginfo_t *si, void
*data)
{
/* Skip the faulty instruction */
ucontext_t *uc = (ucontext_t *)data;
+
+#ifdef __linux__
uc->uc_mcontext.__gregs[REG_PC] += 4;
+#elif defined(__OpenBSD__)
+uc->sc_sepc += 4;
+#else
+# error Unsupported OS
+#endif
got_sigill = 1;
}
--
2.34.1
[PATCH 0/3] util: Add cpuinfo support for riscv
Do cpu feature detection in util, like other hosts. Support the OpenBSD ucontext_t. Support the Linux __riscv_hwprobe syscall. r~ Richard Henderson (3): util/cpuinfo-riscv: Support host/cpuinfo.h for riscv util/cpuinfo-riscv: Support OpenBSD signal frame util/cpuinfo-riscv: Use linux __riscv_hwprobe syscall meson.build | 6 ++ host/include/riscv/host/cpuinfo.h | 23 ++ tcg/riscv/tcg-target.h| 46 ++-- util/cpuinfo-riscv.c | 118 ++ tcg/riscv/tcg-target.c.inc| 84 ++--- util/meson.build | 2 + 6 files changed, 178 insertions(+), 101 deletions(-) create mode 100644 host/include/riscv/host/cpuinfo.h create mode 100644 util/cpuinfo-riscv.c -- 2.34.1
Re: [PATCH v3] timer: Fix a race condition between timer's callback and destroying code
Paolo, thank you for your comments.
On Thu, Jun 27, 2024 at 10:16 AM Paolo Bonzini wrote:
> I think the easiest options would be:
>
> 1) if possible, allocate the timer and the callbackState statically in
> the device.
I think this assumption is not good for both QEMU and us.
> 2) use "aio_wait_bh_oneshot(qemu_get_aio_context(), [](void
> *opaque){}, NULL);" after timer_del(). You can also put the timer and
> the callbackState in a RAII wrapper, so that aio_wait_bh_oneshot() is
> executed when the RAII wrapper is destructed
My understanding is that this will work as a fence waiting for all
timers to finish. If so, maybe there is a value to put it into QEMU
(as times_joins() or even as timer_join(QEMUTimer *ts)) if one day
you decide to implement it in a more efficient way?
> Another thing that you could do is to use a shared_ptr<> for the
> timer+callbackState combo, and pass a weak_ptr<> to the timer. Then:
>
> - at the beginning of the timer, you upgrade the weak_ptr with lock()
> and if it fails, return
>
> I'm not sure how you'd pass the weak_ptr/shared_ptr to a callback
I suspect this is not possible in plain C++ without modifying QEMU or
code generating at runtime.
I would go with your aio_wait_bh_oneshot suggestion. Please consider
adding it to QEMU as I pointed above. I can send a patch.
Re: [PULL 00/32] riscv-to-apply queue
On 6/27/24 03:00, Alistair Francis wrote: The following changes since commit 3f044554b94fc0756d5b3cdbf84501e0eea0e629: Merge tag 'pull-vfio-20240624' ofhttps://github.com/legoater/qemu into staging (2024-06-24 21:30:34 -0700) are available in the Git repository at: https://github.com/alistair23/qemu.git tags/pull-riscv-to-apply-20240627-1 for you to fetch changes up to 2f5a2315b84a9b1f089ecfc3f31b29813609a7b7: target/riscv: Apply modularized matching conditions for icount trigger (2024-06-27 13:09:16 +1000) RISC-V PR for 9.1 * Extend virtual irq csrs masks to be 64 bit wide * Move Guest irqs out of the core local irqs range * zvbb implies zvkb * virt: add address-cells in create_fdt_one_aplic() * virt: add aplic nodename helper * virt: rename aplic nodename to 'interrupt-controller' * virt: aplic DT: add 'qemu, aplic' to 'compatible' * virt: aplic DT: rename prop to 'riscv, delegation' * virt: change imsic nodename to 'interrupt-controller' * virt: imsics DT: add 'qemu, imsics' to 'compatible' * virt: imsics DT: add '#msi-cells' * QEMU support for KVM Guest Debug on RISC-V * Support RISC-V privilege 1.13 spec * Add support for RISC-V ACPI tests * Modularize common match conditions for trigger Applied, thanks. Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate. r~
Re: [PATCH] hw/core/loader: allow loading larger ROMs
Hi Xingtao, > Can you reproduce this issue? Absolutely! I encountered this when trying to load an OpenSBI payload firmware using the bios option for the QEMU RISC-V virt board. These payload firmwares bundle the entire next boot stage, which in my case is a build of the Linux kernel (which is a standard configuration, supported by tools such as Buildroot [1]). My kernel (configured with the default 64-bit RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI firmware of final size 10M. Then, I run the following QEMU command: qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin and get the following output: rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392) qemu-system-riscv64: could not load firmware 'fw_payload.bin' This is from my development machine, running Arch Linux with kernel 6.9.6 and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make a minimal reproducer for this, or if you need any more information. Thanks, Gregor [1] https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95 On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu) < yaoxt.f...@fujitsu.com> wrote: > Hi, Gregor > > > > The read() syscall is not guaranteed to return all data from a file. The > > default ROM loader implementation currently does not take this into > account, > > instead failing if all bytes are not read at once. This change wraps the > > read() syscall in a do/while loop to ensure all bytes of the ROM are > read. > Can you reproduce this issue? > > Thanks > Xingtao >
Re: [PATCH] tcg/riscv: Fix building on OpenBSD/riscv64
On 6/26/24 21:27, Brad Smith wrote:
tcg/riscv: Fix building on OpenBSD/riscv64
Use ucontext_t/struct sigcontext member name from machine/signal.h
instead of Linux specific name.
Signed-off-by: Brad Smith
---
tcg/riscv/tcg-target.c.inc | 5 +
1 file changed, 5 insertions(+)
Thanks. I'm going to clean all of this up into util/cpuinfo-riscv.c, similar to other
hosts, and I will include this.
r~
diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 639363039b..aeae9a8ad8 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -2116,7 +2116,12 @@ static void sigill_handler(int signo, siginfo_t *si,
void *data)
{
/* Skip the faulty instruction */
ucontext_t *uc = (ucontext_t *)data;
+
+#ifdef __OpenBSD__
+uc->sc_sepc += 4;
+#else
uc->uc_mcontext.__gregs[REG_PC] += 4;
+#endif
got_sigill = 1;
}
Re: [PATCH v3 15/17] tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA
On 27/6/24 18:47, Thomas Huth wrote:
On 27/06/2024 18.22, Philippe Mathieu-Daudé wrote:
Disable tests using 0x4567 hardcoded RCA otherwise when
using random RCA we get:
ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread:
assertion failed: (ret == len)
not ok /arm/npcm7xx_sdhci/read_sd -
ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread:
assertion failed: (ret == len)
Bail out!
See
https://lore.kernel.org/qemu-devel/37f83be9-deb5-42a1-b704-14984351d...@linaro.org/
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Hao Wu
Cc: Shengtan Mao
Cc: Tyrone Ting
---
tests/qtest/npcm7xx_sdhci-test.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/tests/qtest/npcm7xx_sdhci-test.c
b/tests/qtest/npcm7xx_sdhci-test.c
index 5d68540e52..6a42b142ad 100644
--- a/tests/qtest/npcm7xx_sdhci-test.c
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -44,6 +44,7 @@ static QTestState *setup_sd_card(void)
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 <<
8));
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0,
SDHC_SEND_RELATIVE_ADDR);
+ g_test_skip("hardcoded 0x4567 card address");
This g_test_skip here does not make too much sense (since you're doing
it in the caller site, too) ... could you please replace it with a
proper comment why this code needs to be reworked? Thanks!
Sorry I forgot to tag "NOTFORMERGE". Ideally I'd wait Google
maintainers to fix the test so we don't need this patch. If
they don't I'll update as you suggested.
(An alternative I haven't investigated is to run the test
using the -seed argument to force deterministic mode).
Thanks!
Thomas
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
SDHC_SELECT_DESELECT_CARD);
@@ -76,6 +77,9 @@ static void test_read_sd(void)
{
QTestState *qts = setup_sd_card();
+ g_test_skip("hardcoded 0x4567 card address used in
setup_sd_card()");
+ return;
Re: [PATCH v2] util: fix building on OpenBSD/powerpc
On 6/26/24 19:22, Brad Smith wrote: util: fix building on OpenBSD/powerpc Signed-off-by: Brad Smith --- v2: Keep headers in the same order as they originally were. util/cpuinfo-ppc.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) Queued, replacing v1. r~
Re: [PATCH v3] timer: Fix a race condition between timer's callback and destroying code
On Thu, Jun 27, 2024 at 6:12 PM Roman Kiryanov wrote:
>
> On Thu, Jun 27, 2024 at 6:27 AM Paolo Bonzini wrote:
> >
> > On Thu, Jun 27, 2024 at 2:32 AM Roman Kiryanov wrote:
> > > +if (qatomic_read(>cb_running)) {
> > > +qemu_event_wait(_list->timers_done_ev);
> > > +}
> >
> > qemu_event_wait() already has the right atomic magic, and
> > ts->cb_running is both redundant (in general), and I think racy (as
> > implemented in this patch).
>
> I added cb_running to avoid waiting for timers_done_ev if we know our
> cb is done.
Yes, but it's very tricky. Assuming we want to fix it in the timer
core, the QemuEvent should be enough, no need to optimize it. On the
other hand, I'm still worried about deadlocks (more below).
> > But especially, you haven't justified in the commit message _why_ you
> > need this.
>
> I mentioned the problem of cleanup racing with the timer's callback function
> in the current shape of QEMU.
Yes, but it was not clear what are the involved threads. It is clear
now that you have a function in a separate thread, creating a timer in
the main QEMU event loop.
> > using
> > aio_bh_schedule_oneshot() or aio_wait_bh_oneshot() to synchronize
> > everything with the AioContext thread seems like a superior solution
> > to me.
>
> Could you please elaborate? The problem we want to solve is this:
>
> void myThreadFunc() {
> CallbackState callbackState;
> QEMUTimer timer;
>
> timer_init(, myClockType, myScale, ,
> );
> ...
> timer_del();
> }
>
> Currently, myTimerCallbackFunc could fire after myThreadFunc exits
> (if timer_del runs between qemu_mutex_unlock and cb(opaque) in
> timerlist_run_timers) and callbackState gets destroyed.
Ok, got it now. I agree that qemu_event_wait() is safe for you here
because you are in a completely separate thread. But I'm worried that
it causes deadlocks in QEMU where the timer callback and the timer_del
run in the same thread.
I think the easiest options would be:
1) if possible, allocate the timer and the callbackState statically in
the device.
2) use "aio_wait_bh_oneshot(qemu_get_aio_context(), [](void
*opaque){}, NULL);" after timer_del(). You can also put the timer and
the callbackState in a RAII wrapper, so that aio_wait_bh_oneshot() is
executed when the RAII wrapper is destructed
Another thing that you could do is to use a shared_ptr<> for the
timer+callbackState combo, and pass a weak_ptr<> to the timer. Then:
- at the beginning of the timer, you upgrade the weak_ptr with lock()
and if it fails, return
- at the end of myThreadfunc, you destruct the shared_ptr before
deleting the timer.
I'm not sure how you'd pass the weak_ptr/shared_ptr to a callback
(Rust has Weak::into_raw/Weak::from_raw, but I don't know C++ well
enough). That may be overkill.
Paolo
[RFC PATCH 19/19] hw/sd/sdcard: Add boot config support
From: Joel Stanley
Introduced "boot-config" property to set CSD 179, the boot config
register.
With this correctly set we can use the enable bit to detect if
partition support is enabled.
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bbf054ea1e..b598974bbf 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -129,6 +129,7 @@ struct SDState {
uint8_t spec_version;
BlockBackend *blk;
bool aspeed_emmc_kludge;
+uint8_t boot_config;
const SDProto *proto;
@@ -505,6 +506,8 @@ static void mmc_set_ext_csd(SDState *sd, uint64_t size)
sd->ext_csd[159] = 0x00; /* Max enhanced area size */
sd->ext_csd[158] = 0x00; /* ... */
sd->ext_csd[157] = 0xEC; /* ... */
+
+sd->ext_csd[EXT_CSD_PART_CONFIG] = sd->boot_config;
}
static void sd_emmc_set_csd(SDState *sd, uint64_t size)
@@ -1004,8 +1007,14 @@ static uint32_t sd_emmc_bootpart_offset(SDState *sd)
{
unsigned int access = sd->ext_csd[EXT_CSD_PART_CONFIG] &
EXT_CSD_PART_CONFIG_ACC_MASK;
+unsigned int enable = sd->ext_csd[EXT_CSD_PART_CONFIG] &
+ EXT_CSD_PART_CONFIG_EN_MASK;
unsigned int boot_capacity = sd_boot_capacity_bytes(sd);
+if (!enable) {
+return 0;
+}
+
switch (access) {
case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
return boot_capacity * 2;
@@ -2808,6 +2817,7 @@ static Property sd_properties[] = {
* whether card should be in SSI or MMC/SD mode. It is also up to the
* board to ensure that ssi transfers only occur when the chip select
* is asserted. */
+DEFINE_PROP_UINT8("boot-config", SDState, boot_config, 0x0),
DEFINE_PROP_END_OF_LIST()
};
--
2.41.0
[RFC PATCH 17/19] hw/sd/sdcard: Support boot area in emmc image
From: Joel Stanley
This assumes a specially constructed image:
dd if=/dev/zero of=mmc-bootarea.img count=2 bs=1M
dd if=u-boot-spl.bin of=mmc-bootarea.img conv=notrunc
dd if=u-boot.bin of=mmc-bootarea.img conv=notrunc count=64 bs=1K
cat mmc-bootarea.img obmc-phosphor-image.wic > mmc.img
truncate --size 16GB mmc.img
truncate --size 128MB mmc-bootarea.img
For now this still requires a mtd image to load the SPL:
qemu-system-arm -M tacoma-bmc -nographic \
-global driver=sd-card,property=emmc,value=true \
-drive file=mmc.img,if=sd,index=2 \
-drive file=mmc-bootarea.img,if=mtd,format=raw
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/sd/sd.h | 1 +
hw/sd/sd.c | 39 +++
2 files changed, 40 insertions(+)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index d35a839f5e..07435d2e17 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -132,6 +132,7 @@ struct SDCardClass {
bool (*get_readonly)(SDState *sd);
void (*set_cid)(SDState *sd);
void (*set_csd)(SDState *sd, uint64_t size);
+uint32_t (*bootpart_offset)(SDState *sd);
const struct SDProto *proto;
};
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e7d8b9c0fb..2d49be61f6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -783,6 +783,12 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
}
+
+static unsigned sd_boot_capacity_bytes(SDState *sd)
+{
+return sd->ext_csd[EXT_CSD_BOOT_MULT] << 17;
+}
+
static void sd_reset(DeviceState *dev)
{
SDState *sd = SD_CARD(dev);
@@ -984,9 +990,40 @@ void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq
insert)
qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
}
+/*
+ * This requires a disk image that has two boot partitions inserted at the
+ * beginning of it. The size of the boot partitions are configured in the
+ * ext_csd structure, which is hardcoded in qemu. They are currently set to
+ * 1MB each.
+ */
+static uint32_t sd_emmc_bootpart_offset(SDState *sd)
+{
+unsigned int access = sd->ext_csd[EXT_CSD_PART_CONFIG] &
+EXT_CSD_PART_CONFIG_ACC_MASK;
+unsigned int boot_capacity = sd_boot_capacity_bytes(sd);
+
+switch (access) {
+case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
+return boot_capacity * 2;
+case EXT_CSD_PART_CONFIG_ACC_BOOT0:
+return 0;
+case EXT_CSD_PART_CONFIG_ACC_BOOT0 + 1:
+return boot_capacity * 1;
+default:
+ g_assert_not_reached();
+}
+}
+
+static uint32_t sd_bootpart_offset(SDState *sd)
+{
+SDCardClass *sc = SD_CARD_GET_CLASS(sd);
+return sc->bootpart_offset ? sc->bootpart_offset(sd) : 0;
+}
+
static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
{
trace_sdcard_read_block(addr, len);
+addr += sd_bootpart_offset(sd);
if (!sd->blk || blk_pread(sd->blk, addr, len, sd->data, 0) < 0) {
fprintf(stderr, "sd_blk_read: read error on host side\n");
}
@@ -995,6 +1032,7 @@ static void sd_blk_read(SDState *sd, uint64_t addr,
uint32_t len)
static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
{
trace_sdcard_write_block(addr, len);
+addr += sd_bootpart_offset(sd);
if (!sd->blk || blk_pwrite(sd->blk, addr, len, sd->data, 0) < 0) {
fprintf(stderr, "sd_blk_write: write error on host side\n");
}
@@ -2833,6 +2871,7 @@ static void emmc_class_init(ObjectClass *klass, void
*data)
dc->realize = emmc_realize;
sc->proto = _proto_emmc;
sc->set_csd = sd_emmc_set_csd;
+sc->bootpart_offset = sd_emmc_bootpart_offset;
}
static const TypeInfo sd_types[] = {
--
2.41.0
[RFC PATCH 18/19] hw/sd/sdcard: Subtract bootarea size from blk
From: Joel Stanley
The userdata size is derived from the file the user passes on the
command line, but we must take into account the boot areas.
Signed-off-by: Joel Stanley
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2d49be61f6..bbf054ea1e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -804,6 +804,10 @@ static void sd_reset(DeviceState *dev)
}
size = sect << HWBLOCK_SHIFT;
+if (sc->bootpart_offset) {
+size -= sd_boot_capacity_bytes(sd) * 2;
+}
+
sect = sd_addr_to_wpnum(size) + 1;
sd->state = sd_idle_state;
--
2.41.0
[RFC PATCH 16/19] hw/sd/sdcard: Add mmc SWITCH function support (CMD6)
From: Sai Pavan Boddu
switch operation in mmc cards, updated the ext_csd register to
request changes in card operations. Here we implement similar
sequence but requests are mostly dummy and make no change.
Implement SWITCH_ERROR if the write operation offset goes beyond
length of ext_csd.
Signed-off-by: Sai Pavan Boddu
Signed-off-by: Edgar E. Iglesias
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 52
1 file changed, 52 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ae5e73175e..e7d8b9c0fb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -625,6 +625,7 @@ static bool sd_req_rca_same(SDState *s, SDRequest req)
FIELD(CSR, AKE_SEQ_ERROR, 3, 1)
FIELD(CSR, APP_CMD, 5, 1)
FIELD(CSR, FX_EVENT,6, 1)
+FIELD(CSR, SWITCH_ERROR,7, 1)
FIELD(CSR, READY_FOR_DATA, 8, 1)
FIELD(CSR, CURRENT_STATE, 9, 4)
FIELD(CSR, ERASE_RESET,13, 1)
@@ -1075,6 +1076,43 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
return ret;
}
+enum {
+MMC_CMD6_ACCESS_COMMAND_SET = 0,
+MMC_CMD6_ACCESS_SET_BITS,
+MMC_CMD6_ACCESS_CLEAR_BITS,
+MMC_CMD6_ACCESS_WRITE_BYTE,
+};
+
+static void mmc_function_switch(SDState *sd, uint32_t arg)
+{
+uint32_t access = extract32(arg, 24, 2);
+uint32_t index = extract32(arg, 16, 8);
+uint32_t value = extract32(arg, 8, 8);
+uint8_t b = sd->ext_csd[index];
+
+switch (access) {
+case MMC_CMD6_ACCESS_COMMAND_SET:
+qemu_log_mask(LOG_UNIMP, "MMC Command set switching not supported\n");
+return;
+case MMC_CMD6_ACCESS_SET_BITS:
+b |= value;
+break;
+case MMC_CMD6_ACCESS_CLEAR_BITS:
+b &= ~value;
+break;
+case MMC_CMD6_ACCESS_WRITE_BYTE:
+b = value;
+break;
+}
+
+if (index >= 192) {
+sd->card_status |= R_CSR_SWITCH_ERROR_MASK;
+return;
+}
+
+sd->ext_csd[index] = b;
+}
+
static void sd_function_switch(SDState *sd, uint32_t arg)
{
int i, mode, new_func;
@@ -1398,6 +1436,19 @@ static sd_rsp_type_t sd_cmd_SWITCH_FUNCTION(SDState *sd,
SDRequest req)
return sd_cmd_to_sendingdata(sd, req, 0, NULL, 64);
}
+static sd_rsp_type_t emmc_cmd_SWITCH(SDState *sd, SDRequest req)
+{
+switch (sd->state) {
+case sd_transfer_state:
+sd->state = sd_programming_state;
+mmc_function_switch(sd, req.arg);
+sd->state = sd_transfer_state;
+return sd_r1b;
+default:
+return sd_invalid_state_for_cmd(sd, req);
+}
+}
+
/* CMD7 */
static sd_rsp_type_t sd_cmd_DE_SELECT_CARD(SDState *sd, SDRequest req)
{
@@ -2602,6 +2653,7 @@ static const SDProto sd_proto_emmc = {
[3] = {0, sd_ac, "SET_RELATIVE_ADDR", emmc_cmd_SET_RELATIVE_ADDR},
[4] = {0, sd_bc, "SEND_DSR", sd_cmd_unimplemented},
[5] = {0, sd_ac, "SLEEP/AWAKE", emmc_cmd_sleep_awake},
+[6] = {10, sd_adtc, "SWITCH", emmc_cmd_SWITCH},
[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
[8] = {0, sd_adtc, "SEND_EXT_CSD", emmc_cmd_SEND_EXT_CSD},
[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
--
2.41.0
[RFC PATCH 15/19] hw/sd/sdcard: add emmc_cmd_SEND_TUNING_BLOCK handler (CMD21)
From: Sai Pavan Boddu
MMC cards support different tuning sequence for entering HS200 mode.
Signed-off-by: Sai Pavan Boddu
Signed-off-by: Edgar E. Iglesias
Signed-off-by: Cédric Le Goater
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdmmc-internal.h | 3 +++
hw/sd/sd.c | 41 +
2 files changed, 44 insertions(+)
diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 20d85aea6d..a2769a80aa 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -108,4 +108,7 @@
#define EXT_CSD_PART_CONFIG_EN_BOOT0(0x1 << 3)
#define EXT_CSD_PART_CONFIG_EN_USER (0x7 << 3)
+#define EXT_CSD_BUS_WIDTH_8_MASK0x4
+#define EXT_CSD_BUS_WIDTH_4_MASK0x2
+
#endif
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0561079eff..ae5e73175e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -698,6 +698,25 @@ static const uint8_t sd_tuning_block_pattern4[64] = {
0xbb, 0xff, 0xf7, 0xff, 0xf7, 0x7f, 0x7b, 0xde
};
+static const uint8_t mmc_tuning_block_pattern8[128] = {
+0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00,
+0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc, 0xcc,
+0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff, 0xff,
+0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee, 0xff,
+0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd, 0xdd,
+0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff, 0xbb,
+0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff, 0xff,
+0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee, 0xff,
+0xff, 0xff, 0xff, 0x00, 0xff, 0xff, 0xff, 0x00,
+0x00, 0xff, 0xff, 0xcc, 0xcc, 0xcc, 0x33, 0xcc,
+0xcc, 0xcc, 0x33, 0x33, 0xcc, 0xcc, 0xcc, 0xff,
+0xff, 0xff, 0xee, 0xff, 0xff, 0xff, 0xee, 0xee,
+0xff, 0xff, 0xff, 0xdd, 0xff, 0xff, 0xff, 0xdd,
+0xdd, 0xff, 0xff, 0xff, 0xbb, 0xff, 0xff, 0xff,
+0xbb, 0xbb, 0xff, 0xff, 0xff, 0x77, 0xff, 0xff,
+0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee
+};
+
static int sd_req_crc_validate(SDRequest *req)
{
uint8_t buffer[5];
@@ -1603,6 +1622,26 @@ static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState
*sd, SDRequest req)
sizeof(sd_tuning_block_pattern4));
}
+/* CMD21 */
+static sd_rsp_type_t emmc_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
+{
+const uint8_t *buf;
+size_t size;
+
+if (sd->state != sd_transfer_state) {
+sd_invalid_state_for_cmd(sd, req);
+}
+
+if (sd->ext_csd[EXT_CSD_BUS_WIDTH] & EXT_CSD_BUS_WIDTH_8_MASK) {
+buf = mmc_tuning_block_pattern8;
+size = sizeof(mmc_tuning_block_pattern8);
+} else {
+buf = sd_tuning_block_pattern4;
+size = sizeof(sd_tuning_block_pattern4);
+}
+return sd_cmd_to_sendingdata(sd, req, 0, buf, size);
+}
+
/* CMD23 */
static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd, SDRequest req)
{
@@ -2391,6 +2430,7 @@ uint8_t sd_read_byte(SDState *sd)
case 13: /* ACMD13: SD_STATUS */
case 17: /* CMD17: READ_SINGLE_BLOCK */
case 19: /* CMD19: SEND_TUNING_BLOCK (SD) */
+case 21: /* CMD21: SEND_TUNING_BLOCK (MMC) */
case 22: /* ACMD22: SEND_NUM_WR_BLOCKS */
case 30: /* CMD30: SEND_WRITE_PROT */
case 51: /* ACMD51: SEND_SCR */
@@ -2573,6 +2613,7 @@ static const SDProto sd_proto_emmc = {
[16] = {2, sd_ac, "SET_BLOCKLEN", sd_cmd_SET_BLOCKLEN},
[17] = {2, sd_adtc, "READ_SINGLE_BLOCK", sd_cmd_READ_SINGLE_BLOCK},
[19] = {0, sd_adtc, "BUSTEST_W", sd_cmd_unimplemented},
+[21] = {2, sd_adtc, "SEND_TUNING_BLOCK", emmc_cmd_SEND_TUNING_BLOCK},
[23] = {2, sd_ac, "SET_BLOCK_COUNT", sd_cmd_SET_BLOCK_COUNT},
[24] = {4, sd_adtc, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
[26] = {4, sd_adtc, "PROGRAM_CID", mmc_cmd_PROGRAM_CID},
--
2.41.0
[RFC PATCH 14/19] hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8)
From: Vincent Palatin
The parameters mimick a real 4GB eMMC, but it can be set to various
sizes. Initially from Vincent Palatin
eMMC CSD is similar to SD with an option to refer EXT_CSD for larger
devices.
Signed-off-by: Vincent Palatin
Signed-off-by: Cédric Le Goater
Signed-off-by: Sai Pavan Boddu
Signed-off-by: Edgar E. Iglesias
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
TODO simplify params, see:
https://lore.kernel.org/qemu-devel/54bc25fd-acea-44a3-b696-c261e7e97...@kaod.org/
---
hw/sd/sd.c | 91 ++
1 file changed, 91 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 82e0b5838f..0561079eff 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -122,6 +122,7 @@ struct SDState {
uint16_t rca;
uint32_t card_status;
uint8_t sd_status[64];
+uint8_t ext_csd[512];
/* Static properties */
@@ -460,6 +461,82 @@ static const uint8_t sd_csd_rw_mask[16] = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
};
+static void mmc_set_ext_csd(SDState *sd, uint64_t size)
+{
+uint32_t sectcount = size >> HWBLOCK_SHIFT;
+
+memset(sd->ext_csd, 0, sizeof(sd->ext_csd));
+
+sd->ext_csd[EXT_CSD_S_CMD_SET] = 0x1; /* supported command sets */
+sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features */
+sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background operations */
+sd->ext_csd[241] = 0xA; /* 1st initialization time after partitioning */
+sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
+sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */
+sd->ext_csd[EXT_CSD_SEC_ERASE_MULT] = 0x96; /* Secure erase support */
+sd->ext_csd[EXT_CSD_SEC_TRIM_MULT] = 0x96; /* Secure TRIM multiplier */
+sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
+sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 128KB unit */
+sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */
+sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */
+sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x1; /* HC erase timeout */
+sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write sector count */
+sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect group size
*/
+sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC */
+sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */
+sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */
+sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */
+sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */
+sd->ext_csd[213] = (sectcount >> 8) & 0xff; /* ... */
+sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff); /* ... */
+sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */
+sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz */
+sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */
+sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */
+sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */
+sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */
+sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1;
+sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1;
+sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7;
+sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2;
+sd->ext_csd[EXT_CSD_REV] = 0x5;
+sd->ext_csd[EXT_CSD_RPMB_MULT] = 0x1; /* RPMB size */
+sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
+sd->ext_csd[159] = 0x00; /* Max enhanced area size */
+sd->ext_csd[158] = 0x00; /* ... */
+sd->ext_csd[157] = 0xEC; /* ... */
+}
+
+static void sd_emmc_set_csd(SDState *sd, uint64_t size)
+{
+sd->csd[0] = 0xd0;
+sd->csd[1] = 0x0f;
+sd->csd[2] = 0x00;
+sd->csd[3] = 0x32;
+sd->csd[4] = 0x0f;
+if (size <= 2 * GiB) {
+/* use 1k blocks */
+uint32_t csize1k = (size >> (CMULT_SHIFT + 10)) - 1;
+sd->csd[5] = 0x5a;
+sd->csd[6] = 0x80 | ((csize1k >> 10) & 0xf);
+sd->csd[7] = (csize1k >> 2) & 0xff;
+} else { /* >= 2GB : size stored in ext CSD, block addressing */
+sd->csd[5] = 0x59;
+sd->csd[6] = 0x8f;
+sd->csd[7] = 0xff;
+sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_CAPACITY, 1);
+}
+sd->csd[8] = 0xff;
+sd->csd[9] = 0xff;
+sd->csd[10] = 0xf7;
+sd->csd[11] = 0xfe;
+sd->csd[12] = 0x49;
+sd->csd[13] = 0x10;
+sd->csd[14] = 0x00;
+sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
+mmc_set_ext_csd(sd, size);
+}
+
static void sd_set_csd(SDState *sd, uint64_t size)
{
int hwblock_shift = HWBLOCK_SHIFT;
@@ -1364,6 +1441,17 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd,
SDRequest req)
return sd_r7;
}
+/* CMD8 */
+static sd_rsp_type_t emmc_cmd_SEND_EXT_CSD(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+return sd_cmd_to_sendingdata(sd,
[RFC PATCH 12/19] hw/sd/sdcard: Adapt sd_cmd_ALL_SEND_CID handler for eMMC (CMD2)
From: Cédric Le Goater
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
[PMD: Use aspeed_emmc_kludge]
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index dc692fe1fa..f875fcd741 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1218,7 +1218,13 @@ static sd_rsp_type_t emmc_cmd_SEND_OP_COND(SDState *sd,
SDRequest req)
static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
{
switch (sd->state) {
+case sd_idle_state:
+if (!sd->aspeed_emmc_kludge) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+/* fall-through */
case sd_ready_state:
+/* Bus always won */
sd->state = sd_identification_state;
return sd_r2_i;
default:
--
2.41.0
[RFC PATCH 13/19] hw/sd/sdcard: Adapt sd_cmd_APP_CMD handler for eMMC (CMD55)
From: Cédric Le Goater
Signed-off-by: Cédric Le Goater
[PMD: Use aspeed_emmc_kludge]
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f875fcd741..82e0b5838f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1693,6 +1693,9 @@ static sd_rsp_type_t sd_cmd_APP_CMD(SDState *sd,
SDRequest req)
case sd_sleep_state:
return sd_invalid_state_for_cmd(sd, req);
case sd_idle_state:
+if (sd_is_emmc(sd)) {
+return sd_invalid_state_for_cmd(sd, req);
+}
if (!sd_is_spi(sd) && sd_req_get_rca(sd, req) != 0x) {
qemu_log_mask(LOG_GUEST_ERROR,
"SD: illegal RCA 0x%04x for APP_CMD\n", req.cmd);
@@ -1707,7 +1710,7 @@ static sd_rsp_type_t sd_cmd_APP_CMD(SDState *sd,
SDRequest req)
sd->expecting_acmd = true;
sd->card_status |= APP_CMD;
-return sd_r1;
+return sd->aspeed_emmc_kludge ? sd_r0 : sd_r1;
}
/* CMD56 */
--
2.41.0
[PATCH 10/19] hw/sd/sdcard: Implement eMMC sleep state (CMD5)
From: Luc Michel
The JEDEC standards specifies a sleep state where the eMMC won't answer
any command appart from RESET and WAKEUP and go to low power state.
Implement this state and the corresponding command number 5.
Signed-off-by: Luc Michel
Signed-off-by: Francisco Iglesias
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 47 +--
1 file changed, 45 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0f9bab105e..bd77853419 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1182,8 +1182,19 @@ static sd_rsp_type_t sd_cmd_to_sendingdata(SDState *sd,
SDRequest req,
/* CMD0 */
static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
{
-sd->state = sd_idle_state;
-sd_reset(DEVICE(sd));
+if (sd->state == sd_sleep_state) {
+switch (req.arg) {
+case 0x:
+case 0xf0f0f0f0:
+break;
+default:
+return sd_r0;
+}
+}
+if (sd->state != sd_inactive_state) {
+sd->state = sd_idle_state;
+sd_reset(DEVICE(sd));
+}
return sd_is_spi(sd) ? sd_r1 : sd_r0;
}
@@ -1246,6 +1257,30 @@ static sd_rsp_type_t emmc_cmd_SET_RELATIVE_ADDR(SDState
*sd, SDRequest req)
}
}
+/* CMD5 */
+static sd_rsp_type_t emmc_cmd_sleep_awake(SDState *sd, SDRequest req)
+{
+bool do_sleep = extract32(req.arg, 15, 1);
+
+switch (sd->state) {
+case sd_sleep_state:
+if (!do_sleep) {
+/* Awake */
+sd->state = sd_standby_state;
+}
+return sd_r1b;
+
+case sd_standby_state:
+if (do_sleep) {
+sd->state = sd_sleep_state;
+}
+return sd_r1b;
+
+default:
+return sd_invalid_state_for_cmd(sd, req);
+}
+}
+
/* CMD6 */
static sd_rsp_type_t sd_cmd_SWITCH_FUNCTION(SDState *sd, SDRequest req)
{
@@ -1648,6 +1683,7 @@ static sd_rsp_type_t sd_cmd_APP_CMD(SDState *sd,
SDRequest req)
case sd_ready_state:
case sd_identification_state:
case sd_inactive_state:
+case sd_sleep_state:
return sd_invalid_state_for_cmd(sd, req);
case sd_idle_state:
if (!sd_is_spi(sd) && sd_req_get_rca(sd, req) != 0x) {
@@ -1969,6 +2005,12 @@ int sd_do_command(SDState *sd, SDRequest *req,
req->cmd &= 0x3f;
}
+if (sd->state == sd_sleep_state && req->cmd) {
+qemu_log_mask(LOG_GUEST_ERROR, "SD: Card is sleeping\n");
+rtype = sd_r0;
+goto send_response;
+}
+
if (sd->card_status & CARD_IS_LOCKED) {
if (!cmd_valid_while_locked(sd, req->cmd)) {
sd->card_status |= ILLEGAL_COMMAND;
@@ -2420,6 +2462,7 @@ static const SDProto sd_proto_emmc = {
[2] = {0, sd_bcr, "ALL_SEND_CID", sd_cmd_ALL_SEND_CID},
[3] = {0, sd_ac, "SET_RELATIVE_ADDR", emmc_cmd_SET_RELATIVE_ADDR},
[4] = {0, sd_bc, "SEND_DSR", sd_cmd_unimplemented},
+[5] = {0, sd_ac, "SLEEP/AWAKE", emmc_cmd_sleep_awake},
[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
[10] = {0, sd_ac, "SEND_CID", sd_cmd_SEND_CID},
--
2.41.0
[RFC PATCH 11/19] hw/sd/sdcard: Add experimental 'x-aspeed-emmc-kludge' property
When booting U-boot/Linux on Aspeed boards via eMMC,
some commands don't behave as expected from the spec.
Add the 'x-aspeed-emmc-kludge' property to allow non
standard uses until we figure out the reasons.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bd77853419..dc692fe1fa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -127,6 +127,7 @@ struct SDState {
uint8_t spec_version;
BlockBackend *blk;
+bool aspeed_emmc_kludge;
const SDProto *proto;
@@ -2567,6 +2568,8 @@ static Property sd_properties[] = {
DEFINE_PROP_UINT8("spec_version", SDState,
spec_version, SD_PHY_SPECv3_01_VERS),
DEFINE_PROP_DRIVE("drive", SDState, blk),
+DEFINE_PROP_BOOL("x-aspeed-emmc-kludge", SDState,
+ aspeed_emmc_kludge, false),
/* We do not model the chip select pin, so allow the board to select
* whether card should be in SSI or MMC/SD mode. It is also up to the
* board to ensure that ssi transfers only occur when the chip select
--
2.41.0
[PATCH 09/19] hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7c6f5ccc72..0f9bab105e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1520,6 +1520,12 @@ static sd_rsp_type_t sd_cmd_WRITE_SINGLE_BLOCK(SDState
*sd, SDRequest req)
return sd_cmd_to_receivingdata(sd, req, addr, sd->blk_len);
}
+/* CMD26 */
+static sd_rsp_type_t mmc_cmd_PROGRAM_CID(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
+}
+
/* CMD27 */
static sd_rsp_type_t sd_cmd_PROGRAM_CSD(SDState *sd, SDRequest req)
{
@@ -1868,9 +1874,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
}
break;
-case 26: /* CMD26: PROGRAM_CID */
-return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-
default:
qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
return sd_illegal;
@@ -2429,6 +2432,7 @@ static const SDProto sd_proto_emmc = {
[19] = {0, sd_adtc, "BUSTEST_W", sd_cmd_unimplemented},
[23] = {2, sd_ac, "SET_BLOCK_COUNT", sd_cmd_SET_BLOCK_COUNT},
[24] = {4, sd_adtc, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
+[26] = {4, sd_adtc, "PROGRAM_CID", mmc_cmd_PROGRAM_CID},
[27] = {4, sd_adtc, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[28] = {6, sd_ac, "SET_WRITE_PROT", sd_cmd_SET_WRITE_PROT},
[29] = {6, sd_ac, "CLR_WRITE_PROT", sd_cmd_CLR_WRITE_PROT},
--
2.41.0
[PATCH 05/19] hw/sd/sdcard: Register unimplemented command handlers
Per the spec v4.5 these commands are mandatory,
but we don't implement them.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ebcd8c1e43..9a2bfeaab6 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2385,24 +2385,30 @@ static const SDProto sd_proto_emmc = {
.cmd = {
[0] = {0, sd_bc, "GO_IDLE_STATE", sd_cmd_GO_IDLE_STATE},
[2] = {0, sd_bcr, "ALL_SEND_CID", sd_cmd_ALL_SEND_CID},
+[4] = {0, sd_bc, "SEND_DSR", sd_cmd_unimplemented},
[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
[10] = {0, sd_ac, "SEND_CID", sd_cmd_SEND_CID},
[12] = {0, sd_ac, "STOP_TRANSMISSION", sd_cmd_STOP_TRANSMISSION},
[13] = {0, sd_ac, "SEND_STATUS", sd_cmd_SEND_STATUS},
+[14] = {0, sd_adtc, "BUSTEST_R", sd_cmd_unimplemented},
[15] = {0, sd_ac, "GO_INACTIVE_STATE", sd_cmd_GO_INACTIVE_STATE},
[16] = {2, sd_ac, "SET_BLOCKLEN", sd_cmd_SET_BLOCKLEN},
[17] = {2, sd_adtc, "READ_SINGLE_BLOCK", sd_cmd_READ_SINGLE_BLOCK},
+[19] = {0, sd_adtc, "BUSTEST_W", sd_cmd_unimplemented},
[23] = {2, sd_ac, "SET_BLOCK_COUNT", sd_cmd_SET_BLOCK_COUNT},
[24] = {4, sd_adtc, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
[27] = {4, sd_adtc, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[28] = {6, sd_ac, "SET_WRITE_PROT", sd_cmd_SET_WRITE_PROT},
[29] = {6, sd_ac, "CLR_WRITE_PROT", sd_cmd_CLR_WRITE_PROT},
[30] = {6, sd_adtc, "SEND_WRITE_PROT", sd_cmd_SEND_WRITE_PROT},
+[31] = {6, sd_adtc, "SEND_WRITE_PROT_TYPE", sd_cmd_unimplemented},
[35] = {5, sd_ac, "ERASE_WR_BLK_START", sd_cmd_ERASE_WR_BLK_START},
[36] = {5, sd_ac, "ERASE_WR_BLK_END", sd_cmd_ERASE_WR_BLK_END},
[38] = {5, sd_ac, "ERASE", sd_cmd_ERASE},
+[39] = {9, sd_ac, "FAST_IO", sd_cmd_unimplemented},
[42] = {7, sd_adtc, "LOCK_UNLOCK", sd_cmd_LOCK_UNLOCK},
+[49] = {0, sd_adtc, "SET_TIME", sd_cmd_unimplemented},
[55] = {8, sd_ac, "APP_CMD", sd_cmd_APP_CMD},
[56] = {8, sd_adtc, "GEN_CMD", sd_cmd_GEN_CMD},
},
--
2.41.0
[PATCH 03/19] hw/sd/sdcard: Basis for eMMC support
From: Cédric Le Goater
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/sd/sd.h | 3 +++
hw/sd/sd.c | 34 ++
2 files changed, 37 insertions(+)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 0d6d9e452b..d35a839f5e 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -96,6 +96,9 @@ OBJECT_DECLARE_TYPE(SDState, SDCardClass, SD_CARD)
#define TYPE_SD_CARD_SPI "sd-card-spi"
DECLARE_INSTANCE_CHECKER(SDState, SD_CARD_SPI, TYPE_SD_CARD_SPI)
+#define TYPE_EMMC "emmc"
+DECLARE_INSTANCE_CHECKER(SDState, EMMC, TYPE_EMMC)
+
struct SDCardClass {
/*< private >*/
DeviceClass parent_class;
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 92ac57a648..249fad0468 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2379,6 +2379,13 @@ static const SDProto sd_proto_sd = {
},
};
+static const SDProto sd_proto_emmc = {
+/* Only v4.5 is supported */
+.name = "eMMC",
+.cmd = {
+},
+};
+
static void sd_instance_init(Object *obj)
{
SDState *sd = SD_CARD(obj);
@@ -2504,6 +2511,28 @@ static void sd_spi_class_init(ObjectClass *klass, void
*data)
sc->proto = _proto_spi;
}
+static void emmc_realize(DeviceState *dev, Error **errp)
+{
+SDState *sd = SD_CARD(dev);
+
+if (sd->spec_version == SD_PHY_SPECv2_00_VERS) {
+error_setg(errp, "eMMC can not use spec v2.00");
+return;
+}
+
+sd_realize(dev, errp);
+}
+
+static void emmc_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+SDCardClass *sc = SD_CARD_CLASS(klass);
+
+dc->desc = "eMMC";
+dc->realize = emmc_realize;
+sc->proto = _proto_emmc;
+}
+
static const TypeInfo sd_types[] = {
{
.name = TYPE_SD_CARD,
@@ -2519,6 +2548,11 @@ static const TypeInfo sd_types[] = {
.parent = TYPE_SD_CARD,
.class_init = sd_spi_class_init,
},
+{
+.name = TYPE_EMMC,
+.parent = TYPE_SD_CARD,
+.class_init = emmc_class_init,
+},
};
DEFINE_TYPES(sd_types)
--
2.41.0
[PATCH 06/19] hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR() handler
From: Cédric Le Goater
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9a2bfeaab6..c6e5c93acb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1220,6 +1220,20 @@ static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState
*sd, SDRequest req)
}
}
+static sd_rsp_type_t emmc_cmd_SET_RELATIVE_ADDR(SDState *sd, SDRequest req)
+{
+switch (sd->state) {
+case sd_identification_state:
+case sd_standby_state:
+sd->state = sd_standby_state;
+sd_set_rca(sd, req.arg >> 16);
+return sd_r1;
+
+default:
+return sd_invalid_state_for_cmd(sd, req);
+}
+}
+
/* CMD6 */
static sd_rsp_type_t sd_cmd_SWITCH_FUNCTION(SDState *sd, SDRequest req)
{
@@ -2385,6 +2399,7 @@ static const SDProto sd_proto_emmc = {
.cmd = {
[0] = {0, sd_bc, "GO_IDLE_STATE", sd_cmd_GO_IDLE_STATE},
[2] = {0, sd_bcr, "ALL_SEND_CID", sd_cmd_ALL_SEND_CID},
+[3] = {0, sd_ac, "SET_RELATIVE_ADDR", emmc_cmd_SET_RELATIVE_ADDR},
[4] = {0, sd_bc, "SEND_DSR", sd_cmd_unimplemented},
[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
--
2.41.0
[PATCH 08/19] hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23)
From: Cédric Le Goater
The number of blocks is defined in the lower bits [15:0].
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2dc0209482..7c6f5ccc72 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -171,12 +171,18 @@ struct SDState {
static void sd_realize(DeviceState *dev, Error **errp);
static const SDProto sd_proto_spi;
+static const SDProto sd_proto_emmc;
static bool sd_is_spi(SDState *sd)
{
return sd->proto == _proto_spi;
}
+static bool sd_is_emmc(SDState *sd)
+{
+return sd->proto == _proto_emmc;
+}
+
static const char *sd_version_str(enum SDPhySpecificationVersion version)
{
static const char *sdphy_version[] = {
@@ -1479,6 +1485,9 @@ static sd_rsp_type_t sd_cmd_SET_BLOCK_COUNT(SDState *sd,
SDRequest req)
}
sd->multi_blk_cnt = req.arg;
+if (sd_is_emmc(sd)) {
+sd->multi_blk_cnt &= 0x;
+}
trace_sdcard_set_block_count(sd->multi_blk_cnt);
return sd_r1;
--
2.41.0
[PATCH 04/19] hw/sd/sdcard: Register generic command handlers
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 249fad0468..ebcd8c1e43 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2383,6 +2383,28 @@ static const SDProto sd_proto_emmc = {
/* Only v4.5 is supported */
.name = "eMMC",
.cmd = {
+[0] = {0, sd_bc, "GO_IDLE_STATE", sd_cmd_GO_IDLE_STATE},
+[2] = {0, sd_bcr, "ALL_SEND_CID", sd_cmd_ALL_SEND_CID},
+[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
+[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
+[10] = {0, sd_ac, "SEND_CID", sd_cmd_SEND_CID},
+[12] = {0, sd_ac, "STOP_TRANSMISSION", sd_cmd_STOP_TRANSMISSION},
+[13] = {0, sd_ac, "SEND_STATUS", sd_cmd_SEND_STATUS},
+[15] = {0, sd_ac, "GO_INACTIVE_STATE", sd_cmd_GO_INACTIVE_STATE},
+[16] = {2, sd_ac, "SET_BLOCKLEN", sd_cmd_SET_BLOCKLEN},
+[17] = {2, sd_adtc, "READ_SINGLE_BLOCK", sd_cmd_READ_SINGLE_BLOCK},
+[23] = {2, sd_ac, "SET_BLOCK_COUNT", sd_cmd_SET_BLOCK_COUNT},
+[24] = {4, sd_adtc, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
+[27] = {4, sd_adtc, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
+[28] = {6, sd_ac, "SET_WRITE_PROT", sd_cmd_SET_WRITE_PROT},
+[29] = {6, sd_ac, "CLR_WRITE_PROT", sd_cmd_CLR_WRITE_PROT},
+[30] = {6, sd_adtc, "SEND_WRITE_PROT", sd_cmd_SEND_WRITE_PROT},
+[35] = {5, sd_ac, "ERASE_WR_BLK_START", sd_cmd_ERASE_WR_BLK_START},
+[36] = {5, sd_ac, "ERASE_WR_BLK_END", sd_cmd_ERASE_WR_BLK_END},
+[38] = {5, sd_ac, "ERASE", sd_cmd_ERASE},
+[42] = {7, sd_adtc, "LOCK_UNLOCK", sd_cmd_LOCK_UNLOCK},
+[55] = {8, sd_ac, "APP_CMD", sd_cmd_APP_CMD},
+[56] = {8, sd_adtc, "GEN_CMD", sd_cmd_GEN_CMD},
},
};
--
2.41.0
[PATCH 07/19] hw/sd/sdcard: Add emmc_cmd_SEND_OP_COND handler (CMD1)
From: Sai Pavan Boddu
Add support to Power up the card and send response r3 in case of MMC.
Signed-off-by: Sai Pavan Boddu
Signed-off-by: Edgar E. Iglesias
Signed-off-by: Cédric Le Goater
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c6e5c93acb..2dc0209482 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1190,6 +1190,12 @@ static sd_rsp_type_t spi_cmd_SEND_OP_COND(SDState *sd,
SDRequest req)
return sd_r1;
}
+static sd_rsp_type_t emmc_cmd_SEND_OP_COND(SDState *sd, SDRequest req)
+{
+sd_ocr_powerup(sd);
+return sd->state == sd_idle_state ? sd_r3 : sd_r0;
+}
+
/* CMD2 */
static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
{
@@ -2398,6 +2404,7 @@ static const SDProto sd_proto_emmc = {
.name = "eMMC",
.cmd = {
[0] = {0, sd_bc, "GO_IDLE_STATE", sd_cmd_GO_IDLE_STATE},
+[1] = {0, sd_bcr, "SEND_OP_COND", emmc_cmd_SEND_OP_COND},
[2] = {0, sd_bcr, "ALL_SEND_CID", sd_cmd_ALL_SEND_CID},
[3] = {0, sd_ac, "SET_RELATIVE_ADDR", emmc_cmd_SET_RELATIVE_ADDR},
[4] = {0, sd_bc, "SEND_DSR", sd_cmd_unimplemented},
--
2.41.0
[PATCH 02/19] hw/sd/sdcard: Cover more SDCardStates
So far eMMC will only use sd_sleep_state, but
all all states specified for completeness.
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b0ef252001..92ac57a648 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -76,7 +76,9 @@ enum SDCardModes {
};
enum SDCardStates {
+sd_waitirq_state= -2, /* emmc */
sd_inactive_state = -1,
+
sd_idle_state = 0,
sd_ready_state = 1,
sd_identification_state = 2,
@@ -86,6 +88,9 @@ enum SDCardStates {
sd_receivingdata_state = 6,
sd_programming_state= 7,
sd_disconnect_state = 8,
+sd_bus_test_state = 9, /* emmc */
+sd_sleep_state = 10, /* emmc */
+sd_io_state = 15 /* sd */
};
#define SDMMC_CMD_MAX 64
@@ -205,13 +210,19 @@ static const char *sd_state_name(enum SDCardStates state)
[sd_standby_state] = "standby",
[sd_transfer_state] = "transfer",
[sd_sendingdata_state] = "sendingdata",
+[sd_bus_test_state] = "bus-test",
[sd_receivingdata_state]= "receivingdata",
[sd_programming_state] = "programming",
[sd_disconnect_state] = "disconnect",
+[sd_sleep_state]= "sleep",
+[sd_io_state] = "i/o"
};
if (state == sd_inactive_state) {
return "inactive";
}
+if (state == sd_waitirq_state) {
+return "wait-irq";
+}
assert(state < ARRAY_SIZE(state_name));
return state_name[state];
}
--
2.41.0
[PATCH 00/19] hw/sd: Add support for eMMC cards
Trying to land what Vincent Palatin started 13 years ago! https://lore.kernel.org/qemu-devel/1311635951-11047-5-git-send-email-vpala...@chromium.org/ with fixes from Sai, Joel and Cédric. I reviewed Cédric patches but still plan to review the EXT_CSD layout and Joel "boot config" patches. In order to avoid deviation with the spec, the experimental 'x-aspeed-emmc-kludge' property is used. Based-on: <20240627164815.82606-1-phi...@linaro.org> Full series for testing: https://gitlab.com/philmd/qemu/-/tags/emmc-v4 Including Aspeed branch from Cédric on top (aspeed-9.1): https://gitlab.com/philmd/qemu/-/tags/aspeed_emmc-v4 Cédric Le Goater (5): hw/sd/sdcard: Basis for eMMC support hw/sd/sdcard: Add emmc_cmd_SET_RELATIVE_ADDR() handler hw/sd/sdcard: Fix SET_BLOCK_COUNT command argument on eMMC (CMD23) hw/sd/sdcard: Adapt sd_cmd_ALL_SEND_CID handler for eMMC (CMD2) hw/sd/sdcard: Adapt sd_cmd_APP_CMD handler for eMMC (CMD55) Joel Stanley (3): hw/sd/sdcard: Support boot area in emmc image hw/sd/sdcard: Subtract bootarea size from blk hw/sd/sdcard: Add boot config support Luc Michel (1): hw/sd/sdcard: Implement eMMC sleep state (CMD5) Philippe Mathieu-Daudé (6): hw/sd/sdcard: Introduce set_csd/set_cid handlers hw/sd/sdcard: Cover more SDCardStates hw/sd/sdcard: Register generic command handlers hw/sd/sdcard: Register unimplemented command handlers hw/sd/sdcard: Add mmc_cmd_PROGRAM_CID handler (CMD26) hw/sd/sdcard: Add experimental 'x-aspeed-emmc-kludge' property Sai Pavan Boddu (3): hw/sd/sdcard: Add emmc_cmd_SEND_OP_COND handler (CMD1) hw/sd/sdcard: add emmc_cmd_SEND_TUNING_BLOCK handler (CMD21) hw/sd/sdcard: Add mmc SWITCH function support (CMD6) Vincent Palatin (1): hw/sd/sdcard: Add emmc_cmd_SEND_EXT_CSD handler (CMD8) hw/sd/sdmmc-internal.h | 3 + include/hw/sd/sd.h | 6 + hw/sd/sd.c | 419 - 3 files changed, 420 insertions(+), 8 deletions(-) -- 2.41.0
[PATCH 01/19] hw/sd/sdcard: Introduce set_csd/set_cid handlers
In preparation of introducing eMMC support which have
different CSD/CID structures, introduce a pair of handlers
in SDCardClass.
Signed-off-by: Philippe Mathieu-Daudé
---
include/hw/sd/sd.h | 2 ++
hw/sd/sd.c | 7 +--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index c1a35ab420..0d6d9e452b 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -127,6 +127,8 @@ struct SDCardClass {
void (*enable)(SDState *sd, bool enable);
bool (*get_inserted)(SDState *sd);
bool (*get_readonly)(SDState *sd);
+void (*set_cid)(SDState *sd);
+void (*set_csd)(SDState *sd, uint64_t size);
const struct SDProto *proto;
};
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a0da06e017..b0ef252001 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -671,6 +671,7 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
static void sd_reset(DeviceState *dev)
{
SDState *sd = SD_CARD(dev);
+SDCardClass *sc = SD_CARD_GET_CLASS(sd);
uint64_t size;
uint64_t sect;
@@ -691,8 +692,8 @@ static void sd_reset(DeviceState *dev)
sd->size = size;
sd_set_ocr(sd);
sd_set_scr(sd);
-sd_set_cid(sd);
-sd_set_csd(sd, size);
+sc->set_cid(sd);
+sc->set_csd(sd, size);
sd_set_cardstatus(sd);
sd_set_sdstatus(sd);
@@ -2472,6 +2473,8 @@ static void sd_class_init(ObjectClass *klass, void *data)
sc->enable = sd_enable;
sc->get_inserted = sd_get_inserted;
sc->get_readonly = sd_get_readonly;
+sc->set_cid = sd_set_cid;
+sc->set_csd = sd_set_csd;
sc->proto = _proto_sd;
}
--
2.41.0
[PATCH 19/21] hw/sd/sdcard: Add sd_cmd_GEN_CMD handler (CMD56)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 36 +---
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5461e56e17..50cee5ac40 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -245,7 +245,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[25]= "WRITE_MULTIPLE_BLOCK",
[26]= "MANUF_RSVD",
[40]= "DPS_spec",
-[56]= "GEN_CMD",
[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
[62]= "MANUF_RSVD", [63]= "MANUF_RSVD",
};
@@ -910,9 +909,6 @@ static void sd_blk_write(SDState *sd, uint64_t addr,
uint32_t len)
}
}
-#define APP_READ_BLOCK(a, len) memset(sd->data, 0xec, len)
-#define APP_WRITE_BLOCK(a, len)
-
static void sd_erase(SDState *sd)
{
uint64_t erase_start = sd->erase_start;
@@ -1649,6 +1645,21 @@ static sd_rsp_type_t sd_cmd_APP_CMD(SDState *sd,
SDRequest req)
return sd_r1;
}
+/* CMD56 */
+static sd_rsp_type_t sd_cmd_GEN_CMD(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+if (req.arg & 1) {
+return sd_cmd_to_sendingdata(sd, req, 0,
+ sd->vendor_data, sizeof(sd->vendor_data));
+} else {
+return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->vendor_data));
+}
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1737,21 +1748,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
/* Application specific commands (Class 8) */
-case 56: /* CMD56: GEN_CMD */
-switch (sd->state) {
-case sd_transfer_state:
-if (req.arg & 1) {
-return sd_cmd_to_sendingdata(sd, req, 0,
- sd->vendor_data,
- sizeof(sd->vendor_data));
-}
-return sd_cmd_to_receivingdata(sd, req, 0,
sizeof(sd->vendor_data));
-
-default:
-break;
-}
-break;
-
case 58:/* CMD58: READ_OCR (SPI) */
return sd_r3;
@@ -2323,6 +2319,7 @@ static const SDProto sd_proto_spi = {
[52] = {9, sd_spi, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_spi, "IO_RW_EXTENDED", sd_cmd_optional},
[55] = {8, sd_spi, "APP_CMD", sd_cmd_APP_CMD},
+[56] = {8, sd_spi, "GEN_CMD", sd_cmd_GEN_CMD},
[57] = {10, sd_spi, "DIRECT_SECURE_WRITE", sd_cmd_optional},
},
.acmd = {
@@ -2376,6 +2373,7 @@ static const SDProto sd_proto_sd = {
[52] = {9, sd_bc, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_bc, "IO_RW_EXTENDED", sd_cmd_optional},
[55] = {8, sd_ac, "APP_CMD", sd_cmd_APP_CMD},
+[56] = {8, sd_adtc, "GEN_CMD", sd_cmd_GEN_CMD},
[57] = {10, sd_adtc, "DIRECT_SECURE_WRITE", sd_cmd_optional},
[58] = {11, sd_adtc, "READ_EXTR_MULTI", sd_cmd_optional},
[59] = {11, sd_adtc, "WRITE_EXTR_MULTI", sd_cmd_optional},
--
2.41.0
[PATCH 01/12] hw/sd/sdcard: Add sd_acmd_SET_BUS_WIDTH handler (ACMD6)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2f853a89d1..0310a5a3a1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -260,7 +260,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
static const char *sd_acmd_name(SDState *sd, uint8_t cmd)
{
static const char *acmd_abbrev[SDMMC_CMD_MAX] = {
- [6] = "SET_BUS_WIDTH",
[13] = "SD_STATUS",
[14] = "DPS_spec", [15] = "DPS_spec",
[16] = "DPS_spec",
@@ -1672,6 +1671,18 @@ static sd_rsp_type_t spi_cmd_CRC_ON_OFF(SDState *sd,
SDRequest req)
return sd_r1;
}
+/* ACMD6 */
+static sd_rsp_type_t sd_acmd_SET_BUS_WIDTH(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+sd->sd_status[0] &= 0x3f;
+sd->sd_status[0] |= (req.arg & 0x03) << 6;
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1780,18 +1791,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
}
switch (req.cmd) {
-case 6: /* ACMD6: SET_BUS_WIDTH */
-switch (sd->state) {
-case sd_transfer_state:
-sd->sd_status[0] &= 0x3f;
-sd->sd_status[0] |= (req.arg & 0x03) << 6;
-return sd_r1;
-
-default:
-break;
-}
-break;
-
case 13: /* ACMD13: SD_STATUS */
switch (sd->state) {
case sd_transfer_state:
@@ -2385,6 +2384,9 @@ static const SDProto sd_proto_sd = {
[58] = {11, sd_adtc, "READ_EXTR_MULTI", sd_cmd_optional},
[59] = {11, sd_adtc, "WRITE_EXTR_MULTI", sd_cmd_optional},
},
+.acmd = {
+[6] = {8, sd_ac, "SET_BUS_WIDTH", sd_acmd_SET_BUS_WIDTH},
+},
};
static void sd_instance_init(Object *obj)
--
2.41.0
[PATCH 21/21] hw/sd/sdcard: Add spi_cmd_CRC_ON_OFF handler (CMD59)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b3b4cd5a3a..2f853a89d1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1666,6 +1666,12 @@ static sd_rsp_type_t spi_cmd_READ_OCR(SDState *sd,
SDRequest req)
return sd_r3;
}
+/* CMD59 */
+static sd_rsp_type_t spi_cmd_CRC_ON_OFF(SDState *sd, SDRequest req)
+{
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1753,10 +1759,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 26: /* CMD26: PROGRAM_CID */
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-/* Application specific commands (Class 8) */
-case 59:/* CMD59: CRC_ON_OFF (SPI) */
-return sd_r1;
-
default:
qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
return sd_illegal;
@@ -2325,6 +2327,7 @@ static const SDProto sd_proto_spi = {
[56] = {8, sd_spi, "GEN_CMD", sd_cmd_GEN_CMD},
[57] = {10, sd_spi, "DIRECT_SECURE_WRITE", sd_cmd_optional},
[58] = {0, sd_spi, "READ_OCR", spi_cmd_READ_OCR},
+[59] = {0, sd_spi, "CRC_ON_OFF", spi_cmd_CRC_ON_OFF},
},
.acmd = {
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
--
2.41.0
[PATCH 18/21] hw/sd/sdcard: Add sd_cmd_APP_CMD handler (CMD55)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 53 -
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4d78ac5b59..5461e56e17 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -245,7 +245,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[25]= "WRITE_MULTIPLE_BLOCK",
[26]= "MANUF_RSVD",
[40]= "DPS_spec",
-[54]= "SDIO_RSVD", [55]= "APP_CMD",
[56]= "GEN_CMD",
[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
[62]= "MANUF_RSVD", [63]= "MANUF_RSVD",
@@ -1624,9 +1623,34 @@ static sd_rsp_type_t sd_cmd_LOCK_UNLOCK(SDState *sd,
SDRequest req)
return sd_cmd_to_receivingdata(sd, req, 0, 0);
}
+/* CMD55 */
+static sd_rsp_type_t sd_cmd_APP_CMD(SDState *sd, SDRequest req)
+{
+switch (sd->state) {
+case sd_ready_state:
+case sd_identification_state:
+case sd_inactive_state:
+return sd_invalid_state_for_cmd(sd, req);
+case sd_idle_state:
+if (!sd_is_spi(sd) && sd_req_get_rca(sd, req) != 0x) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "SD: illegal RCA 0x%04x for APP_CMD\n", req.cmd);
+}
+/* fall-through */
+default:
+break;
+}
+if (!sd_is_spi(sd) && !sd_req_rca_same(sd, req)) {
+return sd_r0;
+}
+sd->expecting_acmd = true;
+sd->card_status |= APP_CMD;
+
+return sd_r1;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
-uint16_t rca;
uint64_t addr;
sd->last_cmd_name = sd_cmd_name(sd, req.cmd);
@@ -1713,29 +1737,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
/* Application specific commands (Class 8) */
-case 55: /* CMD55: APP_CMD */
-rca = sd_req_get_rca(sd, req);
-switch (sd->state) {
-case sd_ready_state:
-case sd_identification_state:
-return sd_illegal;
-case sd_idle_state:
-if (rca) {
-qemu_log_mask(LOG_GUEST_ERROR,
- "SD: illegal RCA 0x%04x for APP_CMD\n", req.cmd);
-}
-default:
-break;
-}
-if (!sd_is_spi(sd)) {
-if (sd->rca != rca) {
-return sd_r0;
-}
-}
-sd->expecting_acmd = true;
-sd->card_status |= APP_CMD;
-return sd_r1;
-
case 56: /* CMD56: GEN_CMD */
switch (sd->state) {
case sd_transfer_state:
@@ -2321,6 +2322,7 @@ static const SDProto sd_proto_spi = {
[50] = {10, sd_spi, "DIRECT_SECURE_READ", sd_cmd_optional},
[52] = {9, sd_spi, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_spi, "IO_RW_EXTENDED", sd_cmd_optional},
+[55] = {8, sd_spi, "APP_CMD", sd_cmd_APP_CMD},
[57] = {10, sd_spi, "DIRECT_SECURE_WRITE", sd_cmd_optional},
},
.acmd = {
@@ -2373,6 +2375,7 @@ static const SDProto sd_proto_sd = {
[50] = {10, sd_adtc, "DIRECT_SECURE_READ", sd_cmd_optional},
[52] = {9, sd_bc, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_bc, "IO_RW_EXTENDED", sd_cmd_optional},
+[55] = {8, sd_ac, "APP_CMD", sd_cmd_APP_CMD},
[57] = {10, sd_adtc, "DIRECT_SECURE_WRITE", sd_cmd_optional},
[58] = {11, sd_adtc, "READ_EXTR_MULTI", sd_cmd_optional},
[59] = {11, sd_adtc, "WRITE_EXTR_MULTI", sd_cmd_optional},
--
2.41.0
[PATCH 16/21] hw/sd/sdcard: Add sd_cmd_ERASE handler (CMD38)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 41 -
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4e31dfe18f..17fec612eb 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -244,7 +244,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[21]= "DPS_spec",
[25]= "WRITE_MULTIPLE_BLOCK",
[26]= "MANUF_RSVD",
-[38]= "ERASE",
[40]= "DPS_spec",
[42]= "LOCK_UNLOCK",
[54]= "SDIO_RSVD", [55]= "APP_CMD",
@@ -1602,6 +1601,24 @@ static sd_rsp_type_t sd_cmd_ERASE_WR_BLK_END(SDState
*sd, SDRequest req)
return sd_r1;
}
+/* CMD38 */
+static sd_rsp_type_t sd_cmd_ERASE(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+if (sd->csd[14] & 0x30) {
+sd->card_status |= WP_VIOLATION;
+return sd_r1b;
+}
+
+sd->state = sd_programming_state;
+sd_erase(sd);
+/* Bzzztt Operation complete. */
+sd->state = sd_transfer_state;
+return sd_r1b;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
@@ -1690,26 +1707,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 26: /* CMD26: PROGRAM_CID */
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-/* Erase commands (Class 5) */
-case 38: /* CMD38: ERASE */
-switch (sd->state) {
-case sd_transfer_state:
-if (sd->csd[14] & 0x30) {
-sd->card_status |= WP_VIOLATION;
-return sd_r1b;
-}
-
-sd->state = sd_programming_state;
-sd_erase(sd);
-/* Bzzztt Operation complete. */
-sd->state = sd_transfer_state;
-return sd_r1b;
-
-default:
-break;
-}
-break;
-
/* Lock card commands (Class 7) */
case 42: /* CMD42: LOCK_UNLOCK */
return sd_cmd_to_receivingdata(sd, req, 0, 0);
@@ -2318,6 +2315,7 @@ static const SDProto sd_proto_spi = {
[35] = {10, sd_spi, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
[37] = {10, sd_spi, "CONTROL_ASSD_SYSTEM", sd_cmd_optional},
+[38] = {5, sd_spi, "ERASE", sd_cmd_ERASE},
[50] = {10, sd_spi, "DIRECT_SECURE_READ", sd_cmd_optional},
[52] = {9, sd_spi, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_spi, "IO_RW_EXTENDED", sd_cmd_optional},
@@ -2361,6 +2359,7 @@ static const SDProto sd_proto_sd = {
[35] = {10, sd_adtc, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_adtc, "SEND_PSI", sd_cmd_optional},
[37] = {10, sd_ac, "CONTROL_ASSD_SYSTEM", sd_cmd_optional},
+[38] = {5, sd_ac, "ERASE", sd_cmd_ERASE},
[43] = {1, sd_ac, "Q_MANAGEMENT", sd_cmd_optional},
[44] = {1, sd_ac, "Q_TASK_INFO_A", sd_cmd_optional},
[45] = {1, sd_ac, "Q_TASK_INFO_B", sd_cmd_optional},
--
2.41.0
[PATCH 20/21] hw/sd/sdcard: Add spi_cmd_READ_OCR handler (CMD58)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 50cee5ac40..b3b4cd5a3a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1660,6 +1660,12 @@ static sd_rsp_type_t sd_cmd_GEN_CMD(SDState *sd,
SDRequest req)
}
}
+/* CMD58 */
+static sd_rsp_type_t spi_cmd_READ_OCR(SDState *sd, SDRequest req)
+{
+return sd_r3;
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint64_t addr;
@@ -1748,9 +1754,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
/* Application specific commands (Class 8) */
-case 58:/* CMD58: READ_OCR (SPI) */
-return sd_r3;
-
case 59:/* CMD59: CRC_ON_OFF (SPI) */
return sd_r1;
@@ -2321,6 +2324,7 @@ static const SDProto sd_proto_spi = {
[55] = {8, sd_spi, "APP_CMD", sd_cmd_APP_CMD},
[56] = {8, sd_spi, "GEN_CMD", sd_cmd_GEN_CMD},
[57] = {10, sd_spi, "DIRECT_SECURE_WRITE", sd_cmd_optional},
+[58] = {0, sd_spi, "READ_OCR", spi_cmd_READ_OCR},
},
.acmd = {
[41] = {8, sd_spi, "SEND_OP_COND", spi_cmd_SEND_OP_COND},
--
2.41.0
Re: [PATCH v3 15/17] tests/qtest: Disable npcm7xx_sdhci tests using hardcoded RCA
On 27/06/2024 18.22, Philippe Mathieu-Daudé wrote:
Disable tests using 0x4567 hardcoded RCA otherwise when
using random RCA we get:
ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion
failed: (ret == len)
not ok /arm/npcm7xx_sdhci/read_sd -
ERROR:../../tests/qtest/npcm7xx_sdhci-test.c:69:write_sdread: assertion failed:
(ret == len)
Bail out!
See
https://lore.kernel.org/qemu-devel/37f83be9-deb5-42a1-b704-14984351d...@linaro.org/
Signed-off-by: Philippe Mathieu-Daudé
---
Cc: Hao Wu
Cc: Shengtan Mao
Cc: Tyrone Ting
---
tests/qtest/npcm7xx_sdhci-test.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
index 5d68540e52..6a42b142ad 100644
--- a/tests/qtest/npcm7xx_sdhci-test.c
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -44,6 +44,7 @@ static QTestState *setup_sd_card(void)
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 << 8));
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
+g_test_skip("hardcoded 0x4567 card address");
This g_test_skip here does not make too much sense (since you're doing it in
the caller site, too) ... could you please replace it with a proper comment
why this code needs to be reworked? Thanks!
Thomas
sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
SDHC_SELECT_DESELECT_CARD);
@@ -76,6 +77,9 @@ static void test_read_sd(void)
{
QTestState *qts = setup_sd_card();
+g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
+return;
+
write_sdread(qts, "hello world");
write_sdread(qts, "goodbye");
@@ -108,6 +112,9 @@ static void test_write_sd(void)
{
QTestState *qts = setup_sd_card();
+g_test_skip("hardcoded 0x4567 card address used in setup_sd_card()");
+return;
+
sdwrite_read(qts, "hello world");
sdwrite_read(qts, "goodbye");
[PATCH 17/21] hw/sd/sdcard: Add sd_cmd_LOCK_UNLOCK handler (CMD42)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 17fec612eb..4d78ac5b59 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -245,7 +245,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[25]= "WRITE_MULTIPLE_BLOCK",
[26]= "MANUF_RSVD",
[40]= "DPS_spec",
-[42]= "LOCK_UNLOCK",
[54]= "SDIO_RSVD", [55]= "APP_CMD",
[56]= "GEN_CMD",
[60]= "MANUF_RSVD", [61]= "MANUF_RSVD",
@@ -1619,6 +1618,12 @@ static sd_rsp_type_t sd_cmd_ERASE(SDState *sd, SDRequest
req)
return sd_r1b;
}
+/* CMD42 */
+static sd_rsp_type_t sd_cmd_LOCK_UNLOCK(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_receivingdata(sd, req, 0, 0);
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
@@ -1707,10 +1712,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 26: /* CMD26: PROGRAM_CID */
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-/* Lock card commands (Class 7) */
-case 42: /* CMD42: LOCK_UNLOCK */
-return sd_cmd_to_receivingdata(sd, req, 0, 0);
-
/* Application specific commands (Class 8) */
case 55: /* CMD55: APP_CMD */
rca = sd_req_get_rca(sd, req);
@@ -2316,6 +2317,7 @@ static const SDProto sd_proto_spi = {
[36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
[37] = {10, sd_spi, "CONTROL_ASSD_SYSTEM", sd_cmd_optional},
[38] = {5, sd_spi, "ERASE", sd_cmd_ERASE},
+[42] = {7, sd_spi, "LOCK_UNLOCK", sd_cmd_LOCK_UNLOCK},
[50] = {10, sd_spi, "DIRECT_SECURE_READ", sd_cmd_optional},
[52] = {9, sd_spi, "IO_RW_DIRECT", sd_cmd_optional},
[53] = {9, sd_spi, "IO_RW_EXTENDED", sd_cmd_optional},
@@ -2360,6 +2362,7 @@ static const SDProto sd_proto_sd = {
[36] = {10, sd_adtc, "SEND_PSI", sd_cmd_optional},
[37] = {10, sd_ac, "CONTROL_ASSD_SYSTEM", sd_cmd_optional},
[38] = {5, sd_ac, "ERASE", sd_cmd_ERASE},
+[42] = {7, sd_adtc, "LOCK_UNLOCK", sd_cmd_LOCK_UNLOCK},
[43] = {1, sd_ac, "Q_MANAGEMENT", sd_cmd_optional},
[44] = {1, sd_ac, "Q_TASK_INFO_A", sd_cmd_optional},
[45] = {1, sd_ac, "Q_TASK_INFO_B", sd_cmd_optional},
--
2.41.0
[PATCH 14/21] hw/sd/sdcard: Add sd_cmd_SEND_WRITE_PROT handler (CMD30)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 47 +--
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a63213613b..bf9975e9b1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -244,7 +244,6 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[21]= "DPS_spec",
[25]= "WRITE_MULTIPLE_BLOCK",
[26]= "MANUF_RSVD",
-[30]= "SEND_WRITE_PROT",
[32]= "ERASE_WR_BLK_START", [33]= "ERASE_WR_BLK_END",
[38]= "ERASE",
[40]= "DPS_spec",
@@ -1561,11 +1560,33 @@ static sd_rsp_type_t sd_cmd_CLR_WRITE_PROT(SDState *sd,
SDRequest req)
return sd_cmd_SET_CLR_WRITE_PROT(sd, req, false);
}
+/* CMD30 */
+static sd_rsp_type_t sd_cmd_SEND_WRITE_PROT(SDState *sd, SDRequest req)
+{
+uint64_t addr;
+uint32_t data;
+
+if (sd->size > SDSC_MAX_CAPACITY) {
+return sd_illegal;
+}
+
+if (sd->state != sd_transfer_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+addr = sd_req_get_address(sd, req);
+if (!address_in_range(sd, "SEND_WRITE_PROT", addr, sd->blk_len)) {
+return sd_r1;
+}
+
+data = sd_wpbits(sd, req.arg);
+return sd_cmd_to_sendingdata(sd, req, addr, , sizeof(data));
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
uint64_t addr;
-uint32_t data;
sd->last_cmd_name = sd_cmd_name(sd, req.cmd);
/* CMD55 precedes an ACMD, so we are not interested in tracing it.
@@ -1650,26 +1671,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 26: /* CMD26: PROGRAM_CID */
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-/* Write protection (Class 6) */
-case 30: /* CMD30: SEND_WRITE_PROT */
-if (sd->size > SDSC_MAX_CAPACITY) {
-return sd_illegal;
-}
-addr = sd_req_get_address(sd, req);
-switch (sd->state) {
-case sd_transfer_state:
-if (!address_in_range(sd, "SEND_WRITE_PROT",
- req.arg, sd->blk_len)) {
-return sd_r1;
-}
-data = sd_wpbits(sd, req.arg);
-return sd_cmd_to_sendingdata(sd, req, addr, , sizeof(data));
-
-default:
-break;
-}
-break;
-
/* Erase commands (Class 5) */
case 32: /* CMD32: ERASE_WR_BLK_START */
switch (sd->state) {
@@ -2313,6 +2314,7 @@ static const SDProto sd_proto_spi = {
[27] = {4, sd_spi, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[28] = {6, sd_spi, "SET_WRITE_PROT", sd_cmd_SET_WRITE_PROT},
[29] = {6, sd_spi, "CLR_WRITE_PROT", sd_cmd_CLR_WRITE_PROT},
+[30] = {6, sd_spi, "SEND_WRITE_PROT", sd_cmd_SEND_WRITE_PROT},
[34] = {10, sd_spi, "READ_SEC_CMD", sd_cmd_optional},
[35] = {10, sd_spi, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
@@ -2353,6 +2355,7 @@ static const SDProto sd_proto_sd = {
[27] = {4, sd_adtc, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[28] = {6, sd_ac, "SET_WRITE_PROT", sd_cmd_SET_WRITE_PROT},
[29] = {6, sd_ac, "CLR_WRITE_PROT", sd_cmd_CLR_WRITE_PROT},
+[30] = {6, sd_adtc, "SEND_WRITE_PROT", sd_cmd_SEND_WRITE_PROT},
[34] = {10, sd_adtc, "READ_SEC_CMD", sd_cmd_optional},
[35] = {10, sd_adtc, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_adtc, "SEND_PSI", sd_cmd_optional},
--
2.41.0
[PATCH 00/12] hw/sd/sdcard: Convert ACMD to sd_cmd_handler format
For each switch case in the big sd_app_command() function, extract the corresponding sd_cmd_handler. Few housekeeping patches at the end. Based-on: <20240627164345.82192-1-phi...@linaro.org> Full series for testing: https://gitlab.com/philmd/qemu/-/tags/emmc-v4 Philippe Mathieu-Daudé (12): hw/sd/sdcard: Add sd_acmd_SET_BUS_WIDTH handler (ACMD6) hw/sd/sdcard: Add sd_acmd_SD_STATUS handler (ACMD13) hw/sd/sdcard: Add sd_acmd_SEND_NUM_WR_BLOCKS handler (ACMD22) hw/sd/sdcard: Add sd_acmd_SET_WR_BLK_ERASE_COUNT handler (ACMD23) hw/sd/sdcard: Add sd_acmd_SD_APP_OP_COND handler (ACMD41) hw/sd/sdcard: Add sd_acmd_SET_CLR_CARD_DETECT handler (ACMD42) hw/sd/sdcard: Add sd_acmd_SEND_SCR handler (ACMD51) hw/sd/sdcard: Remove sd_none enum from sd_cmd_type_t hw/sd/sdcard: Remove noise from sd_acmd_name() hw/sd/sdcard: Remove noise from sd_cmd_name() hw/sd/sdcard: Remove default case in read/write on DAT lines hw/sd/sdcard: Trace length of data read on DAT lines include/hw/sd/sd.h | 1 - hw/sd/sd.c | 256 + hw/sd/trace-events | 2 +- 3 files changed, 119 insertions(+), 140 deletions(-) -- 2.41.0
[PATCH 04/21] hw/sd/sdcard: Add sd_cmd_SEND_CSD/CID handlers (CMD9 & CMD10)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 36 ++--
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e2f7e99ea2..bd7c7cf518 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -240,8 +240,6 @@ static const char *sd_response_name(sd_rsp_type_t rsp)
static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
{
static const char *cmd_abbrev[SDMMC_CMD_MAX] = {
- [9]= "SEND_CSD",
-[10]= "SEND_CID",
[12]= "STOP_TRANSMISSION", [13]= "SEND_STATUS",
[15]= "GO_INACTIVE_STATE",
[16]= "SET_BLOCKLEN", [17]= "READ_SINGLE_BLOCK",
@@ -1324,6 +1322,26 @@ static sd_rsp_type_t sd_cmd_SEND_IF_COND(SDState *sd,
SDRequest req)
return sd_r7;
}
+/* CMD9 */
+static sd_rsp_type_t sd_cmd_SEND_CSD(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_standby_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+return sd_req_rca_same(sd, req) ? sd_r2_s : sd_r0;
+}
+
+/* CMD10 */
+static sd_rsp_type_t sd_cmd_SEND_CID(SDState *sd, SDRequest req)
+{
+if (sd->state != sd_standby_state) {
+return sd_invalid_state_for_cmd(sd, req);
+}
+
+return sd_req_rca_same(sd, req) ? sd_r2_i : sd_r0;
+}
+
/* CMD19 */
static sd_rsp_type_t sd_cmd_SEND_TUNING_BLOCK(SDState *sd, SDRequest req)
{
@@ -1393,12 +1411,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 9: /* CMD9: SEND_CSD */
rca = sd_req_get_rca(sd, req);
switch (sd->state) {
-case sd_standby_state:
-if (sd->rca != rca)
-return sd_r0;
-
-return sd_r2_s;
-
case sd_transfer_state:
if (!sd_is_spi(sd)) {
break;
@@ -1414,12 +1426,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 10: /* CMD10: SEND_CID */
rca = sd_req_get_rca(sd, req);
switch (sd->state) {
-case sd_standby_state:
-if (sd->rca != rca)
-return sd_r0;
-
-return sd_r2_i;
-
case sd_transfer_state:
if (!sd_is_spi(sd)) {
break;
@@ -2307,6 +2313,8 @@ static const SDProto sd_proto_sd = {
[6] = {10, sd_adtc, "SWITCH_FUNCTION", sd_cmd_SWITCH_FUNCTION},
[7] = {0, sd_ac, "(DE)SELECT_CARD", sd_cmd_DE_SELECT_CARD},
[8] = {0, sd_bcr, "SEND_IF_COND", sd_cmd_SEND_IF_COND},
+[9] = {0, sd_ac, "SEND_CSD", sd_cmd_SEND_CSD},
+[10] = {0, sd_ac, "SEND_CID", sd_cmd_SEND_CID},
[11] = {0, sd_ac, "VOLTAGE_SWITCH", sd_cmd_optional},
[19] = {2, sd_adtc, "SEND_TUNING_BLOCK", sd_cmd_SEND_TUNING_BLOCK},
[20] = {2, sd_ac, "SPEED_CLASS_CONTROL", sd_cmd_optional},
--
2.41.0
[PATCH 12/21] hw/sd/sdcard: Add sd_cmd_PROGRAM_CSD handler (CMD27)
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sd.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4650d20ee7..9d33113f11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -243,7 +243,7 @@ static const char *sd_cmd_name(SDState *sd, uint8_t cmd)
[18]= "READ_MULTIPLE_BLOCK",
[21]= "DPS_spec",
[25]= "WRITE_MULTIPLE_BLOCK",
-[26]= "MANUF_RSVD", [27]= "PROGRAM_CSD",
+[26]= "MANUF_RSVD",
[28]= "SET_WRITE_PROT", [29]= "CLR_WRITE_PROT",
[30]= "SEND_WRITE_PROT",
[32]= "ERASE_WR_BLK_START", [33]= "ERASE_WR_BLK_END",
@@ -1514,6 +1514,12 @@ static sd_rsp_type_t sd_cmd_WRITE_SINGLE_BLOCK(SDState
*sd, SDRequest req)
return sd_cmd_to_receivingdata(sd, req, addr, sd->blk_len);
}
+/* CMD27 */
+static sd_rsp_type_t sd_cmd_PROGRAM_CSD(SDState *sd, SDRequest req)
+{
+return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->csd));
+}
+
static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
{
uint16_t rca;
@@ -1603,9 +1609,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
SDRequest req)
case 26: /* CMD26: PROGRAM_CID */
return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->cid));
-case 27: /* CMD27: PROGRAM_CSD */
-return sd_cmd_to_receivingdata(sd, req, 0, sizeof(sd->csd));
-
/* Write protection (Class 6) */
case 28: /* CMD28: SET_WRITE_PROT */
if (sd->size > SDSC_MAX_CAPACITY) {
@@ -2310,6 +2313,7 @@ static const SDProto sd_proto_spi = {
[16] = {2, sd_spi, "SET_BLOCKLEN", sd_cmd_SET_BLOCKLEN},
[17] = {2, sd_spi, "READ_SINGLE_BLOCK", sd_cmd_READ_SINGLE_BLOCK},
[24] = {4, sd_spi, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
+[27] = {4, sd_spi, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[34] = {10, sd_spi, "READ_SEC_CMD", sd_cmd_optional},
[35] = {10, sd_spi, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_spi, "SEND_PSI", sd_cmd_optional},
@@ -2347,6 +2351,7 @@ static const SDProto sd_proto_sd = {
[20] = {2, sd_ac, "SPEED_CLASS_CONTROL", sd_cmd_optional},
[23] = {2, sd_ac, "SET_BLOCK_COUNT", sd_cmd_SET_BLOCK_COUNT},
[24] = {4, sd_adtc, "WRITE_SINGLE_BLOCK", sd_cmd_WRITE_SINGLE_BLOCK},
+[27] = {4, sd_adtc, "PROGRAM_CSD", sd_cmd_PROGRAM_CSD},
[34] = {10, sd_adtc, "READ_SEC_CMD", sd_cmd_optional},
[35] = {10, sd_adtc, "WRITE_SEC_CMD", sd_cmd_optional},
[36] = {10, sd_adtc, "SEND_PSI", sd_cmd_optional},
--
2.41.0
Re: [PATCH 0/4] hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate
* Markus Armbruster (arm...@redhat.com) wrote: > "Dr. David Alan Gilbert" writes: > > > * Markus Armbruster (arm...@redhat.com) wrote: > >> Philippe Mathieu-Daudé writes: > >> > >> > Hi Daniel, Dave, Markus & Thomas. > >> > > >> > On 4/6/24 06:58, Markus Armbruster wrote: > >> >> "Dr. David Alan Gilbert" writes: > >> >>> * Daniel P. Berrangé (berra...@redhat.com) wrote: > >> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > >> > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > >> >> We are trying to unify all qemu-system-FOO to a single binary. > >> >> In order to do that we need to remove QAPI target specific code. > >> >> > >> >> @dump-skeys is only available on qemu-system-s390x. This series > >> >> rename it as @dump-s390-skey, making it available on other > >> >> binaries. We take care of backward compatibility via deprecation. > >> >> > >> >> Philippe Mathieu-Daudé (4): > >> >> hw/s390x: Introduce the @dump-s390-skeys QMP command > >> >> hw/s390x: Introduce the 'dump_s390_skeys' HMP command > >> >> hw/s390x: Deprecate the HMP 'dump_skeys' command > >> >> hw/s390x: Deprecate the QMP @dump-skeys command > >> > > >> > Why do we have to rename the command? Just for the sake of it? I > >> > think > >> > renaming HMP commands is maybe ok, but breaking the API in QMP is > >> > something > >> > you should consider twice. > >> > > >> > I'm looking at how to include this command in the new "single binary". > >> > > >> > Markus explained in an earlier series, just expanding this command as > >> > stub to targets that don't implement it is not backward compatible and > >> > breaks QMP introspection. Currently on s390x we get a result, on other > >> > targets the command doesn't exist. If we add a stubs, then other targets > >> > return something (even if it is an empty list), confusing management > >> > interface. > >> > >> Loss of introspection precision is a concern, not a hard "no". > >> > >> We weigh all the concerns, and pick a solution we hate the least :) > >> > >> > So this approach use to deprecate process to include a new command > >> > which behaves differently on non-s390x targets. > >> > > >> > If we don't care for this particular case, better. However I'd still > >> > like to discuss this approach for other target-specific commands. > >> > > >> >> PRO rename: the command's tie to S390 is them immediately obvious, which > >> >> may be useful when the command becomes available in qemu-systems capable > >> >> of running other targets. > >> >> > >> >> CON rename: users need to adapt. > >> >> > >> >> What are the users? Not libvirt, as far as I can tell. > >> > > >> > Years ago we said, "all HMP must be based on QMP". > >> > >> In practice, it's closer to "HMP must be base on QMP when the > >> functionality does or should exist in QMP." > >> > >> >Now we realize HMP > >> > became stable because QMP-exposed, although not consumed externally... > >> > >> I'm afraid I didn't get this part. > >> > >> > Does the concept of "internal QMP commands" makes sense for HMP debug > >> > ones? (Looking at a way to not expose them). We could use the "x-" > >> > prefix to not care about stable / backward compat, but what is the point > >> > of exposing to QMP commands that will never be accessed there? > >> > > >> That was going to be my question too. Seems like its possible to > >> simply > >> stub out the existing command for other targets. > >> >> > >> >> That's going to happen whether we rename the commands or not. > >> >> > >> >>> Are these commands really supposed to be stable, or are they just debug > >> >>> commands? If they are debug, then add the x- and don't worry too much. > >> > > >> > OK. > >> > > >> >> docs/devel/qapi-code-gen.rst: > >> >> > >> >> Names beginning with ``x-`` used to signify "experimental". This > >> >> convention has been replaced by special feature "unstable". > >> >> > >> >> Feature "unstable" is what makes something unstable, and is what > >> >> machines should check. > >> > > >> > What I mentioned earlier could be 'Feature "internal" or "debug"'. > >> > >> What's the difference to "unstable"? > > > > It should be clear *why* something is marked x- - something that's > > marked 'x-' because the feature is still in development is expected to shake > > out at some point, and the interface designed so it can. > > (and at some point the developer should get a prod to be asked whethere the > > x- can be removed). > > That's different from it permenantly being x- because it's expected to > > change as the needs of the people debugging change. > > When you add special feature 'unstable', the tooling insists you cover > it in the doc comment. Review should then ensure the doc comment > explains why it is unstable. Examples: > ># @unstable: Member @x-perf is experimental. > >
