Re: [PATCH for-8.2 v3 6/8] target/riscv: add 'max' CPU type
On 2023/7/15 01:43, Daniel Henrique Barboza wrote:
The 'max' CPU type is used by tooling to determine what's the most
capable CPU a current QEMU version implements. Other archs such as ARM
implements this type. Let's add it to RISC-V.
What we consider "most capable CPU" in this context are related to
ratified, non-vendor extensions. This means that we want the 'max' CPU
to enable all (possible) ratified extensions by default. The reasoning
behind this design is (1) vendor extensions can conflict with each other
and we won't play favorities deciding which one is default or not and
(2) non-ratified extensions are always prone to changes, not being
stable enough to be enabled by default.
All this said, we're still not able to enable all ratified extensions
due to conflicts between them. Zfinx and all its dependencies aren't
enabled because of a conflict with RVF. zce, zcmp and zcmt are also
disabled due to RVD conflicts. When running with 64 bits we're also
disabling zcf.
MISA bits RVG, RVJ and RVV are also being set manually since they're
default disabled.
This is the resulting 'riscv,isa' DT for this new CPU:
rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_
zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_
zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_
smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt
Signed-off-by: Daniel Henrique Barboza
---
Reviewed-by: Weiwei Li
Weiwei Li
target/riscv/cpu-qom.h | 1 +
target/riscv/cpu.c | 53 ++
2 files changed, 54 insertions(+)
diff --git a/target/riscv/cpu-qom.h b/target/riscv/cpu-qom.h
index 04af50983e..f3fbe37a2c 100644
--- a/target/riscv/cpu-qom.h
+++ b/target/riscv/cpu-qom.h
@@ -30,6 +30,7 @@
#define CPU_RESOLVING_TYPE TYPE_RISCV_CPU
#define TYPE_RISCV_CPU_ANY RISCV_CPU_TYPE_NAME("any")
+#define TYPE_RISCV_CPU_MAX RISCV_CPU_TYPE_NAME("max")
#define TYPE_RISCV_CPU_BASE32 RISCV_CPU_TYPE_NAME("rv32")
#define TYPE_RISCV_CPU_BASE64 RISCV_CPU_TYPE_NAME("rv64")
#define TYPE_RISCV_CPU_BASE128 RISCV_CPU_TYPE_NAME("x-rv128")
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f7083b2d5c..1cdffd5927 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -248,6 +248,7 @@ static const char * const riscv_intr_names[] = {
};
static void riscv_cpu_add_user_properties(Object *obj);
+static void riscv_init_max_cpu_extensions(Object *obj);
const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
{
@@ -374,6 +375,25 @@ static void riscv_any_cpu_init(Object *obj)
cpu->cfg.pmp = true;
}
+static void riscv_max_cpu_init(Object *obj)
+{
+RISCVCPU *cpu = RISCV_CPU(obj);
+CPURISCVState *env = >env;
+RISCVMXL mlx = MXL_RV64;
+
+#ifdef TARGET_RISCV32
+mlx = MXL_RV32;
+#endif
+set_misa(env, mlx, 0);
+riscv_cpu_add_user_properties(obj);
+riscv_init_max_cpu_extensions(obj);
+env->priv_ver = PRIV_VERSION_LATEST;
+#ifndef CONFIG_USER_ONLY
+set_satp_mode_max_supported(RISCV_CPU(obj), mlx == MXL_RV32 ?
+VM_1_10_SV32 : VM_1_10_SV57);
+#endif
+}
+
#if defined(TARGET_RISCV64)
static void rv64_base_cpu_init(Object *obj)
{
@@ -1930,6 +1950,38 @@ static void riscv_cpu_add_user_properties(Object *obj)
ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
}
+/*
+ * The 'max' type CPU will have all possible ratified
+ * non-vendor extensions enabled.
+ */
+static void riscv_init_max_cpu_extensions(Object *obj)
+{
+RISCVCPU *cpu = RISCV_CPU(obj);
+CPURISCVState *env = >env;
+Property *prop;
+
+/* Enable RVG, RVJ and RVV that are disabled by default */
+set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
+
+for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
+object_property_set_bool(obj, prop->name, true, NULL);
+}
+
+/* Zfinx is not compatible with F. Disable it */
+object_property_set_bool(obj, "zfinx", false, NULL);
+object_property_set_bool(obj, "zdinx", false, NULL);
+object_property_set_bool(obj, "zhinx", false, NULL);
+object_property_set_bool(obj, "zhinxmin", false, NULL);
+
+object_property_set_bool(obj, "zce", false, NULL);
+object_property_set_bool(obj, "zcmp", false, NULL);
+object_property_set_bool(obj, "zcmt", false, NULL);
+
+if (env->misa_mxl != MXL_RV32) {
+object_property_set_bool(obj, "zcf", false, NULL);
+}
+}
+
static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
@@ -2268,6 +2320,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
.abstract = true,
},
DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_ANY, riscv_any_cpu_init),
+DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_MAX, riscv_max_cpu_init),
#if defined(CONFIG_KVM)
DEFINE_CPU(TYPE_RISCV_CPU_HOST, riscv_host_cpu_init),
#endif
Re: [PATCH for-8.2 v3 5/8] target/riscv/cpu.c: add a ADD_CPU_PROPERTIES_ARRAY() macro
On 2023/7/15 01:43, Daniel Henrique Barboza wrote:
The code inside riscv_cpu_add_user_properties() became quite repetitive
after recent changes. Add a macro to hide the repetition away.
Signed-off-by: Daniel Henrique Barboza
---
Reviewed-by: Weiwei Li
Weiwei Li
target/riscv/cpu.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5689368f02..f7083b2d5c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1875,6 +1875,13 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor
*v,
}
#endif
+#define ADD_CPU_PROPERTIES_ARRAY(_dev, _array) \
+do { \
+for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
+qdev_property_add_static(_dev, &_array[i]); \
+} \
+} while (0)
+
/*
* Add CPU properties with user-facing flags.
*
@@ -1918,17 +1925,9 @@ static void riscv_cpu_add_user_properties(Object *obj)
qdev_property_add_static(dev, prop);
}
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
-qdev_property_add_static(dev, _cpu_options[i]);
-}
-
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_vendor_exts); i++) {
-qdev_property_add_static(dev, _cpu_vendor_exts[i]);
-}
-
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_experimental_exts); i++) {
-qdev_property_add_static(dev, _cpu_experimental_exts[i]);
-}
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_options);
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_vendor_exts);
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
}
static Property riscv_cpu_properties[] = {
[PATCH 2/6] qtest: implement named interception of out-GPIO
Adds qtest_irq_intercept_out_named method, which utilizes a new optional
name parameter to the irq_intercept_out qtest command.
Signed-off-by: Chris Laplante
---
softmmu/qtest.c| 39 ++-
tests/qtest/libqtest.c | 6 ++
tests/qtest/libqtest.h | 11 +++
3 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8d764b719..7c3dea5760 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -388,8 +388,12 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
|| strcmp(words[0], "irq_intercept_in") == 0) {
DeviceState *dev;
NamedGPIOList *ngl;
+bool is_named;
+bool is_outbound;
g_assert(words[1]);
+is_named = words[2] != NULL;
+is_outbound = words[0][14] == 'o';
dev = DEVICE(object_resolve_path(words[1], NULL));
if (!dev) {
qtest_send_prefix(chr);
@@ -408,19 +412,28 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
}
QLIST_FOREACH(ngl, >gpios, node) {
-/* We don't support intercept of named GPIOs yet */
-if (ngl->name) {
-continue;
-}
-if (words[0][14] == 'o') {
-int i;
-for (i = 0; i < ngl->num_out; ++i) {
-qemu_irq *disconnected = g_new0(qemu_irq, 1);
-qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
- disconnected, i);
-
-*disconnected = qdev_intercept_gpio_out(dev, icpt,
-ngl->name, i);
+/* We don't support inbound interception of named GPIOs yet */
+if (is_outbound) {
+if (is_named) {
+if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
+qemu_irq *disconnected = g_new0(qemu_irq, 1);
+qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+ disconnected, 0);
+
+*disconnected = qdev_intercept_gpio_out(dev, icpt,
+ngl->name, 0);
+break;
+}
+} else if (!ngl->name) {
+int i;
+for (i = 0; i < ngl->num_out; ++i) {
+qemu_irq *disconnected = g_new0(qemu_irq, 1);
+qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+ disconnected, i);
+
+*disconnected = qdev_intercept_gpio_out(dev, icpt,
+ngl->name, i);
+}
}
} else {
qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index c22dfc30d3..471529e6cc 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -993,6 +993,12 @@ void qtest_irq_intercept_out(QTestState *s, const char
*qom_path)
qtest_rsp(s);
}
+void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const
char *name)
+{
+qtest_sendf(s, "irq_intercept_out %s %s\n", qom_path, name);
+qtest_rsp(s);
+}
+
void qtest_irq_intercept_in(QTestState *s, const char *qom_path)
{
qtest_sendf(s, "irq_intercept_in %s\n", qom_path);
diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h
index 3a71bc45fc..e53e350e3a 100644
--- a/tests/qtest/libqtest.h
+++ b/tests/qtest/libqtest.h
@@ -371,6 +371,17 @@ void qtest_irq_intercept_in(QTestState *s, const char
*string);
*/
void qtest_irq_intercept_out(QTestState *s, const char *string);
+/**
+ * qtest_irq_intercept_out_named:
+ * @s: #QTestState instance to operate on.
+ * @qom_path: QOM path of a device.
+ * @name: Name of the GPIO out pin
+ *
+ * Associate a qtest irq with the named GPIO-out pin of the device
+ * whose path is specified by @string and whose name is @name.
+ */
+void qtest_irq_intercept_out_named(QTestState *s, const char *qom_path, const
char *name);
+
/**
* qtest_set_irq_in:
* @s: QTestState instance to operate on.
--
2.39.2
[PATCH 5/6] qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed
This is much better than just silently failing with OK.
Signed-off-by: Chris Laplante
---
softmmu/qtest.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 051bbf4177..e888acb319 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -399,6 +399,7 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
NamedGPIOList *ngl;
bool is_named;
bool is_outbound;
+bool interception_succeeded = false;
g_assert(words[1]);
is_named = words[2] != NULL;
@@ -431,6 +432,7 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
if (is_named) {
if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
qtest_install_gpio_out_intercepts(dev, ngl->name, 0);
+interception_succeeded = true;
break;
}
} else if (!ngl->name) {
@@ -438,15 +440,22 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
for (i = 0; i < ngl->num_out; ++i) {
qtest_install_gpio_out_intercepts(dev, ngl->name, i);
}
+interception_succeeded = true;
}
} else {
qemu_irq_intercept_in(ngl->in, qtest_irq_handler,
ngl->num_in);
+interception_succeeded = true;
}
}
-irq_intercept_dev = dev;
+
qtest_send_prefix(chr);
-qtest_send(chr, "OK\n");
+if (interception_succeeded) {
+irq_intercept_dev = dev;
+qtest_send(chr, "OK\n");
+} else {
+qtest_send(chr, "FAIL No intercepts installed\n");
+}
} else if (strcmp(words[0], "set_irq_in") == 0) {
DeviceState *dev;
qemu_irq irq;
--
2.39.2
[PATCH 6/6] qtest: microbit-test: add tests for nRF51 DETECT
Exercise the DETECT mechanism of the GPIO peripheral.
Signed-off-by: Chris Laplante
---
tests/qtest/microbit-test.c | 42 +
1 file changed, 42 insertions(+)
diff --git a/tests/qtest/microbit-test.c b/tests/qtest/microbit-test.c
index 6022a92b6a..3c85adba37 100644
--- a/tests/qtest/microbit-test.c
+++ b/tests/qtest/microbit-test.c
@@ -393,6 +393,47 @@ static void test_nrf51_gpio(void)
qtest_quit(qts);
}
+static void test_nrf51_gpio_detect(void) {
+QTestState *qts = qtest_init("-M microbit");
+int i;
+
+// Connect input buffer on pins 1-7, configure SENSE for high level
+for (i = 1; i <= 7; i++) {
+qtest_writel(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_CNF_START + i * 4,
deposit32(0, 16, 2, 2));
+}
+
+qtest_irq_intercept_out_named(qts, "/machine/nrf51", "detect");
+
+for (i = 1; i <= 7; i++) {
+// Set pin high
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1);
+uint32_t actual = qtest_readl(qts, NRF51_GPIO_BASE +
NRF51_GPIO_REG_IN);
+g_assert_cmpuint(actual, ==, 1 << i);
+
+// Check that DETECT is high
+g_assert_true(qtest_get_irq(qts, 0));
+
+// Set pin low, check that DETECT goes low.
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 0);
+actual = qtest_readl(qts, NRF51_GPIO_BASE + NRF51_GPIO_REG_IN);
+g_assert_cmpuint(actual, ==, 0x0);
+g_assert_false(qtest_get_irq(qts, 0));
+}
+
+// Set pin 0 high, check that DETECT doesn't fire
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 1);
+g_assert_false(qtest_get_irq(qts, 0));
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 0, 0);
+
+// Set pins 1, 2, and 3 high, then set 3 low. Check that DETECT is still
high.
+for (i = 1; i <= 3; i++) {
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", i, 1);
+}
+g_assert_true(qtest_get_irq(qts, 0));
+qtest_set_irq_in(qts, "/machine/nrf51", "unnamed-gpio-in", 3, 0);
+g_assert_true(qtest_get_irq(qts, 0));
+}
+
static void timer_task(QTestState *qts, hwaddr task)
{
qtest_writel(qts, NRF51_TIMER_BASE + task, NRF51_TRIGGER_TASK);
@@ -499,6 +540,7 @@ int main(int argc, char **argv)
qtest_add_func("/microbit/nrf51/uart", test_nrf51_uart);
qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
+qtest_add_func("/microbit/nrf51/gpio_detect", test_nrf51_gpio_detect);
qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
--
2.39.2
[PATCH 4/6] qtest: factor out qtest_install_gpio_out_intercepts
Simplify the code a bit.
Signed-off-by: Chris Laplante
---
softmmu/qtest.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 74482ce3cd..051bbf4177 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -365,6 +365,15 @@ void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr,
gchar **words))
process_command_cb = pc_cb;
}
+static void qtest_install_gpio_out_intercepts(DeviceState *dev, const char
*name, int n)
+{
+qemu_irq *disconnected = g_new0(qemu_irq, 1);
+qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
+ disconnected, n);
+
+*disconnected = qdev_intercept_gpio_out(dev, icpt,name, n);
+}
+
static void qtest_process_command(CharBackend *chr, gchar **words)
{
const gchar *command;
@@ -421,23 +430,13 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
if (is_outbound) {
if (is_named) {
if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
-qemu_irq *disconnected = g_new0(qemu_irq, 1);
-qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
- disconnected, 0);
-
-*disconnected = qdev_intercept_gpio_out(dev, icpt,
-ngl->name, 0);
+qtest_install_gpio_out_intercepts(dev, ngl->name, 0);
break;
}
} else if (!ngl->name) {
int i;
for (i = 0; i < ngl->num_out; ++i) {
-qemu_irq *disconnected = g_new0(qemu_irq, 1);
-qemu_irq icpt = qemu_allocate_irq(qtest_irq_handler,
- disconnected, i);
-
-*disconnected = qdev_intercept_gpio_out(dev, icpt,
-ngl->name, i);
+qtest_install_gpio_out_intercepts(dev, ngl->name, i);
}
}
} else {
--
2.39.2
[PATCH 3/6] qtest: bail from irq_intercept_in if name is specified
Named interception of in-GPIOs is not supported yet.
Signed-off-by: Chris Laplante
---
softmmu/qtest.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 7c3dea5760..74482ce3cd 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -401,6 +401,12 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
return;
}
+if (is_named && !is_outbound) {
+qtest_send_prefix(chr);
+qtest_send(chr, "FAIL Interception of named in-GPIOs not yet
supported\n");
+return;
+}
+
if (irq_intercept_dev) {
qtest_send_prefix(chr);
if (irq_intercept_dev != dev) {
@@ -412,7 +418,6 @@ static void qtest_process_command(CharBackend *chr, gchar
**words)
}
QLIST_FOREACH(ngl, >gpios, node) {
-/* We don't support inbound interception of named GPIOs yet */
if (is_outbound) {
if (is_named) {
if (ngl->name && strcmp(ngl->name, words[2]) == 0) {
--
2.39.2
[PATCH 0/6] Add nRF51 DETECT signal with test
This patch series implements the nRF51 DETECT signal in the GPIO peripheral. A qtest is added exercising the signal. To implement the test, named out-GPIO IRQ interception had to be added to the qtest framework. I also took the opportunity to improve IRQ interception a bit by adding 'FAIL' responses when interception fails. Otherwise, it is frustrating to troubleshoot why calls to qtest_irq_intercept_out and friends appears to do nothing. Chris Laplante (6): hw/gpio/nrf51: implement DETECT signal qtest: implement named interception of out-GPIO qtest: bail from irq_intercept_in if name is specified qtest: factor out qtest_install_gpio_out_intercepts qtest: irq_intercept_[out/in]: return FAIL if no intercepts are installed qtest: microbit-test: add tests for nRF51 DETECT hw/arm/nrf51_soc.c | 1 + hw/gpio/nrf51_gpio.c | 14 - include/hw/gpio/nrf51_gpio.h | 1 + softmmu/qtest.c | 56 ++-- tests/qtest/libqtest.c | 6 tests/qtest/libqtest.h | 11 +++ tests/qtest/microbit-test.c | 42 +++ 7 files changed, 115 insertions(+), 16 deletions(-) -- 2.39.2
[PATCH 1/6] hw/gpio/nrf51: implement DETECT signal
Implement nRF51 DETECT signal in the GPIO peripheral.
The reference manual makes mention of a per-pin DETECT signal, but these
are not exposed to the user. See
https://devzone.nordicsemi.com/f/nordic-q-a/39858/gpio-per-pin-detect-signal-available
for more information. Currently, I don't see a reason to model these.
Signed-off-by: Chris Laplante
---
hw/arm/nrf51_soc.c | 1 +
hw/gpio/nrf51_gpio.c | 14 +-
include/hw/gpio/nrf51_gpio.h | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 34da0d62f0..7ae54e18be 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -150,6 +150,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error
**errp)
/* Pass all GPIOs to the SOC layer so they are available to the board */
qdev_pass_gpios(DEVICE(>gpio), dev_soc, NULL);
+qdev_pass_gpios(DEVICE(>gpio), dev_soc, "detect");
/* TIMER */
for (i = 0; i < NRF51_NUM_TIMERS; i++) {
diff --git a/hw/gpio/nrf51_gpio.c b/hw/gpio/nrf51_gpio.c
index b47fddf4ed..08396c69a4 100644
--- a/hw/gpio/nrf51_gpio.c
+++ b/hw/gpio/nrf51_gpio.c
@@ -78,6 +78,7 @@ static void update_state(NRF51GPIOState *s)
int pull;
size_t i;
bool connected_out, dir, connected_in, out, in, input;
+bool assert_detect = false;
for (i = 0; i < NRF51_GPIO_PINS; i++) {
pull = pull_value(s->cnf[i]);
@@ -99,7 +100,15 @@ static void update_state(NRF51GPIOState *s)
qemu_log_mask(LOG_GUEST_ERROR,
"GPIO pin %zu short circuited\n", i);
}
-if (!connected_in) {
+if (connected_in) {
+uint32_t detect_config = extract32(s->cnf[i], 16, 2);
+if ((detect_config == 2) && (in == 1)) {
+assert_detect = true;
+}
+if ((detect_config == 3) && (in == 0)) {
+assert_detect = true;
+}
+} else {
/*
* Floating input: the output stimulates IN if connected,
* otherwise pull-up/pull-down resistors put a value on both
@@ -116,6 +125,8 @@ static void update_state(NRF51GPIOState *s)
}
update_output_irq(s, i, connected_out, out);
}
+
+qemu_set_irq(s->detect, assert_detect);
}
/*
@@ -291,6 +302,7 @@ static void nrf51_gpio_init(Object *obj)
qdev_init_gpio_in(DEVICE(s), nrf51_gpio_set, NRF51_GPIO_PINS);
qdev_init_gpio_out(DEVICE(s), s->output, NRF51_GPIO_PINS);
+qdev_init_gpio_out_named(DEVICE(s), >detect, "detect", 1);
}
static void nrf51_gpio_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
index 8f9c2f86da..fcfa2bac17 100644
--- a/include/hw/gpio/nrf51_gpio.h
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -64,6 +64,7 @@ struct NRF51GPIOState {
uint32_t old_out_connected;
qemu_irq output[NRF51_GPIO_PINS];
+qemu_irq detect;
};
--
2.39.2
Re: [PULL 0/5] Patches for QEMU 8.1 hard freeze
On 7/14/23 11:59, Paolo Bonzini wrote: The following changes since commit 3dd9e54703e6ae4f9ab3767f5cecc99edf08: Merge tag 'block-pull-request' ofhttps://gitlab.com/stefanha/qemu into staging (2023-07-12 20:46:10 +0100) are available in the Git repository at: https://gitlab.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to 2eb5599e8a73e70a9e86a97120818ff95a43a23a: scsi: clear unit attention only for REPORT LUNS commands (2023-07-14 11:10:58 +0200) * SCSI unit attention fix * add PCIe devices to s390x emulator * IDE unplug fix for Xen Applied, thanks. Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate. r~
Re: [PATCH 0/3] vhost-user-gpu: support dmabuf modifiers
Hi On Fri, Jul 14, 2023 at 7:42 PM Erico Nunes wrote: > virglrenderer recently added virgl_renderer_resource_get_info_ext as a > new api, which gets resource information, including dmabuf modifiers. > https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024 > > We have to support dmabuf modifiers since the driver may choose to > allocate buffers with these modifiers to improve performance, and > importing buffers without modifiers information may result in completely > broken rendering. > > Currently trying to use vhost-user-gpu for rendering backend and using > the qemu dbus ui as a ui backend results in a broken framebuffer with > Intel GPUs as the buffer is allocated with a modifier. With this > patchset, that is fixed. > > > It is tricky to support since it requires to keep compatibility at the > same time with: > (1) build against older virglrenderer which do not provide > virgl_renderer_resource_get_info_ext; > (2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to > increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT > message. > > I tried to reach a compromise here by not defining a completely new > message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels > like a bit of a hack, so I appreciate feedback if there is a better way > (or naming) to handle it. > looks fine to me, we may consider this as a fix for 8.1 imho Reviewed-by: Marc-André Lureau > > > Erico Nunes (3): > docs: vhost-user-gpu: add protocol changes for dmabuf modifiers > contrib/vhost-user-gpu: add support for sending dmabuf modifiers > vhost-user-gpu: support dmabuf modifiers > > contrib/vhost-user-gpu/vhost-user-gpu.c | 5 ++- > contrib/vhost-user-gpu/virgl.c | 51 +++-- > contrib/vhost-user-gpu/vugpu.h | 9 + > docs/interop/vhost-user-gpu.rst | 26 - > hw/display/vhost-user-gpu.c | 17 - > 5 files changed, 102 insertions(+), 6 deletions(-) > > -- > 2.40.1 > > > -- Marc-André Lureau
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 15:44, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 12:12 PM Stefan Berger wrote: On 7/14/23 14:49, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger wrote: On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan I agree that new QEMU should be able to read old QEMU state but vice versa is not always true. There's been many changes in the past that incremented the vmstate's version_id to indicate that the state format has changed. Also, we are not changing the .read behavior because in Unfortunately the CRB device is being used by x86 on some distros and the expectation is that this existing device can also downgrade to a previous version of QEMU I would say. I have read people migrating from RHEL 9.x even to RHEL 8.x and the expectation is that this works. But would the migration even work due to other parts of QEMU? The only way you can, say, migrate from QEMU 8.1.0 to 8.0.0 is if every single VMstate has its version_id unchanged. Does QEMU provide that guarantee? I'm fine with changing it but just want to make sure expectations are set correctly. Have you tested a downgrade and found that no other device impeded the process? No I have not done this. The best we can do is that CRB at least is not the reason that is causing such a failure and since we are introducing a new device it need not be the reason, either. Stefan
Re: [PATCH] linux-user: Reserve space for brk
14.07.2023 22:43, Michael Tokarev wrote: .. ./arm-linux-user/qemu-arm /usr/lib/klibc/bin/fstype qemu: uncaught target signal 11 (Segmentation fault) - core dumped Segmentation fault Note: the segfault happens before qemu tries to run the program, apparently when it is doing the file sections processing. /mjt
Re: [PATCH] treewide: spelling fixes in comments and some strings
14.07.2023 22:40, Alex Williamson wrote: $ git diff qemu-master..spelling | grep -c re-enable 8 I can drop those 8 out of 400 :) If we consider codespell to be the authority I guess we can leave it, but it seems a bit pedantic to me. Thanks, Let's just drop this one. Please if you consider reviewing the series or some patches from it, just omit the changes with re-enables. They're as good as gone, all 8 of them. Thank you for the feedback! /mjt
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On Fri, Jul 14, 2023 at 12:12 PM Stefan Berger wrote: > > > > On 7/14/23 14:49, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger > > wrote: > >> > >> > >> > >> On 7/14/23 14:22, Stefan Berger wrote: > >>> On 7/14/23 13:04, Joelle van Dyne wrote: > On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger > wrote: > > > > > > > > On 7/14/23 10:05, Stefan Berger wrote: > >> > >> > >> On 7/14/23 03:09, Joelle van Dyne wrote: > >>> When we moved to a single mapping and modified TPM CRB's VMState, it > >>> broke restoring of VMs that were saved on an older version. This > >>> change allows those VMs to gracefully migrate to the new memory > >>> mapping. > >> > >> Thanks. This has to be in 4/11 though. > >> > > > > After applying the whole series and trying to resume state taken with > > current git > > master I cannot restore it but it leads to this error here. I would > > just leave it > > completely untouched in 4/11. > > > > 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock > > "tpm-crb-cmd", cannot accept migration > > 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading > > state for instance 0x0 of device 'ram' > > 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration > > failed: Invalid argument > > > > Stefan > > To be clear, you are asking to back out of 4/11? That patch changes > how the registers are mapped so it's impossible to support the old > style register mapping. This patch attempts to fix that with a > >>> > >>> Why can we not keep the old style register mapping as 'secondary mapping'? > >> > >> I think the first goal should be for existing TPM CRB device not to change > >> anything, they > >> keep their .read and .write behaivor as it. > >> > >> If you need different .read behavior for the sysbus device due to AARCH64 > >> then it may want to use its own MemoryRegionOps. > >> > >> I am fairly sure that you could refactor the core of the existing > >> tpm_crb_mmio_write() and have it work on s->regs and mmio regs. > >> The former would be used by existing code, the latter for CRB sysbus > >> calling into this new function from a wrapper. > >> > >> Stefan > > > > I agree that new QEMU should be able to read old QEMU state but vice > > versa is not always true. There's been many changes in the past that > > incremented the vmstate's version_id to indicate that the state format > > has changed. Also, we are not changing the .read behavior because in > > Unfortunately the CRB device is being used by x86 on some distros > and the expectation is that this existing device can also downgrade > to a previous version of QEMU I would say. I have read people migrating > from RHEL 9.x even to RHEL 8.x and the expectation is that this works. But would the migration even work due to other parts of QEMU? The only way you can, say, migrate from QEMU 8.1.0 to 8.0.0 is if every single VMstate has its version_id unchanged. Does QEMU provide that guarantee? I'm fine with changing it but just want to make sure expectations are set correctly. Have you tested a downgrade and found that no other device impeded the process? > > Now you are introducing a new device and I think you can leave > the existing device with its s->regs alone and have the new device > with its mmio regs work a little different just to preserve the QEMU > downgrade for x86. > > > the old code, the only field that gets a dynamic update is > > tpmEstablished which we found is never changed. So effectively, .read > > Correct and that's why you don't need a .read in the new device. > > > is just doing a memcpy of the `regs` state. This makes it possible to > > map the page as memory while retaining the same behavior as before. > > (We are changing the code but not the behavior). > > > > The issue with Windows's buggy tpm.sys driver is that fundamentally it > > cannot work with MemoryRegionOps. The way MMIO is implemented is that > > At least not with the .read part as it seems and you have to have the > .write part to be able to react to cmd transfers etc. > > > a hole is left in the guest memory space so when the device registers > > are accessed, the hypervisor traps it and sends it over to QEMU to > > handle. QEMU looks up the address, sees its a valid MMIO mapping, and > > calls into the MemoryRegionOps implementation. When tpm.sys does a LDP > > instruction access to the hole, the information for QEMU to determine > > if it's a valid access is not provided. Other hypervisors like Apple's > > VZ.framework and VMware will read the guest PC, manually decode the > > AArch64 instruction, determine the type of access, read the guest Rn > > registers, does a TLB lookup to determine the physical address, then > > emulate the MMIO. None of this capability currently exists in QEMU's > > ARM64 backend. That's why we decided the
Re: [PATCH] treewide: spelling fixes in comments and some strings
On Fri, 14 Jul 2023 21:50:01 +0300
Michael Tokarev wrote:
> 14.07.2023 21:22, Alex Williamson wrote:
> > On Fri, 14 Jul 2023 14:38:06 +0300
> > Michael Tokarev wrote:
> >
> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> >> index d8aeee0b7e..12e7790cf6 100644
> >> --- a/hw/ppc/spapr_pci_vfio.c
> >> +++ b/hw/ppc/spapr_pci_vfio.c
> >> @@ -39,7 +39,7 @@ static void spapr_phb_vfio_eeh_reenable(SpaprPhbState
> >> *sphb)
> >> void spapr_phb_vfio_reset(DeviceState *qdev)
> >> {
> >> /*
> >> - * The PE might be in frozen state. To reenable the EEH
> >> + * The PE might be in frozen state. To re-enable the EEH
> >>* functionality on it will clean the frozen state, which
> >>* ensures that the contained PCI devices will work properly
> >>* after reboot.
> >
> > This looks like personal preference, I can't actually find a source
> > that indicates "reenable" is anything other than a valid alternative of
> > "re-enable". Thanks,
>
> Well, it was one of the questionable suggestions from codespell. I like
> the re-enable better but in this case I don't really care for one way or
> another. I can drop this and other similar fixes. This one definitely
> does not hurt my eyes when I see it ;)
>
> $ git diff qemu-master..spelling | grep -c re-enable
> 8
>
> I can drop those 8 out of 400 :)
If we consider codespell to be the authority I guess we can leave it,
but it seems a bit pedantic to me. Thanks,
Alex
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 14:49, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger wrote: On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan I agree that new QEMU should be able to read old QEMU state but vice versa is not always true. There's been many changes in the past that incremented the vmstate's version_id to indicate that the state format has changed. Also, we are not changing the .read behavior because in Unfortunately the CRB device is being used by x86 on some distros and the expectation is that this existing device can also downgrade to a previous version of QEMU I would say. I have read people migrating from RHEL 9.x even to RHEL 8.x and the expectation is that this works. Now you are introducing a new device and I think you can leave the existing device with its s->regs alone and have the new device with its mmio regs work a little different just to preserve the QEMU downgrade for x86. the old code, the only field that gets a dynamic update is tpmEstablished which we found is never changed. So effectively, .read Correct and that's why you don't need a .read in the new device. is just doing a memcpy of the `regs` state. This makes it possible to map the page as memory while retaining the same behavior as before. (We are changing the code but not the behavior). The issue with Windows's buggy tpm.sys driver is that fundamentally it cannot work with MemoryRegionOps. The way MMIO is implemented is that At least not with the .read part as it seems and you have to have the .write part to be able to react to cmd transfers etc. a hole is left in the guest memory space so when the device registers are accessed, the hypervisor traps it and sends it over to QEMU to handle. QEMU looks up the address, sees its a valid MMIO mapping, and calls into the MemoryRegionOps implementation. When tpm.sys does a LDP instruction access to the hole, the information for QEMU to determine if it's a valid access is not provided. Other hypervisors like Apple's VZ.framework and VMware will read the guest PC, manually decode the AArch64 instruction, determine the type of access, read the guest Rn registers, does a TLB lookup to determine the physical address, then emulate the MMIO. None of this capability currently exists in QEMU's ARM64 backend. That's why we decided the easier path is to tell QEMU that this mapping is RAM for read purposes and MMIO only for write purposes (thankfully Windows does not do a STP or we'd be hosed). Thanks, this confirms what I thought. Stefan
Re: [PATCH] treewide: spelling fixes in comments and some strings
14.07.2023 21:22, Alex Williamson wrote:
On Fri, 14 Jul 2023 14:38:06 +0300
Michael Tokarev wrote:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index d8aeee0b7e..12e7790cf6 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -39,7 +39,7 @@ static void spapr_phb_vfio_eeh_reenable(SpaprPhbState *sphb)
void spapr_phb_vfio_reset(DeviceState *qdev)
{
/*
- * The PE might be in frozen state. To reenable the EEH
+ * The PE might be in frozen state. To re-enable the EEH
* functionality on it will clean the frozen state, which
* ensures that the contained PCI devices will work properly
* after reboot.
This looks like personal preference, I can't actually find a source
that indicates "reenable" is anything other than a valid alternative of
"re-enable". Thanks,
Well, it was one of the questionable suggestions from codespell. I like
the re-enable better but in this case I don't really care for one way or
another. I can drop this and other similar fixes. This one definitely
does not hurt my eyes when I see it ;)
$ git diff qemu-master..spelling | grep -c re-enable
8
I can drop those 8 out of 400 :)
Thanks,
/mjt
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On Fri, Jul 14, 2023 at 11:41 AM Stefan Berger wrote: > > > > On 7/14/23 14:22, Stefan Berger wrote: > > On 7/14/23 13:04, Joelle van Dyne wrote: > >> On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger > >> wrote: > >>> > >>> > >>> > >>> On 7/14/23 10:05, Stefan Berger wrote: > > > On 7/14/23 03:09, Joelle van Dyne wrote: > > When we moved to a single mapping and modified TPM CRB's VMState, it > > broke restoring of VMs that were saved on an older version. This > > change allows those VMs to gracefully migrate to the new memory > > mapping. > > Thanks. This has to be in 4/11 though. > > >>> > >>> After applying the whole series and trying to resume state taken with > >>> current git > >>> master I cannot restore it but it leads to this error here. I would just > >>> leave it > >>> completely untouched in 4/11. > >>> > >>> 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock > >>> "tpm-crb-cmd", cannot accept migration > >>> 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state > >>> for instance 0x0 of device 'ram' > >>> 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: > >>> Invalid argument > >>> > >>> Stefan > >> > >> To be clear, you are asking to back out of 4/11? That patch changes > >> how the registers are mapped so it's impossible to support the old > >> style register mapping. This patch attempts to fix that with a > > > > Why can we not keep the old style register mapping as 'secondary mapping'? > > I think the first goal should be for existing TPM CRB device not to change > anything, they > keep their .read and .write behaivor as it. > > If you need different .read behavior for the sysbus device due to AARCH64 > then it may want to use its own MemoryRegionOps. > > I am fairly sure that you could refactor the core of the existing > tpm_crb_mmio_write() and have it work on s->regs and mmio regs. > The former would be used by existing code, the latter for CRB sysbus calling > into this new function from a wrapper. > > Stefan I agree that new QEMU should be able to read old QEMU state but vice versa is not always true. There's been many changes in the past that incremented the vmstate's version_id to indicate that the state format has changed. Also, we are not changing the .read behavior because in the old code, the only field that gets a dynamic update is tpmEstablished which we found is never changed. So effectively, .read is just doing a memcpy of the `regs` state. This makes it possible to map the page as memory while retaining the same behavior as before. (We are changing the code but not the behavior). The issue with Windows's buggy tpm.sys driver is that fundamentally it cannot work with MemoryRegionOps. The way MMIO is implemented is that a hole is left in the guest memory space so when the device registers are accessed, the hypervisor traps it and sends it over to QEMU to handle. QEMU looks up the address, sees its a valid MMIO mapping, and calls into the MemoryRegionOps implementation. When tpm.sys does a LDP instruction access to the hole, the information for QEMU to determine if it's a valid access is not provided. Other hypervisors like Apple's VZ.framework and VMware will read the guest PC, manually decode the AArch64 instruction, determine the type of access, read the guest Rn registers, does a TLB lookup to determine the physical address, then emulate the MMIO. None of this capability currently exists in QEMU's ARM64 backend. That's why we decided the easier path is to tell QEMU that this mapping is RAM for read purposes and MMIO only for write purposes (thankfully Windows does not do a STP or we'd be hosed).
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 14:22, Stefan Berger wrote: On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? I think the first goal should be for existing TPM CRB device not to change anything, they keep their .read and .write behaivor as it. If you need different .read behavior for the sysbus device due to AARCH64 then it may want to use its own MemoryRegionOps. I am fairly sure that you could refactor the core of the existing tpm_crb_mmio_write() and have it work on s->regs and mmio regs. The former would be used by existing code, the latter for CRB sysbus calling into this new function from a wrapper. Stefan
[PATCH v2] plugins: Set final instruction count in plugin_gen_tb_end
Translation logic may partially decode an instruction, then abort and
remove the instruction from the TB. This can happen for example when an
instruction spans two pages. In this case, plugins may get an incorrect
result when calling qemu_plugin_tb_n_insns to query for the number of
instructions in the TB. This patch updates plugin_gen_tb_end to set the
final instruction count.
Signed-off-by: Matt Borgerson
---
accel/tcg/plugin-gen.c| 5 -
accel/tcg/translator.c| 2 +-
include/exec/plugin-gen.h | 4 ++--
3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 5c13615112..f18ecd6902 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -866,10 +866,13 @@ void plugin_gen_insn_end(void)
* do any clean-up here and make sure things are reset in
* plugin_gen_tb_start.
*/
-void plugin_gen_tb_end(CPUState *cpu)
+void plugin_gen_tb_end(CPUState *cpu, size_t num_insns)
{
struct qemu_plugin_tb *ptb = tcg_ctx->plugin_tb;
+/* translator may have removed instructions, update final count */
+ptb->n = num_insns;
+
/* collect instrumentation requests */
qemu_plugin_tb_trans_cb(cpu, ptb);
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 0fd9efceba..141f514886 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -215,7 +215,7 @@ void translator_loop(CPUState *cpu,
TranslationBlock *tb, int *max_insns,
gen_tb_end(tb, cflags, icount_start_insn, db->num_insns);
if (plugin_enabled) {
-plugin_gen_tb_end(cpu);
+plugin_gen_tb_end(cpu, db->num_insns);
}
/* The disas_log hook may use these values rather than recompute. */
diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h
index 52828781bc..c4552b5061 100644
--- a/include/exec/plugin-gen.h
+++ b/include/exec/plugin-gen.h
@@ -20,7 +20,7 @@ struct DisasContextBase;
bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db,
bool supress);
-void plugin_gen_tb_end(CPUState *cpu);
+void plugin_gen_tb_end(CPUState *cpu, size_t num_insns);
void plugin_gen_insn_start(CPUState *cpu, const struct DisasContextBase *db);
void plugin_gen_insn_end(void);
@@ -42,7 +42,7 @@ void plugin_gen_insn_start(CPUState *cpu, const
struct DisasContextBase *db)
static inline void plugin_gen_insn_end(void)
{ }
-static inline void plugin_gen_tb_end(CPUState *cpu)
+static inline void plugin_gen_tb_end(CPUState *cpu, size_t num_insns)
{ }
static inline void plugin_gen_disable_mem_helpers(void)
--
2.34.1
Re: [PATCH] plugins: Set final instruction count in plugin_gen_tb_end
Hi Philippe, > num_insns is a 'size_t'. You are right. I copied the `int` type from `DisasContextBase`, but it should really be `size_t`. I'll send an updated patch. Thanks, Matt On Fri, Jul 14, 2023 at 11:09 AM Philippe Mathieu-Daudé wrote: > > Hi Matt, > > On 14/7/23 06:18, Matt Borgerson wrote: > > Translation logic may partially decode an instruction, then abort and > > remove the instruction from the TB. This can happen for example when an > > instruction spans two pages. In this case, plugins may get an incorrect > > result when calling qemu_plugin_tb_n_insns to query for the number of > > instructions in the TB. This patch updates plugin_gen_tb_end to set the > > final instruction count. > > > > Signed-off-by: Matt Borgerson > > --- > > accel/tcg/plugin-gen.c| 5 - > > accel/tcg/translator.c| 2 +- > > include/exec/plugin-gen.h | 4 ++-- > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h > > index 52828781bc..4feaa47b08 100644 > > --- a/include/exec/plugin-gen.h > > +++ b/include/exec/plugin-gen.h > > @@ -20,7 +20,7 @@ struct DisasContextBase; > > > > bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db, > >bool supress); > > -void plugin_gen_tb_end(CPUState *cpu); > > +void plugin_gen_tb_end(CPUState *cpu, int num_insns); > > num_insns is a 'size_t'.
[PATCH] hw/virtio-iommu: Fix potential OOB access in virtio_iommu_handle_command()
In the virtio_iommu_handle_command() when a PROBE request is handled,
output_size takes a value greater than the tail size and on a subsequent
iteration we can get a stack out-of-band access. Initialize the
output_size on each iteration.
The issue was found with ASAN. Credits to:
Yiming Tao(Zhejiang University)
Gaoning Pan(Zhejiang University)
Fixes: 1733eebb9e7 ("virtio-iommu: Implement RESV_MEM probe request")
Signed-off-by: Eric Auger
Reported-by: Mauro Matteo Cascella
---
hw/virtio/virtio-iommu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 201127c488..4dcf1d5c62 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -728,13 +728,15 @@ static void virtio_iommu_handle_command(VirtIODevice
*vdev, VirtQueue *vq)
VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
struct virtio_iommu_req_head head;
struct virtio_iommu_req_tail tail = {};
-size_t output_size = sizeof(tail), sz;
VirtQueueElement *elem;
unsigned int iov_cnt;
struct iovec *iov;
void *buf = NULL;
+size_t sz;
for (;;) {
+size_t output_size = sizeof(tail);
+
elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
if (!elem) {
return;
--
2.38.1
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 13:04, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a Why can we not keep the old style register mapping as 'secondary mapping'? The expectation is that old VM state / CRB state can be used by the new QEMU and also new QEMU CRB state should be readable by old QEMU. So in a way the old 'secondary' mmaping has to hold the true value of the registers so that the latter works. migration path but I realized that I missed the "tpm-crb-cmd" ramblock. It can be added to v3 for this patch. Similar to the logic to have `legacy_regs` we will add a `legacy_cmdmem` memory region that is not registered with the system bus but only exists to migrate the data. Would that work? Also open to better ideas on migrating legacy saved state. If we back out of 4/11 (the split mapping), then the proposed way for working on Apple Silicon would not be available and we would have to go down the path of emulating AArch64 instruction in HVF backend (or decide not to support Apple Silicon).
Re: [PATCH] treewide: spelling fixes in comments and some strings
On Fri, 14 Jul 2023 14:38:06 +0300
Michael Tokarev wrote:
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index d8aeee0b7e..12e7790cf6 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -39,7 +39,7 @@ static void spapr_phb_vfio_eeh_reenable(SpaprPhbState *sphb)
> void spapr_phb_vfio_reset(DeviceState *qdev)
> {
> /*
> - * The PE might be in frozen state. To reenable the EEH
> + * The PE might be in frozen state. To re-enable the EEH
> * functionality on it will clean the frozen state, which
> * ensures that the contained PCI devices will work properly
> * after reboot.
This looks like personal preference, I can't actually find a source
that indicates "reenable" is anything other than a valid alternative of
"re-enable". Thanks,
Alex
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On Fri, Jul 14, 2023 at 11:01 AM Stefan Berger wrote: > > > > On 7/14/23 13:46, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger > > wrote: > >> > >> > >> > >> On 7/14/23 13:39, Joelle van Dyne wrote: > >>> On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger > >>> wrote: > > > > On 7/14/23 13:29, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger > > wrote: > >> > >> > >> > >> > >> I don't know whether we would want multiple devices. tpm_find() usage > >> is certainly not prepared for multiple devices. > > Sorry, "multiple TPM interfaces" here does not mean "at the same > > time". Will clarify the description. > > > >> > >> > >> Good for the consolidation. > >> > >> > >> Does moving the TIS to a different address help on aarch64? > > That was the first thing we tried and no it doesn't help. > > I would remove it if we don't have a known alternative address that > makes it work. If we do, I think we should document it in tpm.rst. > >>> "It" is referring to tpm-tis-device? Note that it does work fine with > >>> Linux VMs. > >> > >> yes, tpm_tis_sysbus and I know it works with Liunux but I see this > >> discussion here around Win 11 on aarch64. Why do we need to user another > >> address than the standard address if for Win 11 on aarch64 it doesn't get > >> it to work. > > The standard address won't work for Linux either. > > > > TPM TIS on standard address on ARM64 Virt machines = collision with > > DRAM, will not instantiate > > I thought that this was working with Linux on the aarch64 virt board as > contributed by Eric Auger. > > https://github.com/qemu/qemu/commit/fcaa204194e15ba24cd53087dd616aabbc29e64f > > Also I had tested it to some extent: > https://github.com/stefanberger/swtpm/issues/493#issuecomment-885221109 I think I know where the confusion is. Both your examples use "tpm-tis-device" which indeed uses the SysBus and gets a dynamic address. In this patch, the removed code that generates the AML gets this address by poking into the SysBus device, getting its base, then adding the offset from the TIS device to it. In the change, we move that code to get the address to earlier in the Virt init sequence (before the machine is realized) in order to tell the TIS device what its own base address is. Then, during the AML generation phase, we can just tell the TIS device to "generate your own AML" which is now possible because it knows its own base address. This is also how the TIS ISA bus device does it but that is simpler because it can just use the default address. Separately, there is a `build_tpm2` table function which also needs the device base address but only for CRB devices (TIS has the field set to 0) so it's irrelevant here. > > > > > TPM TIS on SysBus with dynamically allocated address = works on Linux, > > cannot start on Windows > > > >> > >>> > > > >> > >> Can the size really be an option? I don't see it useful and if one > >> gave the wrong size it may break things. > > It was added for consistency (otherwise we have to determine the size > > by looking at the interface everywhere). Also, it is possible for the > > size to be larger than the constant. For example, Apple Silicon uses > > 16KiB page sizes and we may decide to force the device to be 16KiB > > aligned (not sure if this is needed yet while we still track down why > > the dual mapping was not working). In that case, we would need to > > inform the OS of the true region size to prevent any overlap issues. > > Both baseaddr and size should be provided only by the plug handler in > > the virt machine, otherwise things may break even if we get rid of > > size and have just an incorrect baseaddr. > > > >> > >> > >>
Re: [PATCH] plugins: Set final instruction count in plugin_gen_tb_end
Hi Matt, On 14/7/23 06:18, Matt Borgerson wrote: Translation logic may partially decode an instruction, then abort and remove the instruction from the TB. This can happen for example when an instruction spans two pages. In this case, plugins may get an incorrect result when calling qemu_plugin_tb_n_insns to query for the number of instructions in the TB. This patch updates plugin_gen_tb_end to set the final instruction count. Signed-off-by: Matt Borgerson --- accel/tcg/plugin-gen.c| 5 - accel/tcg/translator.c| 2 +- include/exec/plugin-gen.h | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/exec/plugin-gen.h b/include/exec/plugin-gen.h index 52828781bc..4feaa47b08 100644 --- a/include/exec/plugin-gen.h +++ b/include/exec/plugin-gen.h @@ -20,7 +20,7 @@ struct DisasContextBase; bool plugin_gen_tb_start(CPUState *cpu, const struct DisasContextBase *db, bool supress); -void plugin_gen_tb_end(CPUState *cpu); +void plugin_gen_tb_end(CPUState *cpu, int num_insns); num_insns is a 'size_t'.
Re: [PULL v1 1/1] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 13:58, Philippe Mathieu-Daudé wrote:
Hi Stefan,
On 14/7/23 17:41, Stefan Berger wrote:
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Message-id: 20230713171955.149236-1-stef...@linux.ibm.com
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
- DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
Since properties are user-facing, shouldn't we deprecate their
removal? I'm not sure so I ask :) Otherwise we could register
the property with object_class_property_add_bool() and have
the setter display a warning. Anyhow I suppose now setting
"ppi" triggers some error, which is better than a abort.
ppi=on crashed it, now it doesn't crash it. On the next level, ppi=on may come
with the expectation that ppi is working on aarch64 and I am not sure about
this.
DEFINE_PROP_END_OF_LIST(),
};
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:46, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger wrote: On 7/14/23 13:39, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger wrote: On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. The standard address won't work for Linux either. TPM TIS on standard address on ARM64 Virt machines = collision with DRAM, will not instantiate I thought that this was working with Linux on the aarch64 virt board as contributed by Eric Auger. https://github.com/qemu/qemu/commit/fcaa204194e15ba24cd53087dd616aabbc29e64f Also I had tested it to some extent: https://github.com/stefanberger/swtpm/issues/493#issuecomment-885221109 TPM TIS on SysBus with dynamically allocated address = works on Linux, cannot start on Windows Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
Re: [PULL v1 1/1] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
Hi Stefan,
On 14/7/23 17:41, Stefan Berger wrote:
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Message-id: 20230713171955.149236-1-stef...@linux.ibm.com
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
-DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
Since properties are user-facing, shouldn't we deprecate their
removal? I'm not sure so I ask :) Otherwise we could register
the property with object_class_property_add_bool() and have
the setter display a warning. Anyhow I suppose now setting
"ppi" triggers some error, which is better than a abort.
DEFINE_PROP_END_OF_LIST(),
};
Re: [PATCH v2 06/11] tpm_crb: move ACPI table building to device interface
On 7/14/23 03:09, Joelle van Dyne wrote:
This logic is similar to TPM TIS ISA device. Since TPM CRB can only
support TPM 2.0 backends, we check for this in realize.
Signed-off-by: Joelle van Dyne
This patch changes the order of in which the ACPI table elements are created
but doesn't matter and also doesn't seem to upset ACPI test cases from what I
saw:
Reviewed-by: Stefan Berger
---
hw/i386/acpi-build.c | 23 ---
hw/tpm/tpm_crb.c | 29 +
2 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9c74fa17ad..b767df39df 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1441,9 +1441,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
uint32_t nr_mem = machine->ram_slots;
int root_bus_limit = 0xFF;
PCIBus *bus = NULL;
-#ifdef CONFIG_TPM
-TPMIf *tpm = tpm_find();
-#endif
bool cxl_present = false;
int i;
VMBusBridge *vmbus_bridge = vmbus_bridge_find();
@@ -1793,26 +1790,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
}
}
-#ifdef CONFIG_TPM
-if (TPM_IS_CRB(tpm)) {
-dev = aml_device("TPM");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR",
- aml_string("TPM 2.0 Device")));
-crs = aml_resource_template();
-aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE,
- TPM_CRB_ADDR_SIZE, AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-
-aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
-aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-
-tpm_build_ppi_acpi(tpm, dev);
-
-aml_append(sb_scope, dev);
-}
-#endif
-
if (pcms->sgx_epc.size != 0) {
uint64_t epc_base = pcms->sgx_epc.base;
uint64_t epc_size = pcms->sgx_epc.size;
diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
index 6144081d30..594696ffb8 100644
--- a/hw/tpm/tpm_crb.c
+++ b/hw/tpm/tpm_crb.c
@@ -19,6 +19,8 @@
#include "qemu/module.h"
#include "qapi/error.h"
#include "exec/address-spaces.h"
+#include "hw/acpi/acpi_aml_interface.h"
+#include "hw/acpi/tpm.h"
#include "hw/qdev-properties.h"
#include "hw/pci/pci_ids.h"
#include "hw/acpi/tpm.h"
@@ -99,6 +101,11 @@ static void tpm_crb_isa_realize(DeviceState *dev, Error
**errp)
return;
}
+if (tpm_crb_isa_get_version(TPM_IF(s)) != TPM_VERSION_2_0) {
+error_setg(errp, "TPM CRB only supports TPM 2.0 backends");
+return;
+}
+
tpm_crb_init_memory(OBJECT(s), >state, errp);
memory_region_add_subregion(isa_address_space(ISA_DEVICE(dev)),
@@ -116,10 +123,30 @@ static void tpm_crb_isa_realize(DeviceState *dev, Error
**errp)
}
}
+static void build_tpm_crb_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
+{
+Aml *dev, *crs;
+CRBState *s = CRB(adev);
+TPMIf *ti = TPM_IF(s);
+
+dev = aml_device("TPM");
+aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
+aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
+aml_append(dev, aml_name_decl("_UID", aml_int(1)));
+aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+crs = aml_resource_template();
+aml_append(crs, aml_memory32_fixed(TPM_CRB_ADDR_BASE, TPM_CRB_ADDR_SIZE,
+ AML_READ_WRITE));
+aml_append(dev, aml_name_decl("_CRS", crs));
+tpm_build_ppi_acpi(ti, dev);
+aml_append(scope, dev);
+}
+
static void tpm_crb_isa_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
TPMIfClass *tc = TPM_IF_CLASS(klass);
+AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
dc->realize = tpm_crb_isa_realize;
device_class_set_props(dc, tpm_crb_isa_properties);
@@ -128,6 +155,7 @@ static void tpm_crb_isa_class_init(ObjectClass *klass, void
*data)
tc->model = TPM_MODEL_TPM_CRB;
tc->get_version = tpm_crb_isa_get_version;
tc->request_completed = tpm_crb_isa_request_completed;
+adevc->build_dev_aml = build_tpm_crb_isa_aml;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
}
@@ -139,6 +167,7 @@ static const TypeInfo tpm_crb_isa_info = {
.class_init = tpm_crb_isa_class_init,
.interfaces = (InterfaceInfo[]) {
{ TYPE_TPM_IF },
+{ TYPE_ACPI_DEV_AML_IF },
{ }
}
};
Re: [PATCH v2 10/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 7/14/23 13:20, Joelle van Dyne wrote:
On Fri, Jul 14, 2023 at 7:27 AM Stefan Berger wrote:
On 7/14/23 03:09, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64. We largely follow the TPM TIS SysBus device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbud-fdt.c` and change the added line from:
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
---
docs/specs/tpm.rst | 1 +
include/hw/acpi/aml-build.h | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 170
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 2 +
13 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
If you added the command line to use for Windows guests to this document
I think this would be helpful. And also mention that this must be used for
Windows
since the other ones don't work.
SPAPR interface
---
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d1fb08514b..9660e16148 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -3,6 +3,7 @@
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/bios-linker-loader.h"
+#include "exec/hwaddr.h"
#define ACPI_BUILD_APPNAME6 "BOCHS "
#define ACPI_BUILD_APPNAME8 "BXPC"
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 66e3b45f30..f79c8f3575 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -47,6 +47,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -56,6 +57,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..f809137fc9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
} else if (TPM_IS_CRB(tpmif)) {
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
+} else if
Re: [PATCH] target/sparc: Handle FPRS correctly on big-endian hosts
Hi Peter,
On 14/7/23 19:26, Peter Maydell wrote:
In CPUSparcState we define the fprs field as uint64_t. However we
then refer to it in translate.c via a TCGv_i32 which we set up with
tcg_global_mem_new_ptr(). This means that on a big-endian host when
the guest does something to writo te the FPRS register this value
ends up in the wrong half of the uint64_t, and the QEMU C code that
refers to env->fprs sees the wrong value. The effect of this is that
guest code that enables the FPU crashes with spurious FPU Disabled
exceptions. In particular, this is why
tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
times out on an s390 host.
There are multiple ways we could fix this; since there are actually
only three bits in the FPRS register and the code in translate.c
would be a bit painful to convert to dealing with a TCGv_i64, change
the type of the CPU state struct field to match what translate.c is
expecting.
(None of the other fields referenced by the r32[] array in
sparc_tcg_init() have the wrong type.)
Signed-off-by: Peter Maydell
---
Another in my occasional series of "fix an avocado failure on
s390" Friday afternoon patches :-)
:)
diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
index a1c8fdc4d55..bddb9609b7b 100644
--- a/target/sparc/gdbstub.c
+++ b/target/sparc/gdbstub.c
@@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
case 83:
return gdb_get_regl(mem_buf, env->fsr);
case 84:
-return gdb_get_regl(mem_buf, env->fprs);
+{
+target_ulong fprs = env->fprs;
+return gdb_get_regl(mem_buf, fprs);
Why not return gdb_get_reg32() ?
+}
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On Fri, Jul 14, 2023 at 10:43 AM Stefan Berger wrote: > > > > On 7/14/23 13:39, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger > > wrote: > >> > >> > >> > >> On 7/14/23 13:29, Joelle van Dyne wrote: > >>> On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger > >>> wrote: > > > > > I don't know whether we would want multiple devices. tpm_find() usage is > certainly not prepared for multiple devices. > >>> Sorry, "multiple TPM interfaces" here does not mean "at the same > >>> time". Will clarify the description. > >>> > > > Good for the consolidation. > > > Does moving the TIS to a different address help on aarch64? > >>> That was the first thing we tried and no it doesn't help. > >> > >> I would remove it if we don't have a known alternative address that makes > >> it work. If we do, I think we should document it in tpm.rst. > > "It" is referring to tpm-tis-device? Note that it does work fine with Linux > > VMs. > > yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion > here around Win 11 on aarch64. Why do we need to user another address than > the standard address if for Win 11 on aarch64 it doesn't get it to work. The standard address won't work for Linux either. TPM TIS on standard address on ARM64 Virt machines = collision with DRAM, will not instantiate TPM TIS on SysBus with dynamically allocated address = works on Linux, cannot start on Windows > > > > >> > >> > > Can the size really be an option? I don't see it useful and if one gave > the wrong size it may break things. > >>> It was added for consistency (otherwise we have to determine the size > >>> by looking at the interface everywhere). Also, it is possible for the > >>> size to be larger than the constant. For example, Apple Silicon uses > >>> 16KiB page sizes and we may decide to force the device to be 16KiB > >>> aligned (not sure if this is needed yet while we still track down why > >>> the dual mapping was not working). In that case, we would need to > >>> inform the OS of the true region size to prevent any overlap issues. > >>> Both baseaddr and size should be provided only by the plug handler in > >>> the virt machine, otherwise things may break even if we get rid of > >>> size and have just an incorrect baseaddr. > >>> > > >
[PATCH for-8.2 v3 4/8] target/riscv/cpu.c: split non-ratified exts from riscv_cpu_extensions[]
Create a new riscv_cpu_experimental_exts[] to store the non-ratified
extensions properties. Once they are ratified we'll move them back to
riscv_cpu_extensions[].
Change riscv_cpu_add_user_properties to keep adding them to users.
Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
---
target/riscv/cpu.c | 36 +---
1 file changed, 21 insertions(+), 15 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d9c097f602..5689368f02 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1808,21 +1808,6 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
-/* These are experimental so mark with 'x-' */
-DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
-
-/* ePMP 0.9.3 */
-DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
-DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
-DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
-
-DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
-DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
-
-DEFINE_PROP_BOOL("x-zfbfmin", RISCVCPU, cfg.ext_zfbfmin, false),
-DEFINE_PROP_BOOL("x-zvfbfmin", RISCVCPU, cfg.ext_zvfbfmin, false),
-DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false),
-
DEFINE_PROP_END_OF_LIST(),
};
@@ -1841,6 +1826,23 @@ static Property riscv_cpu_vendor_exts[] = {
DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps,
false),
};
+/* These are experimental so mark with 'x-' */
+static Property riscv_cpu_experimental_exts[] = {
+DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
+
+/* ePMP 0.9.3 */
+DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
+DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
+DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
+
+DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
+DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
+
+DEFINE_PROP_BOOL("x-zfbfmin", RISCVCPU, cfg.ext_zfbfmin, false),
+DEFINE_PROP_BOOL("x-zvfbfmin", RISCVCPU, cfg.ext_zvfbfmin, false),
+DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false),
+};
+
static Property riscv_cpu_options[] = {
DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
@@ -1923,6 +1925,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
for (int i = 0; i < ARRAY_SIZE(riscv_cpu_vendor_exts); i++) {
qdev_property_add_static(dev, _cpu_vendor_exts[i]);
}
+
+for (int i = 0; i < ARRAY_SIZE(riscv_cpu_experimental_exts); i++) {
+qdev_property_add_static(dev, _cpu_experimental_exts[i]);
+}
}
static Property riscv_cpu_properties[] = {
--
2.41.0
[PATCH for-8.2 v3 3/8] target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[]
Our goal is to make riscv_cpu_extensions[] hold only ratified,
non-vendor extensions.
Create a new riscv_cpu_vendor_exts[] array for them, changing
riscv_cpu_add_user_properties() accordingly.
Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
---
target/riscv/cpu.c | 33 +++--
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b165ecfcba..d9c097f602 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1808,20 +1808,6 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_BOOL("zcmp", RISCVCPU, cfg.ext_zcmp, false),
DEFINE_PROP_BOOL("zcmt", RISCVCPU, cfg.ext_zcmt, false),
-/* Vendor-specific custom extensions */
-DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
-DEFINE_PROP_BOOL("xtheadbb", RISCVCPU, cfg.ext_xtheadbb, false),
-DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),
-DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
-DEFINE_PROP_BOOL("xtheadcondmov", RISCVCPU, cfg.ext_xtheadcondmov, false),
-DEFINE_PROP_BOOL("xtheadfmemidx", RISCVCPU, cfg.ext_xtheadfmemidx, false),
-DEFINE_PROP_BOOL("xtheadfmv", RISCVCPU, cfg.ext_xtheadfmv, false),
-DEFINE_PROP_BOOL("xtheadmac", RISCVCPU, cfg.ext_xtheadmac, false),
-DEFINE_PROP_BOOL("xtheadmemidx", RISCVCPU, cfg.ext_xtheadmemidx, false),
-DEFINE_PROP_BOOL("xtheadmempair", RISCVCPU, cfg.ext_xtheadmempair, false),
-DEFINE_PROP_BOOL("xtheadsync", RISCVCPU, cfg.ext_xtheadsync, false),
-DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps,
false),
-
/* These are experimental so mark with 'x-' */
DEFINE_PROP_BOOL("x-zicond", RISCVCPU, cfg.ext_zicond, false),
@@ -1840,6 +1826,21 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_END_OF_LIST(),
};
+static Property riscv_cpu_vendor_exts[] = {
+DEFINE_PROP_BOOL("xtheadba", RISCVCPU, cfg.ext_xtheadba, false),
+DEFINE_PROP_BOOL("xtheadbb", RISCVCPU, cfg.ext_xtheadbb, false),
+DEFINE_PROP_BOOL("xtheadbs", RISCVCPU, cfg.ext_xtheadbs, false),
+DEFINE_PROP_BOOL("xtheadcmo", RISCVCPU, cfg.ext_xtheadcmo, false),
+DEFINE_PROP_BOOL("xtheadcondmov", RISCVCPU, cfg.ext_xtheadcondmov, false),
+DEFINE_PROP_BOOL("xtheadfmemidx", RISCVCPU, cfg.ext_xtheadfmemidx, false),
+DEFINE_PROP_BOOL("xtheadfmv", RISCVCPU, cfg.ext_xtheadfmv, false),
+DEFINE_PROP_BOOL("xtheadmac", RISCVCPU, cfg.ext_xtheadmac, false),
+DEFINE_PROP_BOOL("xtheadmemidx", RISCVCPU, cfg.ext_xtheadmemidx, false),
+DEFINE_PROP_BOOL("xtheadmempair", RISCVCPU, cfg.ext_xtheadmempair, false),
+DEFINE_PROP_BOOL("xtheadsync", RISCVCPU, cfg.ext_xtheadsync, false),
+DEFINE_PROP_BOOL("xventanacondops", RISCVCPU, cfg.ext_XVentanaCondOps,
false),
+};
+
static Property riscv_cpu_options[] = {
DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
@@ -1918,6 +1919,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
qdev_property_add_static(dev, _cpu_options[i]);
}
+
+for (int i = 0; i < ARRAY_SIZE(riscv_cpu_vendor_exts); i++) {
+qdev_property_add_static(dev, _cpu_vendor_exts[i]);
+}
}
static Property riscv_cpu_properties[] = {
--
2.41.0
[PATCH for-8.2 v3 0/8] target/riscv: add 'max' CPU, deprecate
Hi,
This version has changes suggested in v2. The most significant change is
the deprecation of the 'any' CPU in patch 8.
The reasoning behind it is that Alistair mentioned that the 'any' CPU
intended to work like the newly added 'max' CPU, so we're better of
removing the 'any' CPU since it'll be out of place. We can't just
remove the CPU out of the gate so we'll have to make it do with
deprecation first.
Patches missing review: 5,6,7,8
Changes from v2:
- patches 1, 3, 4:
- remove "DEFINE_PROP_END_OF_LIST()" at the end of each prop array;
- use ARRAY_SIZE() in the for loop
- patch 5:
- remove the trailing '/' in the last line of the macro
- wrap the macro in "do {} while (0)"
- patch 8 (new):
- deprecate the 'any' CPU
- v2 link:
https://lore.kernel.org/qemu-riscv/20230712205748.446931-1-dbarb...@ventanamicro.com/
Daniel Henrique Barboza (8):
target/riscv/cpu.c: split CPU options from riscv_cpu_extensions[]
target/riscv/cpu.c: skip 'bool' check when filtering KVM props
target/riscv/cpu.c: split vendor exts from riscv_cpu_extensions[]
target/riscv/cpu.c: split non-ratified exts from
riscv_cpu_extensions[]
target/riscv/cpu.c: add a ADD_CPU_PROPERTIES_ARRAY() macro
target/riscv: add 'max' CPU type
avocado, risc-v: add opensbi tests for 'max' CPU
target/riscv: deprecate the 'any' CPU type
docs/about/deprecated.rst | 12
target/riscv/cpu-qom.h | 1 +
target/riscv/cpu.c | 114 ++---
tests/avocado/riscv_opensbi.py | 16 +
4 files changed, 121 insertions(+), 22 deletions(-)
--
2.41.0
[PATCH for-8.2 v3 5/8] target/riscv/cpu.c: add a ADD_CPU_PROPERTIES_ARRAY() macro
The code inside riscv_cpu_add_user_properties() became quite repetitive
after recent changes. Add a macro to hide the repetition away.
Signed-off-by: Daniel Henrique Barboza
---
target/riscv/cpu.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5689368f02..f7083b2d5c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1875,6 +1875,13 @@ static void cpu_set_cfg_unavailable(Object *obj, Visitor
*v,
}
#endif
+#define ADD_CPU_PROPERTIES_ARRAY(_dev, _array) \
+do { \
+for (int i = 0; i < ARRAY_SIZE(_array); i++) { \
+qdev_property_add_static(_dev, &_array[i]); \
+} \
+} while (0)
+
/*
* Add CPU properties with user-facing flags.
*
@@ -1918,17 +1925,9 @@ static void riscv_cpu_add_user_properties(Object *obj)
qdev_property_add_static(dev, prop);
}
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
-qdev_property_add_static(dev, _cpu_options[i]);
-}
-
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_vendor_exts); i++) {
-qdev_property_add_static(dev, _cpu_vendor_exts[i]);
-}
-
-for (int i = 0; i < ARRAY_SIZE(riscv_cpu_experimental_exts); i++) {
-qdev_property_add_static(dev, _cpu_experimental_exts[i]);
-}
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_options);
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_vendor_exts);
+ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
}
static Property riscv_cpu_properties[] = {
--
2.41.0
[PATCH for-8.2 v3 6/8] target/riscv: add 'max' CPU type
The 'max' CPU type is used by tooling to determine what's the most
capable CPU a current QEMU version implements. Other archs such as ARM
implements this type. Let's add it to RISC-V.
What we consider "most capable CPU" in this context are related to
ratified, non-vendor extensions. This means that we want the 'max' CPU
to enable all (possible) ratified extensions by default. The reasoning
behind this design is (1) vendor extensions can conflict with each other
and we won't play favorities deciding which one is default or not and
(2) non-ratified extensions are always prone to changes, not being
stable enough to be enabled by default.
All this said, we're still not able to enable all ratified extensions
due to conflicts between them. Zfinx and all its dependencies aren't
enabled because of a conflict with RVF. zce, zcmp and zcmt are also
disabled due to RVD conflicts. When running with 64 bits we're also
disabling zcf.
MISA bits RVG, RVJ and RVV are also being set manually since they're
default disabled.
This is the resulting 'riscv,isa' DT for this new CPU:
rv64imafdcvh_zicbom_zicboz_zicsr_zifencei_zihintpause_zawrs_zfa_
zfh_zfhmin_zca_zcb_zcd_zba_zbb_zbc_zbkb_zbkc_zbkx_zbs_zk_zkn_zknd_
zkne_zknh_zkr_zks_zksed_zksh_zkt_zve32f_zve64f_zve64d_
smstateen_sscofpmf_sstc_svadu_svinval_svnapot_svpbmt
Signed-off-by: Daniel Henrique Barboza
---
target/riscv/cpu-qom.h | 1 +
target/riscv/cpu.c | 53 ++
2 files changed, 54 insertions(+)
diff --git a/target/riscv/cpu-qom.h b/target/riscv/cpu-qom.h
index 04af50983e..f3fbe37a2c 100644
--- a/target/riscv/cpu-qom.h
+++ b/target/riscv/cpu-qom.h
@@ -30,6 +30,7 @@
#define CPU_RESOLVING_TYPE TYPE_RISCV_CPU
#define TYPE_RISCV_CPU_ANY RISCV_CPU_TYPE_NAME("any")
+#define TYPE_RISCV_CPU_MAX RISCV_CPU_TYPE_NAME("max")
#define TYPE_RISCV_CPU_BASE32 RISCV_CPU_TYPE_NAME("rv32")
#define TYPE_RISCV_CPU_BASE64 RISCV_CPU_TYPE_NAME("rv64")
#define TYPE_RISCV_CPU_BASE128 RISCV_CPU_TYPE_NAME("x-rv128")
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f7083b2d5c..1cdffd5927 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -248,6 +248,7 @@ static const char * const riscv_intr_names[] = {
};
static void riscv_cpu_add_user_properties(Object *obj);
+static void riscv_init_max_cpu_extensions(Object *obj);
const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
{
@@ -374,6 +375,25 @@ static void riscv_any_cpu_init(Object *obj)
cpu->cfg.pmp = true;
}
+static void riscv_max_cpu_init(Object *obj)
+{
+RISCVCPU *cpu = RISCV_CPU(obj);
+CPURISCVState *env = >env;
+RISCVMXL mlx = MXL_RV64;
+
+#ifdef TARGET_RISCV32
+mlx = MXL_RV32;
+#endif
+set_misa(env, mlx, 0);
+riscv_cpu_add_user_properties(obj);
+riscv_init_max_cpu_extensions(obj);
+env->priv_ver = PRIV_VERSION_LATEST;
+#ifndef CONFIG_USER_ONLY
+set_satp_mode_max_supported(RISCV_CPU(obj), mlx == MXL_RV32 ?
+VM_1_10_SV32 : VM_1_10_SV57);
+#endif
+}
+
#if defined(TARGET_RISCV64)
static void rv64_base_cpu_init(Object *obj)
{
@@ -1930,6 +1950,38 @@ static void riscv_cpu_add_user_properties(Object *obj)
ADD_CPU_PROPERTIES_ARRAY(dev, riscv_cpu_experimental_exts);
}
+/*
+ * The 'max' type CPU will have all possible ratified
+ * non-vendor extensions enabled.
+ */
+static void riscv_init_max_cpu_extensions(Object *obj)
+{
+RISCVCPU *cpu = RISCV_CPU(obj);
+CPURISCVState *env = >env;
+Property *prop;
+
+/* Enable RVG, RVJ and RVV that are disabled by default */
+set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
+
+for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
+object_property_set_bool(obj, prop->name, true, NULL);
+}
+
+/* Zfinx is not compatible with F. Disable it */
+object_property_set_bool(obj, "zfinx", false, NULL);
+object_property_set_bool(obj, "zdinx", false, NULL);
+object_property_set_bool(obj, "zhinx", false, NULL);
+object_property_set_bool(obj, "zhinxmin", false, NULL);
+
+object_property_set_bool(obj, "zce", false, NULL);
+object_property_set_bool(obj, "zcmp", false, NULL);
+object_property_set_bool(obj, "zcmt", false, NULL);
+
+if (env->misa_mxl != MXL_RV32) {
+object_property_set_bool(obj, "zcf", false, NULL);
+}
+}
+
static Property riscv_cpu_properties[] = {
DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
@@ -2268,6 +2320,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
.abstract = true,
},
DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_ANY, riscv_any_cpu_init),
+DEFINE_DYNAMIC_CPU(TYPE_RISCV_CPU_MAX, riscv_max_cpu_init),
#if defined(CONFIG_KVM)
DEFINE_CPU(TYPE_RISCV_CPU_HOST, riscv_host_cpu_init),
#endif
--
2.41.0
[PATCH for-8.2 v3 8/8] target/riscv: deprecate the 'any' CPU type
The 'any' CPU type was introduced in commit dc5bd18fa5725 ("RISC-V CPU
Core Definition"), being around since the beginning. It's not an easy
CPU to use: it's undocumented and its name doesn't tell users much about
what the CPU is supposed to bring. 'git log' doesn't help us either in
knowing what was the original design of this CPU type.
The closest we have is a comment from Alistair [1] where he recalls from
memory that the 'any' CPU is supposed to behave like the newly added
'max' CPU. He also suggested that the 'any' CPU should be removed.
The default CPUs are rv32 and rv64, so removing the 'any' CPU will have
impact only on users that might have a script that uses '-cpu any'.
And those users are better off using the default CPUs or the new 'max'
CPU.
We would love to just remove the code and be done with it, but one does
not simply remove a feature in QEMU. We'll put the CPU in quarantine
first, letting users know that we have the intent of removing it in the
future.
[1] https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg02891.html
Signed-off-by: Daniel Henrique Barboza
---
docs/about/deprecated.rst | 12
target/riscv/cpu.c| 5 +
2 files changed, 17 insertions(+)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 02ea5a839f..68afa43fd0 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -371,6 +371,18 @@ QEMU's ``vhost`` feature, which would eliminate the high
latency costs under
which the 9p ``proxy`` backend currently suffers. However as of to date nobody
has indicated plans for such kind of reimplemention unfortunately.
+RISC-V 'any' CPU type ``-cpu any`` (since 8.2)
+^^
+
+The 'any' CPU type was introduced back in 2018 and has been around since the
+initial RISC-V QEMU port. Its usage has always been unclear: users don't know
+what to expect from a CPU called 'any', and in fact the CPU does not do
anything
+special that aren't already done by the default CPUs rv32/rv64.
+
+After the introduction of the 'max' CPU type RISC-V now has a good coverage
+of generic CPUs: rv32 and rv64 as default CPUs and 'max' as a feature complete
+CPU for both 32 and 64 bit builds. Users are then discouraged to use the 'any'
+CPU type starting in 8.2.
Block device options
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1cdffd5927..0f7c76e286 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1470,6 +1470,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error
**errp)
RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
Error *local_err = NULL;
+if (object_dynamic_cast(OBJECT(dev), TYPE_RISCV_CPU_ANY) != NULL) {
+warn_report("The 'any' CPU is deprecated and will be "
+"removed in the future.");
+}
+
cpu_exec_realizefn(cs, _err);
if (local_err != NULL) {
error_propagate(errp, local_err);
--
2.41.0
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:39, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger wrote: On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. yes, tpm_tis_sysbus and I know it works with Liunux but I see this discussion here around Win 11 on aarch64. Why do we need to user another address than the standard address if for Win 11 on aarch64 it doesn't get it to work. Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
[PATCH for-8.2 v3 2/8] target/riscv/cpu.c: skip 'bool' check when filtering KVM props
After the introduction of riscv_cpu_options[] all properties in
riscv_cpu_extensions[] are booleans. This check is now obsolete.
Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
---
target/riscv/cpu.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3b49a696ed..b165ecfcba 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1905,17 +1905,11 @@ static void riscv_cpu_add_user_properties(Object *obj)
* Set the default to disabled for every extension
* unknown to KVM and error out if the user attempts
* to enable any of them.
- *
- * We're giving a pass for non-bool properties since they're
- * not related to the availability of extensions and can be
- * safely ignored as is.
*/
-if (prop->info == _prop_bool) {
-object_property_add(obj, prop->name, "bool",
-NULL, cpu_set_cfg_unavailable,
-NULL, (void *)prop->name);
-continue;
-}
+object_property_add(obj, prop->name, "bool",
+NULL, cpu_set_cfg_unavailable,
+NULL, (void *)prop->name);
+continue;
}
#endif
qdev_property_add_static(dev, prop);
--
2.41.0
[PATCH for-8.2 v3 1/8] target/riscv/cpu.c: split CPU options from riscv_cpu_extensions[]
We'll add a new CPU type that will enable a considerable amount of
extensions. To make it easier for us we'll do a few cleanups in our
existing riscv_cpu_extensions[] array.
Start by splitting all CPU non-boolean options from it. Create a new
riscv_cpu_options[] array for them. Add all these properties in
riscv_cpu_add_user_properties() as it is already being done today.
No functional changes made.
Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
---
target/riscv/cpu.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 9339c0241d..3b49a696ed 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1751,7 +1751,6 @@ static void riscv_cpu_add_misa_properties(Object *cpu_obj)
static Property riscv_cpu_extensions[] = {
/* Defaults for standard extensions */
-DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf, false),
DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
@@ -1767,11 +1766,6 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
DEFINE_PROP_BOOL("sstc", RISCVCPU, cfg.ext_sstc, true),
-DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
-DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
-DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
-DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
-
DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
@@ -1802,9 +1796,7 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_BOOL("zhinxmin", RISCVCPU, cfg.ext_zhinxmin, false),
DEFINE_PROP_BOOL("zicbom", RISCVCPU, cfg.ext_icbom, true),
-DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
DEFINE_PROP_BOOL("zicboz", RISCVCPU, cfg.ext_icboz, true),
-DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
DEFINE_PROP_BOOL("zmmul", RISCVCPU, cfg.ext_zmmul, false),
@@ -1848,6 +1840,18 @@ static Property riscv_cpu_extensions[] = {
DEFINE_PROP_END_OF_LIST(),
};
+static Property riscv_cpu_options[] = {
+DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
+
+DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
+DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
+
+DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
+DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
+
+DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
+DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
+};
#ifndef CONFIG_USER_ONLY
static void cpu_set_cfg_unavailable(Object *obj, Visitor *v,
@@ -1916,6 +1920,10 @@ static void riscv_cpu_add_user_properties(Object *obj)
#endif
qdev_property_add_static(dev, prop);
}
+
+for (int i = 0; i < ARRAY_SIZE(riscv_cpu_options); i++) {
+qdev_property_add_static(dev, _cpu_options[i]);
+}
}
static Property riscv_cpu_properties[] = {
--
2.41.0
[PATCH for-8.2 v3 7/8] avocado, risc-v: add opensbi tests for 'max' CPU
Add smoke tests to ensure that we'll not break the 'max' CPU type when adding new ratified extensions to be enabled. Signed-off-by: Daniel Henrique Barboza --- tests/avocado/riscv_opensbi.py | 16 1 file changed, 16 insertions(+) diff --git a/tests/avocado/riscv_opensbi.py b/tests/avocado/riscv_opensbi.py index bfff9cc3c3..15fd57fe51 100644 --- a/tests/avocado/riscv_opensbi.py +++ b/tests/avocado/riscv_opensbi.py @@ -61,3 +61,19 @@ def test_riscv64_virt(self): :avocado: tags=machine:virt """ self.boot_opensbi() + +def test_riscv32_virt_maxcpu(self): +""" +:avocado: tags=arch:riscv32 +:avocado: tags=machine:virt +:avocado: tags=cpu:max +""" +self.boot_opensbi() + +def test_riscv64_virt_maxcpu(self): +""" +:avocado: tags=arch:riscv64 +:avocado: tags=machine:virt +:avocado: tags=cpu:max +""" +self.boot_opensbi() -- 2.41.0
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On Fri, Jul 14, 2023 at 10:37 AM Stefan Berger wrote: > > > > On 7/14/23 13:29, Joelle van Dyne wrote: > > On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: > >> > >> > >> > >> > >> I don't know whether we would want multiple devices. tpm_find() usage is > >> certainly not prepared for multiple devices. > > Sorry, "multiple TPM interfaces" here does not mean "at the same > > time". Will clarify the description. > > > >> > >> > >> Good for the consolidation. > >> > >> > >> Does moving the TIS to a different address help on aarch64? > > That was the first thing we tried and no it doesn't help. > > I would remove it if we don't have a known alternative address that makes it > work. If we do, I think we should document it in tpm.rst. "It" is referring to tpm-tis-device? Note that it does work fine with Linux VMs. > > > >> > >> Can the size really be an option? I don't see it useful and if one gave > >> the wrong size it may break things. > > It was added for consistency (otherwise we have to determine the size > > by looking at the interface everywhere). Also, it is possible for the > > size to be larger than the constant. For example, Apple Silicon uses > > 16KiB page sizes and we may decide to force the device to be 16KiB > > aligned (not sure if this is needed yet while we still track down why > > the dual mapping was not working). In that case, we would need to > > inform the OS of the true region size to prevent any overlap issues. > > Both baseaddr and size should be provided only by the plug handler in > > the virt machine, otherwise things may break even if we get rid of > > size and have just an incorrect baseaddr. > > > >> > >> > >>
Re: [PATCH 04/11] tpm_crb: use a single read-as-mem/write-as-mmio mapping
On Fri, Jul 14, 2023 at 4:57 AM Stefan Berger wrote: > > > > On 7/14/23 06:05, Peter Maydell wrote: > > On Thu, 13 Jul 2023 at 19:43, Stefan Berger wrote: > >> > >> > >> > >> On 7/13/23 13:18, Peter Maydell wrote: > >>> On Thu, 13 Jul 2023 at 18:16, Stefan Berger wrote: > I guess the first point would be to decide whether to support an i2c bus > on the virt board and then whether we can use the aspeed bus that we > know that the tpm_tis_i2c device model works with but we don't know how > Windows may react to it. > > It seems sysbus is already supported there so ... we may have a 'match'? > >>> > >>> You can use sysbus devices anywhere -- they're just > >> > >> 'anywhere' also includes aarch64 virt board I suppose. > > > > Yes. Literally any machine can have memory mapped devices. > > > >>> "this is a memory mapped device". The question is whether > >>> we should, or whether an i2c controller is more like > >>> what the real world uses (and if so, what i2c controller). > >>> > >> > >>> I don't want to accept changes to the virt board that are > >>> hard to live with in future, because changing virt in > >>> non-backward compatible ways is painful. > >> > >> Once we have the CRB sysbus device we would keep it around forever and it > >> seems to > >> - not require any changes to the virt board (iiuc) since sysbus is already > >> being used > >> - works already with Windows and probably also Linux > > > > "Add a sysbus device to the virt board" is the kind of > > change I mean -- once you do that it's hard to take it > > out again, and if we decide in 6 months time that actually > > i2c would be the better option then we end up with two > > different ways to do the same thing and trying to > > deprecate the other one is a pain. > > > At least CRB is a standard interface and from this perspective we are fine. I > am not sure what would drive the introduction of the i2c bus in 6 months. I > suppose one could then still use sysbus CRB device or the i2c device. The > sysbus CRB device should still work then. Anyway, I think we should continue > with this series. > > Stefan > > > > > -- PMM FWIW the Windows 11 tpm.sys driver does not support the I2C interface. The driver only recognizes ACPI devices and the case for Start Method = 12 (FIFO Interface over I2C bus) goes to an error handler.
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 13:29, Joelle van Dyne wrote: On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: I don't know whether we would want multiple devices. tpm_find() usage is certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. Good for the consolidation. Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. I would remove it if we don't have a known alternative address that makes it work. If we do, I think we should document it in tpm.rst. Can the size really be an option? I don't see it useful and if one gave the wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr.
Re: [PATCH] target/sparc: Handle FPRS correctly on big-endian hosts
On Fri, 14 Jul 2023 at 18:26, Peter Maydell wrote:
>
> In CPUSparcState we define the fprs field as uint64_t. However we
> then refer to it in translate.c via a TCGv_i32 which we set up with
> tcg_global_mem_new_ptr(). This means that on a big-endian host when
> the guest does something to writo te the FPRS register this value
> ends up in the wrong half of the uint64_t, and the QEMU C code that
> refers to env->fprs sees the wrong value. The effect of this is that
> guest code that enables the FPU crashes with spurious FPU Disabled
> exceptions. In particular, this is why
> tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
> times out on an s390 host.
>
> There are multiple ways we could fix this; since there are actually
> only three bits in the FPRS register and the code in translate.c
> would be a bit painful to convert to dealing with a TCGv_i64, change
> the type of the CPU state struct field to match what translate.c is
> expecting.
> diff --git a/target/sparc/machine.c b/target/sparc/machine.c
> index 44b9e7d75d6..5a8502804a4 100644
> --- a/target/sparc/machine.c
> +++ b/target/sparc/machine.c
> @@ -168,7 +168,8 @@ const VMStateDescription vmstate_sparc_cpu = {
> VMSTATE_UINT64_ARRAY(env.bgregs, SPARCCPU, 8),
> VMSTATE_UINT64_ARRAY(env.igregs, SPARCCPU, 8),
> VMSTATE_UINT64_ARRAY(env.mgregs, SPARCCPU, 8),
> -VMSTATE_UINT64(env.fprs, SPARCCPU),
> +VMSTATE_UINT32(env.fprs, SPARCCPU),
> +VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
Whoops, these two fields are the wrong way around. The on-the-wire
format for migration is always big-endian, so we need to put
the VMSTATE_UNUSED() placeholder before the VMSTATE_UINT32(),
not after it.
thanks
-- PMM
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On Fri, Jul 14, 2023 at 9:19 AM Stefan Berger wrote: > > > > > I don't know whether we would want multiple devices. tpm_find() usage is > certainly not prepared for multiple devices. Sorry, "multiple TPM interfaces" here does not mean "at the same time". Will clarify the description. > > > Good for the consolidation. > > > Does moving the TIS to a different address help on aarch64? That was the first thing we tried and no it doesn't help. > > Can the size really be an option? I don't see it useful and if one gave the > wrong size it may break things. It was added for consistency (otherwise we have to determine the size by looking at the interface everywhere). Also, it is possible for the size to be larger than the constant. For example, Apple Silicon uses 16KiB page sizes and we may decide to force the device to be 16KiB aligned (not sure if this is needed yet while we still track down why the dual mapping was not working). In that case, we would need to inform the OS of the true region size to prevent any overlap issues. Both baseaddr and size should be provided only by the plug handler in the virt machine, otherwise things may break even if we get rid of size and have just an incorrect baseaddr. > > >
[PATCH] target/sparc: Handle FPRS correctly on big-endian hosts
In CPUSparcState we define the fprs field as uint64_t. However we
then refer to it in translate.c via a TCGv_i32 which we set up with
tcg_global_mem_new_ptr(). This means that on a big-endian host when
the guest does something to writo te the FPRS register this value
ends up in the wrong half of the uint64_t, and the QEMU C code that
refers to env->fprs sees the wrong value. The effect of this is that
guest code that enables the FPU crashes with spurious FPU Disabled
exceptions. In particular, this is why
tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
times out on an s390 host.
There are multiple ways we could fix this; since there are actually
only three bits in the FPRS register and the code in translate.c
would be a bit painful to convert to dealing with a TCGv_i64, change
the type of the CPU state struct field to match what translate.c is
expecting.
(None of the other fields referenced by the r32[] array in
sparc_tcg_init() have the wrong type.)
Signed-off-by: Peter Maydell
---
Another in my occasional series of "fix an avocado failure on
s390" Friday afternoon patches :-)
target/sparc/cpu.h | 2 +-
target/sparc/cpu.c | 4 ++--
target/sparc/gdbstub.c | 5 -
target/sparc/machine.c | 3 ++-
target/sparc/monitor.c | 2 +-
5 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 95d2d0da71d..98044572f26 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -521,7 +521,7 @@ struct CPUArchState {
uint64_t igregs[8]; /* interrupt general registers */
uint64_t mgregs[8]; /* mmu general registers */
uint64_t glregs[8 * MAXTL_MAX];
-uint64_t fprs;
+uint32_t fprs;
uint64_t tick_cmpr, stick_cmpr;
CPUTimer *tick, *stick;
#define TICK_NPT_MASK0x8000ULL
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index e329a7aece5..130ab8f5781 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -673,8 +673,8 @@ static void sparc_cpu_dump_state(CPUState *cs, FILE *f, int
flags)
"cleanwin: %d cwp: %d\n",
env->cansave, env->canrestore, env->otherwin, env->wstate,
env->cleanwin, env->nwindows - 1 - env->cwp);
-qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs: "
- TARGET_FMT_lx "\n", env->fsr, env->y, env->fprs);
+qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs:
%016x\n",
+ env->fsr, env->y, env->fprs);
#else
qemu_fprintf(f, "psr: %08x (icc: ", cpu_get_psr(env));
diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
index a1c8fdc4d55..bddb9609b7b 100644
--- a/target/sparc/gdbstub.c
+++ b/target/sparc/gdbstub.c
@@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
case 83:
return gdb_get_regl(mem_buf, env->fsr);
case 84:
-return gdb_get_regl(mem_buf, env->fprs);
+{
+target_ulong fprs = env->fprs;
+return gdb_get_regl(mem_buf, fprs);
+}
case 85:
return gdb_get_regl(mem_buf, env->y);
}
diff --git a/target/sparc/machine.c b/target/sparc/machine.c
index 44b9e7d75d6..5a8502804a4 100644
--- a/target/sparc/machine.c
+++ b/target/sparc/machine.c
@@ -168,7 +168,8 @@ const VMStateDescription vmstate_sparc_cpu = {
VMSTATE_UINT64_ARRAY(env.bgregs, SPARCCPU, 8),
VMSTATE_UINT64_ARRAY(env.igregs, SPARCCPU, 8),
VMSTATE_UINT64_ARRAY(env.mgregs, SPARCCPU, 8),
-VMSTATE_UINT64(env.fprs, SPARCCPU),
+VMSTATE_UINT32(env.fprs, SPARCCPU),
+VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
VMSTATE_UINT64(env.tick_cmpr, SPARCCPU),
VMSTATE_UINT64(env.stick_cmpr, SPARCCPU),
VMSTATE_CPU_TIMER(env.tick, SPARCCPU),
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index 318413686aa..73f15aa272d 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -154,7 +154,7 @@ const MonitorDef monitor_defs[] = {
{ "otherwin", offsetof(CPUSPARCState, otherwin) },
{ "wstate", offsetof(CPUSPARCState, wstate) },
{ "cleanwin", offsetof(CPUSPARCState, cleanwin) },
-{ "fprs", offsetof(CPUSPARCState, fprs) },
+{ "fprs", offsetof(CPUSPARCState, fprs), NULL, MD_I32 },
#endif
{ NULL },
};
--
2.34.1
Re: [PATCH v2 10/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
On Fri, Jul 14, 2023 at 7:27 AM Stefan Berger wrote:
>
>
>
> On 7/14/23 03:09, Joelle van Dyne wrote:
> > This SysBus variant of the CRB interface supports dynamically locating
> > the MMIO interface so that Virt machines can use it. This interface
> > is currently the only one supported by QEMU that works on Windows 11
> > ARM64. We largely follow the TPM TIS SysBus device as a template.
> >
> > To try out this device with Windows 11 before OVMF is updated, you
> > will need to modify `sysbud-fdt.c` and change the added line from:
> >
> > ```c
> > TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
> > ```
> >
> > to
> >
> > ```c
> > TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
> > ```
> >
> > This change was not included because it can confuse Linux (although
> > from testing, it seems like Linux is able to properly ignore the
> > device from the TPM TIS driver and recognize it from the ACPI device
> > in the TPM CRB driver). A proper fix would require OVMF to recognize
> > the ACPI device and not depend on the FDT node for recognizing TPM.
> >
> > The command line to try out this device with SWTPM is:
> >
> > ```
> > $ qemu-system-aarch64 \
> > -chardev socket,id=chrtpm0,path=tpm.sock \
> > -tpmdev emulator,id=tpm0,chardev=chrtpm0 \
> > -device tpm-crb-device,tpmdev=tpm0
> > ```
> >
> > along with SWTPM:
> >
> > ```
> > $ swtpm \
> > --ctrl type=unixio,path=tpm.sock,terminate \
> > --tpmstate backend-uri=file://tpm.data \
> > --tpm2
> > ```
> >
> > Signed-off-by: Joelle van Dyne
> > ---
> > docs/specs/tpm.rst | 1 +
> > include/hw/acpi/aml-build.h | 1 +
> > include/sysemu/tpm.h| 3 +
> > hw/acpi/aml-build.c | 7 +-
> > hw/arm/virt.c | 1 +
> > hw/core/sysbus-fdt.c| 1 +
> > hw/loongarch/virt.c | 1 +
> > hw/riscv/virt.c | 1 +
> > hw/tpm/tpm_crb_sysbus.c | 170
> > hw/arm/Kconfig | 1 +
> > hw/riscv/Kconfig| 1 +
> > hw/tpm/Kconfig | 5 ++
> > hw/tpm/meson.build | 2 +
> > 13 files changed, 194 insertions(+), 1 deletion(-)
> > create mode 100644 hw/tpm/tpm_crb_sysbus.c
> >
> > diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
> > index 2bc29c9804..95aeb49220 100644
> > --- a/docs/specs/tpm.rst
> > +++ b/docs/specs/tpm.rst
> > @@ -46,6 +46,7 @@ operating system.
> > QEMU files related to TPM CRB interface:
> >- ``hw/tpm/tpm_crb.c``
> >- ``hw/tpm/tpm_crb_common.c``
> > + - ``hw/tpm/tpm_crb_sysbus.c``
> >
>
> If you added the command line to use for Windows guests to this document
> I think this would be helpful. And also mention that this must be used for
> Windows
> since the other ones don't work.
>
>
> > SPAPR interface
> > ---
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index d1fb08514b..9660e16148 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -3,6 +3,7 @@
> >
> > #include "hw/acpi/acpi-defs.h"
> > #include "hw/acpi/bios-linker-loader.h"
> > +#include "exec/hwaddr.h"
> >
> > #define ACPI_BUILD_APPNAME6 "BOCHS "
> > #define ACPI_BUILD_APPNAME8 "BXPC"
> > diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> > index 66e3b45f30..f79c8f3575 100644
> > --- a/include/sysemu/tpm.h
> > +++ b/include/sysemu/tpm.h
> > @@ -47,6 +47,7 @@ struct TPMIfClass {
> > #define TYPE_TPM_TIS_ISA"tpm-tis"
> > #define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
> > #define TYPE_TPM_CRB"tpm-crb"
> > +#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
> > #define TYPE_TPM_SPAPR "tpm-spapr"
> > #define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
> >
> > @@ -56,6 +57,8 @@ struct TPMIfClass {
> > object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
> > #define TPM_IS_CRB(chr) \
> > object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
> > +#define TPM_IS_CRB_SYSBUS(chr) \
> > +object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
> > #define TPM_IS_SPAPR(chr) \
> > object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
> > #define TPM_IS_TIS_I2C(chr) \
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index ea331a20d1..f809137fc9 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -31,6 +31,7 @@
> > #include "hw/pci/pci_bus.h"
> > #include "hw/pci/pci_bridge.h"
> > #include "qemu/cutils.h"
> > +#include "qom/object.h"
> >
> > static GArray *build_alloc_array(void)
> > {
> > @@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker
> > *linker, GArray *tcpalog,
> > {
> > uint8_t start_method_params[12] = {};
> > unsigned log_addr_offset;
> > -uint64_t control_area_start_address;
> > +uint64_t baseaddr,
Re: [PATCH v2 07/11] hw/arm/virt: add plug handler for TPM on SysBus
On Fri, Jul 14, 2023 at 5:11 AM Stefan Berger wrote:
>
>
>
> On 7/14/23 03:09, Joelle van Dyne wrote:
> > TPM needs to know its own base address in order to generate its DSDT
> > device entry.
> >
> > Signed-off-by: Joelle van Dyne
> > ---
> > hw/arm/virt.c | 37 +
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 7d9dbc2663..432148ef47 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -2732,6 +2732,37 @@ static void virt_memory_plug(HotplugHandler
> > *hotplug_dev,
> >dev, _abort);
> > }
> >
> > +#ifdef CONFIG_TPM
> > +static void virt_tpm_plug(VirtMachineState *vms, TPMIf *tpmif)
> > +{
> > +PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
> > +hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
>
> +static void virt_tpm_plug(LoongArchMachineState *lams, TPMIf *tpmif)
> +{
> +PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(lams->platform_bus_dev);
> +hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;
>
> These seem to be the differences between the loongarch and the arm virt
> implementations.
> Why not have a function with this signature that both archs can call?
>
> static void virt_tpm_plug(PlatformBusDevice *pbus, hwaddr pbus_base, TPMIf
> *tpmif)
>
> Regards,
> Stefan
That was the first intended solution, but the code that this is
replacing was copy-pasted and as I don't know anything about this arch
or know how to test it, this caused the least amount of changes to
that target architecture (no additional includes, changes to the build
scripts, etc). I don't think there is currently a common "virt" module
that all the "virt" targets use so we would have to create that. It
seemed out of scope for this TPM patchset and could be something for a
different patch set. tl;dr: the code it replaces was copy-pasted so
it's not strictly making things worse.
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On Fri, Jul 14, 2023 at 7:51 AM Stefan Berger wrote: > > > > On 7/14/23 10:05, Stefan Berger wrote: > > > > > > On 7/14/23 03:09, Joelle van Dyne wrote: > >> When we moved to a single mapping and modified TPM CRB's VMState, it > >> broke restoring of VMs that were saved on an older version. This > >> change allows those VMs to gracefully migrate to the new memory > >> mapping. > > > > Thanks. This has to be in 4/11 though. > > > > After applying the whole series and trying to resume state taken with current > git > master I cannot restore it but it leads to this error here. I would just > leave it > completely untouched in 4/11. > > 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock > "tpm-crb-cmd", cannot accept migration > 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for > instance 0x0 of device 'ram' > 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: > Invalid argument > > Stefan To be clear, you are asking to back out of 4/11? That patch changes how the registers are mapped so it's impossible to support the old style register mapping. This patch attempts to fix that with a migration path but I realized that I missed the "tpm-crb-cmd" ramblock. It can be added to v3 for this patch. Similar to the logic to have `legacy_regs` we will add a `legacy_cmdmem` memory region that is not registered with the system bus but only exists to migrate the data. Would that work? Also open to better ideas on migrating legacy saved state. If we back out of 4/11 (the split mapping), then the proposed way for working on Apple Silicon would not be available and we would have to go down the path of emulating AArch64 instruction in HVF backend (or decide not to support Apple Silicon).
Re: [PATCH 1/1] qemu-nbd: fix regression with qemu-nbd --fork run over ssh
On 7/14/23 17:35, Eric Blake wrote:
On Thu, Jul 06, 2023 at 09:15:45PM +0200, Denis V. Lunev wrote:
Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
Author: Hanna Reitz
Date: Wed May 8 23:18:18 2019 +0200
qemu-nbd: Do not close stderr
has introduced an interesting regression. Original behavior of
ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
was the following:
* qemu-nbd was started as a daemon
* the command execution is done and ssh exited with success
The patch has changed this behavior and 'ssh' command now hangs forever.
According to the normal specification of the daemon() call, we should
endup with STDERR pointing to /dev/null. That should be done at the
very end of the successful startup sequence when the pipe to the
bootstrap process (used for diagnostics) is no longer needed.
This could be achived in the same way as done for 'qemu-nbd -c' case.
That was commit 0eaf453e, also fixing up e6df58a5.
STDOUT copying to STDERR does the trick.
It took me a while to see how this worked (the double-negative of
passing 0 to the 'noclose' parameter of daemon() doesn't make it easy
to track what is supposed to happen), but I agree that our goal is
that after daemon()izing, we temporarily set stderr to the inherited
pipe for passing back any startup error messages, then usually want to
restore it to /dev/null for the remainder of the process.
This also leads to proper 'ssh' connection closing which fixes my
original problem.
Signed-off-by: Denis V. Lunev
CC: Eric Blake
CC: Vladimir Sementsov-Ogievskiy
CC: Hanna Reitz
---
qemu-nbd.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
I indeed see how this fixes a regression under 'fork_process'.
However, the code that calls qemu_daemon() is also reachable under the
condition 'device && !verbose'. Does it make sense for either:
qemu-nbd -v -c /dev/nbd0
qemu-nbd -f -v -c /dev/nbd0
as a way to connect to the kernel device, but explicitly ask to remain
verbose, as a way to debug issues in connecting to the kernel via
stderr?
Going back to the emails at the time of Hanna's original commit...
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01872.html
I don't see any consideration about that case; capturing the original
stderr was done to fix what looked like an easy bug fix to a botched
implementation of old_stderr in commit ffb31e1d without considering
the ramifications.
But seeing as how pre-existing code DID want at least 'qemu-nbd -v -c
/dev/nbd0' to log indefinitely, I think we need to squash in:
diff --git i/qemu-nbd.c w/qemu-nbd.c
index 4276163564b..4d037798b9b 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -313,7 +313,7 @@ static void *nbd_client_thread(void *arg)
/* update partition table */
pthread_create(_parts_thread, NULL, show_parts, device);
-if (verbose) {
+if (verbose && !fork_process) {
fprintf(stderr, "NBD device %s is now connected to %s\n",
device, srcpath);
} else {
With that tweak, I'm fine with adding:
Reviewed-by: Eric Blake
Do you agree with my tweak? If so, I can queue this through my NBD
tree without needing to see a v2 post. However...
I like this fix. Thanks for pointing this out.
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 4276163564..e9e118dfdb 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -575,7 +575,6 @@ int main(int argc, char **argv)
bool writethrough = false; /* Client will flush as needed. */
bool fork_process = false;
bool list = false;
-int old_stderr = -1;
unsigned socket_activation;
const char *pid_file_name = NULL;
const char *selinux_label = NULL;
@@ -930,11 +929,6 @@ int main(int argc, char **argv)
} else if (pid == 0) {
close(stderr_fd[0]);
-/* Remember parent's stderr if we will be restoring it. */
-if (fork_process) {
-old_stderr = dup(STDERR_FILENO);
-}
-
ret = qemu_daemon(1, 0);
/* Temporarily redirect stderr to the parent's pipe... */
@@ -1152,8 +1146,7 @@ int main(int argc, char **argv)
Pre-existing, but your patch made me notice nearby (minor corner-case)
bugs:
ret = qemu_daemon(1, 0);
/* Temporarily redirect stderr to the parent's pipe... */
dup2(stderr_fd[1], STDERR_FILENO);
We aren't checking for dup2() failure. But even if it succeeds (which
we expect), it could have modified the contents of errno compared to
what they were after qemu_daemon()...
if (ret < 0) {
error_report("Failed to daemonize: %s", strerror(errno));
...so this could be reporting garbage. I wouldn't mind additional
patches to make the code more robust against unlikely failure
scenarios.
OK. Will do. Hope that I'll not forget this again.
Den
Re: [PATCH v21 16/20] tests/avocado: s390x cpu topology entitlement tests
On Wed, 2023-07-12 at 22:11 +0200, Thomas Huth wrote:
> On 12/07/2023 21.37, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-07-05 at 12:22 +0200, Thomas Huth wrote:
> > > On 30/06/2023 11.17, Pierre Morel wrote:
> > > > This test takes care to check the changes on different
> > > > entitlements
> > > > when the guest requests a polarization change.
> > > >
> > > > Signed-off-by: Pierre Morel
> > > > ---
> > > > tests/avocado/s390_topology.py | 47
> > > > ++
> > > > 1 file changed, 47 insertions(+)
> > > >
> > > > diff --git a/tests/avocado/s390_topology.py
> > > > b/tests/avocado/s390_topology.py
> > > > index 2cf731cb1d..4855e5d7e4 100644
> > > > --- a/tests/avocado/s390_topology.py
> > > > +++ b/tests/avocado/s390_topology.py
> > > > @@ -240,3 +240,50 @@ def test_polarisation(self):
> > > > res = self.vm.qmp('query-cpu-polarization')
> > > > self.assertEqual(res['return']['polarization'],
> > > > 'horizontal')
> > > > self.check_topology(0, 0, 0, 0, 'medium', False)
> > > > +
> > > > + def test_entitlement(self):
> > > > + """
> > > > + This test verifies that QEMU modifies the polarization
> > > > + after a guest request.
> > > ...
> > > > + self.check_topology(0, 0, 0, 0, 'low', False)
> > > > + self.check_topology(1, 0, 0, 0, 'medium', False)
> > > > + self.check_topology(2, 1, 0, 0, 'high', False)
> > > > + self.check_topology(3, 1, 0, 0, 'high', False)
> > > > +
> > > > + self.guest_set_dispatching('1');
> > > > +
> > > > + self.check_topology(0, 0, 0, 0, 'low', False)
> > > > + self.check_topology(1, 0, 0, 0, 'medium', False)
> > > > + self.check_topology(2, 1, 0, 0, 'high', False)
> > > > + self.check_topology(3, 1, 0, 0, 'high', False)
> > > > +
> > > > + self.guest_set_dispatching('0');
> > > > +
> > > > + self.check_topology(0, 0, 0, 0, 'low', False)
> > > > + self.check_topology(1, 0, 0, 0, 'medium', False)
> > > > + self.check_topology(2, 1, 0, 0, 'high', False)
> > > > + self.check_topology(3, 1, 0, 0, 'high', False)
> > >
> > > Sorry, I think I'm too blind to see it, but what has changed
> > > after
> > > the guest
> > > changed the polarization?
> >
> > Nothing, the values are retained, they're just not active.
> > The guest will see a horizontal polarization until it changes back
> > to
> > vertical.
>
> But then the comment in front of it ("This test verifies that QEMU
> *modifies* the polarization...") does not quite match, does it?
Yeah, it tests that QEMU reports it's own state changed when using
set-cpu-topology.
I think it would be a good idea to get the guests view from the sysfs,
also.
>
> Thomas
>
>
Re: [PATCH v2 09/11] tpm_tis_sysbus: move DSDT AML generation to device
On 7/14/23 03:09, Joelle van Dyne wrote:
This reduces redundent code in different machine types with ACPI table
generation. Additionally, this will allow us to support multiple TPM
interfaces. Finally, this matches up with the TPM TIS ISA
I don't know whether we would want multiple devices. tpm_find() usage is
certainly not prepared for multiple devices.
implementation.
Ideally, we would be able to call `qbus_build_aml` and avoid any TPM
specific code in the ACPI table generation. However, currently we
still have to call `build_tpm2` anyways and it does not look like
most other ACPI devices support the `ACPI_DEV_AML_IF` interface.
Signed-off-by: Joelle van Dyne
---
hw/arm/virt-acpi-build.c | 38 ++
hw/loongarch/acpi-build.c | 38 ++
hw/tpm/tpm_tis_sysbus.c | 35 +++
3 files changed, 39 insertions(+), 72 deletions(-)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6b674231c2..49b2f19440 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -35,6 +35,7 @@
#include "target/arm/cpu.h"
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_aml_interface.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "hw/acpi/aml-build.h"
@@ -208,41 +209,6 @@ static void acpi_dsdt_add_gpio(Aml *scope, const
MemMapEntry *gpio_memmap,
aml_append(scope, dev);
}
-#ifdef CONFIG_TPM
-static void acpi_dsdt_add_tpm(Aml *scope, VirtMachineState *vms)
-{
-PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
-hwaddr pbus_base = vms->memmap[VIRT_PLATFORM_BUS].base;
-SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find());
-MemoryRegion *sbdev_mr;
-hwaddr tpm_base;
-
-if (!sbdev) {
-return;
-}
-
-tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
-assert(tpm_base != -1);
-
-tpm_base += pbus_base;
-
-sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
-
-Aml *dev = aml_device("TPM0");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
-aml_append(dev, aml_name_decl("_UID", aml_int(0)));
-
-Aml *crs = aml_resource_template();
-aml_append(crs,
- aml_memory32_fixed(tpm_base,
- (uint32_t)memory_region_size(sbdev_mr),
- AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
-}
-#endif
-
#define ID_MAPPING_ENTRY_SIZE 20
#define SMMU_V3_ENTRY_SIZE 68
#define ROOT_COMPLEX_ENTRY_SIZE 36
@@ -891,7 +857,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
VirtMachineState *vms)
acpi_dsdt_add_power_button(scope);
#ifdef CONFIG_TPM
-acpi_dsdt_add_tpm(scope, vms);
+call_dev_aml_func(DEVICE(tpm_find()), scope);
#endif
aml_append(dsdt, scope);
diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
index 0b62c3a2f7..4291e670c8 100644
--- a/hw/loongarch/acpi-build.c
+++ b/hw/loongarch/acpi-build.c
@@ -14,6 +14,7 @@
#include "target/loongarch/cpu.h"
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_aml_interface.h"
#include "hw/nvram/fw_cfg.h"
#include "hw/acpi/bios-linker-loader.h"
#include "migration/vmstate.h"
@@ -328,41 +329,6 @@ static void build_flash_aml(Aml *scope,
LoongArchMachineState *lams)
aml_append(scope, dev);
}
-#ifdef CONFIG_TPM
-static void acpi_dsdt_add_tpm(Aml *scope, LoongArchMachineState *vms)
-{
-PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(vms->platform_bus_dev);
-hwaddr pbus_base = VIRT_PLATFORM_BUS_BASEADDRESS;
-SysBusDevice *sbdev = SYS_BUS_DEVICE(tpm_find());
-MemoryRegion *sbdev_mr;
-hwaddr tpm_base;
-
-if (!sbdev) {
-return;
-}
-
-tpm_base = platform_bus_get_mmio_addr(pbus, sbdev, 0);
-assert(tpm_base != -1);
-
-tpm_base += pbus_base;
-
-sbdev_mr = sysbus_mmio_get_region(sbdev, 0);
-
-Aml *dev = aml_device("TPM0");
-aml_append(dev, aml_name_decl("_HID", aml_string("MSFT0101")));
-aml_append(dev, aml_name_decl("_STR", aml_string("TPM 2.0 Device")));
-aml_append(dev, aml_name_decl("_UID", aml_int(0)));
-
-Aml *crs = aml_resource_template();
-aml_append(crs,
- aml_memory32_fixed(tpm_base,
- (uint32_t)memory_region_size(sbdev_mr),
- AML_READ_WRITE));
-aml_append(dev, aml_name_decl("_CRS", crs));
-aml_append(scope, dev);
-}
-#endif
-
Good for the consolidation.
/* build DSDT */
static void
build_dsdt(GArray *table_data, BIOSLinker *linker, MachineState *machine)
@@ -379,7 +345,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
MachineState *machine)
build_la_ged_aml(dsdt, machine);
build_flash_aml(dsdt, lams);
#ifdef
[PATCH 14/14] target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types
The PAR_EL1.SH field documents that for the cases of:
* Device memory
* Normal memory with both Inner and Outer Non-Cacheable
the field should be 0b10 rather than whatever was in the
translation table descriptor field. (In the pseudocode this
is handled by PAREncodeShareability().) Perform this
adjustment when assembling a PAR value.
Signed-off-by: Peter Maydell
---
target/arm/helper.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1e45fdb47c9..f9c00827018 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3342,6 +3342,19 @@ static CPAccessResult ats_access(CPUARMState *env, const
ARMCPRegInfo *ri,
}
#ifdef CONFIG_TCG
+static int par_el1_shareability(GetPhysAddrResult *res)
+{
+/*
+ * The PAR_EL1.SH field must be 0b10 for Device or Normal-NC
+ * memory -- see pseudocode PAREncodeShareability().
+ */
+if (((res->cacheattrs.attrs & 0xf0) == 0) ||
+res->cacheattrs.attrs == 0x44 || res->cacheattrs.attrs == 0x40) {
+return 2;
+}
+return res->cacheattrs.shareability;
+}
+
static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
MMUAccessType access_type, ARMMMUIdx mmu_idx,
bool is_secure)
@@ -3470,7 +3483,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t
value,
par64 |= (1 << 9); /* NS */
}
par64 |= (uint64_t)res.cacheattrs.attrs << 56; /* ATTR */
-par64 |= res.cacheattrs.shareability << 7; /* SH */
+par64 |= par_el1_shareability() << 7; /* SH */
} else {
uint32_t fsr = arm_fi_to_lfsc();
--
2.34.1
[PATCH 05/14] target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled()
Plumb the ARMSecurityState through to regime_translation_disabled()
rather than just a bool is_secure.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index a873fbe0239..63dd8e3cbe1 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -206,9 +206,10 @@ static uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx
mmu_idx, int ttbrn)
/* Return true if the specified stage of address translation is disabled */
static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
-bool is_secure)
+ARMSecuritySpace space)
{
uint64_t hcr_el2;
+bool is_secure = arm_space_is_secure(space);
if (arm_feature(env, ARM_FEATURE_M)) {
switch (env->v7m.mpu_ctrl[is_secure] &
@@ -2057,9 +2058,8 @@ static bool get_phys_addr_pmsav5(CPUARMState *env,
uint32_t base;
ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
bool is_user = regime_is_user(env, mmu_idx);
-bool is_secure = arm_space_is_secure(ptw->in_space);
-if (regime_translation_disabled(env, mmu_idx, is_secure)) {
+if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) {
/* MPU disabled. */
result->f.phys_addr = address;
result->f.prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
@@ -2231,7 +2231,7 @@ static bool get_phys_addr_pmsav7(CPUARMState *env,
result->f.lg_page_size = TARGET_PAGE_BITS;
result->f.prot = 0;
-if (regime_translation_disabled(env, mmu_idx, secure) ||
+if (regime_translation_disabled(env, mmu_idx, ptw->in_space) ||
m_is_ppb_region(env, address)) {
/*
* MPU disabled or M profile PPB access: use default memory map.
@@ -2475,7 +2475,8 @@ bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
* are done in arm_v7m_load_vector(), which always does a direct
* read using address_space_ldl(), rather than going via this function.
*/
-if (regime_translation_disabled(env, mmu_idx, secure)) { /* MPU disabled */
+if (regime_translation_disabled(env, mmu_idx,
arm_secure_to_space(secure))) {
+/* MPU disabled */
hit = true;
} else if (m_is_ppb_region(env, address)) {
hit = true;
@@ -3303,7 +3304,7 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
*/
ptw->in_mmu_idx = mmu_idx = s1_mmu_idx;
if (arm_feature(env, ARM_FEATURE_EL2) &&
-!regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
+!regime_translation_disabled(env, ARMMMUIdx_Stage2,
ptw->in_space)) {
return get_phys_addr_twostage(env, ptw, address, access_type,
result, fi);
}
@@ -3362,7 +3363,7 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
/* Definitely a real MMU, not an MPU */
-if (regime_translation_disabled(env, mmu_idx, is_secure)) {
+if (regime_translation_disabled(env, mmu_idx, ptw->in_space)) {
return get_phys_addr_disabled(env, ptw, address, access_type,
result, fi);
}
--
2.34.1
[PATCH 08/14] target/arm/ptw: Remove last uses of ptw->in_secure
Replace the last uses of ptw->in_secure with appropriate
checks on ptw->in_space.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index c30d3fe69a0..bc834675fb2 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3247,7 +3247,6 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
ARMMMUFaultInfo *fi)
{
ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-bool is_secure = ptw->in_secure;
ARMMMUIdx s1_mmu_idx;
/*
@@ -3255,8 +3254,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
* cannot upgrade a NonSecure translation regime's attributes
* to Secure or Realm.
*/
-result->f.attrs.secure = is_secure;
result->f.attrs.space = ptw->in_space;
+result->f.attrs.secure = arm_space_is_secure(ptw->in_space);
switch (mmu_idx) {
case ARMMMUIdx_Phys_S:
@@ -3270,8 +3269,12 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
case ARMMMUIdx_Stage1_E0:
case ARMMMUIdx_Stage1_E1:
case ARMMMUIdx_Stage1_E1_PAN:
-/* First stage lookup uses second stage for ptw. */
-ptw->in_ptw_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+/*
+ * First stage lookup uses second stage for ptw; only
+ * Secure has both S and NS IPA and starts with Stage2_S.
+ */
+ptw->in_ptw_idx = (ptw->in_space == ARMSS_Secure) ?
+ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
break;
case ARMMMUIdx_Stage2:
--
2.34.1
[PATCH 07/14] target/arm/ptw: Only fold in NSTable bit effects in Secure state
When we do a translation in Secure state, the NSTable bits in table
descriptors may downgrade us to NonSecure; we update ptw->in_secure
and ptw->in_space accordingly. We guard that check correctly with a
conditional that means it's only applied for Secure stage 1
translations. However, later on in get_phys_addr_lpae() we fold the
effects of the NSTable bits into the final descriptor attributes
bits, and there we do it unconditionally regardless of the CPU state.
That means that in Realm state (where in_secure is false) we will set
bit 5 in attrs, and later use it to decide to output to non-secure
space.
We don't in fact need to do this folding in at all any more (since
commit 2f1ff4e7b9f30c): if an NSTable bit was set then we have
already set ptw->in_space to ARMSS_NonSecure, and in that situation
we don't look at attrs bit 5. The only thing we still need to deal
with is the real NS bit in the final descriptor word, so we can just
drop the code that ORed in the NSTable bit.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 9e45160e1ba..c30d3fe69a0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1884,11 +1884,10 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
* Extract attributes from the (modified) descriptor, and apply
* table descriptors. Stage 2 table descriptors do not include
* any attribute fields. HPD disables all the table attributes
- * except NSTable.
+ * except NSTable (which we have already handled).
*/
attrs = new_descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(50,
14));
if (!regime_is_stage2(mmu_idx)) {
-attrs |= !ptw->in_secure << 5; /* NS */
if (!param.hpd) {
attrs |= extract64(tableattrs, 0, 2) << 53; /* XN, PXN */
/*
--
2.34.1
[PATCH 13/14] target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage 1 ptw
When we report faults due to stage 2 faults during a stage 1
page table walk, the 'level' parameter should be the level
of the walk in stage 2 that faulted, not the level of the
walk in stage 1. Correct the reporting of these faults.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index ed46bb82a75..d1de934702d 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2046,9 +2046,13 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
do_translation_fault:
fi->type = ARMFault_Translation;
do_fault:
-fi->level = level;
-/* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2. */
-fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx);
+if (fi->s1ptw) {
+/* Retain the existing stage 2 fi->level */
+assert(fi->stage2);
+} else {
+fi->level = level;
+fi->stage2 = regime_is_stage2(mmu_idx);
+}
fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx);
return true;
}
--
2.34.1
[PATCH 02/14] target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2 faults
In S1_ptw_translate() we set up the ARMMMUFaultInfo if the attempt to
translate the page descriptor address into a physical address fails.
This used to only be possible if we are doing a stage 2 ptw for that
descriptor address, and so the code always sets fi->stage2 and
fi->s1ptw to true. However, with FEAT_RME it is also possible for
the lookup of the page descriptor address to fail because of a
Granule Protection Check fault. These should not be reported as
stage 2, otherwise arm_deliver_fault() will incorrectly set
HPFAR_EL2. Similarly the s1ptw bit should only be set for stage 2
faults on stage 1 translation table walks, i.e. not for GPC faults.
Add a comment to the the other place where we might detect a
stage2-fault-on-stage-1-ptw, in arm_casq_ptw(), noting why we know in
that case that it must really be a stage 2 fault and not a GPC fault.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index bafeb876ad7..eb57ebd897b 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -600,8 +600,8 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
fi->type = ARMFault_GPCFOnWalk;
}
fi->s2addr = addr;
-fi->stage2 = true;
-fi->s1ptw = true;
+fi->stage2 = regime_is_stage2(s2_mmu_idx);
+fi->s1ptw = fi->stage2;
fi->s1ns = !is_secure;
return false;
}
@@ -719,6 +719,12 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t
old_val,
env->tlb_fi = NULL;
if (unlikely(flags & TLB_INVALID_MASK)) {
+/*
+ * We know this must be a stage 2 fault because the granule
+ * protection table does not separately track read and write
+ * permission, so all GPC faults are caught in S1_ptw_translate():
+ * we only get here for "readable but not writeable".
+ */
assert(fi->type != ARMFault_None);
fi->s2addr = ptw->out_virt;
fi->stage2 = true;
--
2.34.1
[PATCH 01/14] target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault
For an Unsupported Atomic Update fault where the stage 1 translation
table descriptor update can't be done because it's to an unsupported
memory type, this is a stage 1 abort (per the Arm ARM R_VSXXT). This
means we should not set fi->s1ptw, because this will cause the code
in the get_phys_addr_lpae() error-exit path to mark it as stage 2.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 8f94100c61f..bafeb876ad7 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -701,7 +701,6 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t
old_val,
if (unlikely(!host)) {
fi->type = ARMFault_UnsuppAtomicUpdate;
-fi->s1ptw = true;
return 0;
}
--
2.34.1
[PATCH 00/14] target/arm/ptw: Cleanups and a few bugfixes
Based-on: 20230710152130.3928330-1-peter.mayd...@linaro.org
("target/arm: Fix ptw bugs introduced by FEAT_RME changes")
While I was fixing the ptw bug in the series above, I noticed
that we had a somewhat confusing mix of ptw->in_space and
ptw->in_secure, where in theory the two are supposed to be
in sync and you can figure out the in_secure state from the
in_space. This patch series' principal aim is to clean that
up by removing the in_secure and out_secure fields in the
S1Translate struct.
The first three patches are fixes for (minor) bugs I noticed
while I was trying to do this refactoring because they're
in or around places that were using in_secure.
The next three are basically plumbing: passing ARMSecurityState
arguments instead of boolean is_secure arguments.
The next four patches then can get rid of uses of the
in_secure and out_secure fields and drop them entirely.
Finally, the last four patches are minor bug fixes for
various corner cases that I noticed while I was testing this.
I don't expect to land this series until we reopen for
8.2 development, but I might as well put it out on the
list for review, since I've written it.
thanks
-- PMM
Peter Maydell (14):
target/arm/ptw: Don't set fi->s1ptw for UnsuppAtomicUpdate fault
target/arm/ptw: Don't report GPC faults on stage 1 ptw as stage2
faults
target/arm/ptw: Set s1ns bit in fault info more consistently
target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and
get_phys_addr_disabled()
target/arm/ptw: Pass ARMSecurityState to regime_translation_disabled()
target/arm/ptw: Pass an ARMSecuritySpace to arm_hcr_el2_eff_secstate()
target/arm/ptw: Only fold in NSTable bit effects in Secure state
target/arm/ptw: Remove last uses of ptw->in_secure
target/arm/ptw: Remove S1Translate::in_secure
target/arm/ptw: Drop S1Translate::out_secure
target/arm/ptw: Set attributes correctly for MMU disabled data
accesses
target/arm/ptw: Check for block descriptors at invalid levels
target/arm/ptw: Report stage 2 fault level for stage 2 faults on stage
1 ptw
target/arm: Adjust PAR_EL1.SH for Device and Normal-NC memory types
target/arm/cpu.h| 2 +-
target/arm/helper.c | 22 -
target/arm/ptw.c| 190 +++-
3 files changed, 135 insertions(+), 79 deletions(-)
--
2.34.1
[PATCH 04/14] target/arm/ptw: Pass ptw into get_phys_addr_pmsa*() and get_phys_addr_disabled()
In commit 6d2654ffacea813916176 we created the S1Translate struct and
used it to plumb through various arguments that we were previously
passing one-at-a-time to get_phys_addr_v5(), get_phys_addr_v6(), and
get_phys_addr_lpae(). Extend that pattern to get_phys_addr_pmsav5(),
get_phys_addr_pmsav7(), get_phys_addr_pmsav8() and
get_phys_addr_disabled(), so that all the get_phys_addr_* functions
we call from get_phys_addr_nogpc() take the S1Translate struct rather
than the mmu_idx and is_secure bool.
(This refactoring is a prelude to having the called functions look
at ptw->is_space rather than using an is_secure boolean.)
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 57 ++--
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 67078ae3509..a873fbe0239 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -2045,15 +2045,19 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
return true;
}
-static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
- MMUAccessType access_type, ARMMMUIdx mmu_idx,
- bool is_secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav5(CPUARMState *env,
+ S1Translate *ptw,
+ uint32_t address,
+ MMUAccessType access_type,
+ GetPhysAddrResult *result,
ARMMMUFaultInfo *fi)
{
int n;
uint32_t mask;
uint32_t base;
+ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
bool is_user = regime_is_user(env, mmu_idx);
+bool is_secure = arm_space_is_secure(ptw->in_space);
if (regime_translation_disabled(env, mmu_idx, is_secure)) {
/* MPU disabled. */
@@ -2210,14 +2214,18 @@ static bool pmsav7_use_background_region(ARMCPU *cpu,
ARMMMUIdx mmu_idx,
return regime_sctlr(env, mmu_idx) & SCTLR_BR;
}
-static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
- MMUAccessType access_type, ARMMMUIdx mmu_idx,
- bool secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav7(CPUARMState *env,
+ S1Translate *ptw,
+ uint32_t address,
+ MMUAccessType access_type,
+ GetPhysAddrResult *result,
ARMMMUFaultInfo *fi)
{
ARMCPU *cpu = env_archcpu(env);
int n;
+ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
bool is_user = regime_is_user(env, mmu_idx);
+bool secure = arm_space_is_secure(ptw->in_space);
result->f.phys_addr = address;
result->f.lg_page_size = TARGET_PAGE_BITS;
@@ -2736,12 +2744,16 @@ void v8m_security_lookup(CPUARMState *env, uint32_t
address,
}
}
-static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
- MMUAccessType access_type, ARMMMUIdx mmu_idx,
- bool secure, GetPhysAddrResult *result,
+static bool get_phys_addr_pmsav8(CPUARMState *env,
+ S1Translate *ptw,
+ uint32_t address,
+ MMUAccessType access_type,
+ GetPhysAddrResult *result,
ARMMMUFaultInfo *fi)
{
V8M_SAttributes sattrs = {};
+ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
+bool secure = arm_space_is_secure(ptw->in_space);
bool ret;
if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
@@ -3045,12 +3057,15 @@ static ARMCacheAttrs combine_cacheattrs(uint64_t hcr,
* MMU disabled. S1 addresses within aa64 translation regimes are
* still checked for bounds -- see AArch64.S1DisabledOutput().
*/
-static bool get_phys_addr_disabled(CPUARMState *env, target_ulong address,
+static bool get_phys_addr_disabled(CPUARMState *env,
+ S1Translate *ptw,
+ target_ulong address,
MMUAccessType access_type,
- ARMMMUIdx mmu_idx, bool is_secure,
GetPhysAddrResult *result,
ARMMMUFaultInfo *fi)
{
+ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
+bool is_secure = arm_space_is_secure(ptw->in_space);
uint8_t memattr = 0x00;/* Device nGnRnE */
uint8_t shareability = 0; /* non-shareable */
int r_el;
@@ -3252,8 +3267,8 @@ static bool get_phys_addr_nogpc(CPUARMState *env,
S1Translate *ptw,
case ARMMMUIdx_Phys_Root:
case ARMMMUIdx_Phys_Realm:
/* Checking Phys early avoids special casing later vs regime_el. */
-return get_phys_addr_disabled(env, address, access_type, mmu_idx,
-
[PATCH 03/14] target/arm/ptw: Set s1ns bit in fault info more consistently
The s1ns bit in ARMMMUFaultInfo is documented as "true if
we faulted on a non-secure IPA while in secure state". Both the
places which look at this bit only do so after having confirmed
that this is a stage 2 fault and we're dealing with Secure EL2,
which leaves the ptw.c code free to set the bit to any random
value in the other cases.
Instead of taking advantage of that freedom, consistently
make the bit be set to false for the "not a stage 2 fault
for Secure EL2" cases. This removes some cases where we
were using an 'is_secure' boolean and leaving the reader
guessing about whether that was the right thing for Realm
and Root cases.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index eb57ebd897b..67078ae3509 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -514,6 +514,17 @@ static ARMSecuritySpace S2_security_space(ARMSecuritySpace
s1_space,
}
}
+static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx s2_mmu_idx)
+{
+/*
+ * For stage 2 faults in Secure EL22, S1NS indicates
+ * whether the faulting IPA is in the Secure or NonSecure
+ * IPA space. For all other kinds of fault, it is false.
+ */
+return space == ARMSS_Secure && regime_is_stage2(s2_mmu_idx)
+&& s2_mmu_idx == ARMMMUIdx_Stage2_S;
+}
+
/* Translate a S1 pagetable walk through S2 if needed. */
static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
hwaddr addr, ARMMMUFaultInfo *fi)
@@ -586,7 +597,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
fi->s2addr = addr;
fi->stage2 = true;
fi->s1ptw = true;
-fi->s1ns = !is_secure;
+fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx);
return false;
}
}
@@ -602,7 +613,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
fi->s2addr = addr;
fi->stage2 = regime_is_stage2(s2_mmu_idx);
fi->s1ptw = fi->stage2;
-fi->s1ns = !is_secure;
+fi->s1ns = fault_s1ns(ptw->in_space, s2_mmu_idx);
return false;
}
@@ -729,7 +740,7 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t
old_val,
fi->s2addr = ptw->out_virt;
fi->stage2 = true;
fi->s1ptw = true;
-fi->s1ns = !ptw->in_secure;
+fi->s1ns = fault_s1ns(ptw->in_space, ptw->in_ptw_idx);
return 0;
}
@@ -2030,7 +2041,7 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
fi->level = level;
/* Tag the error as S2 for failed S1 PTW at S2 or ordinary S2. */
fi->stage2 = fi->s1ptw || regime_is_stage2(mmu_idx);
-fi->s1ns = mmu_idx == ARMMMUIdx_Stage2;
+fi->s1ns = fault_s1ns(ptw->in_space, mmu_idx);
return true;
}
--
2.34.1
[PATCH 12/14] target/arm/ptw: Check for block descriptors at invalid levels
The architecture doesn't permit block descriptors at any arbitrary
level of the page table walk; it depends on the granule size which
levels are permitted. We implemented only a partial version of this
check which assumes that block descriptors are valid at all levels
except level 3, which meant that we wouldn't deliver the Translation
fault for all cases of this sort of guest page table error.
Implement the logic corresponding to the pseudocode
AArch64.DecodeDescriptorType() and AArch64.BlockDescSupported().
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index e4210abc148..ed46bb82a75 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -1549,6 +1549,25 @@ static int check_s2_mmu_setup(ARMCPU *cpu, bool is_aa64,
uint64_t tcr,
return INT_MIN;
}
+static bool lpae_block_desc_valid(ARMCPU *cpu, bool ds,
+ ARMGranuleSize gran, int level)
+{
+/*
+ * See pseudocode AArch46.BlockDescSupported(): block descriptors
+ * are not valid at all levels, depending on the page size.
+ */
+switch (gran) {
+case Gran4K:
+return (level == 0 && ds) || level == 1 || level == 2;
+case Gran16K:
+return (level == 1 && ds) || level == 2;
+case Gran64K:
+return (level == 1 && arm_pamax(cpu) == 52) || level == 2;
+default:
+g_assert_not_reached();
+}
+}
+
/**
* get_phys_addr_lpae: perform one stage of page table walk, LPAE format
*
@@ -1784,8 +1803,10 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
new_descriptor = descriptor;
restart_atomic_update:
-if (!(descriptor & 1) || (!(descriptor & 2) && (level == 3))) {
-/* Invalid, or the Reserved level 3 encoding */
+if (!(descriptor & 1) ||
+(!(descriptor & 2) &&
+ !lpae_block_desc_valid(cpu, param.ds, param.gran, level))) {
+/* Invalid, or a block descriptor at an invalid level */
goto do_translation_fault;
}
--
2.34.1
[PATCH 06/14] target/arm/ptw: Pass an ARMSecuritySpace to arm_hcr_el2_eff_secstate()
arm_hcr_el2_eff_secstate() takes a bool secure, which it uses to
determine whether EL2 is enabled in the current security state.
With the advent of FEAT_RME this is no longer sufficient, because
EL2 can be enabled for Secure state but not for Root, and both
of those will pass 'secure == true' in the callsites in ptw.c.
As it happens in all of our callsites in ptw.c we either avoid making
the call or else avoid using the returned value if we're doing a
translation for Root, so this is not a behaviour change even if the
experimental FEAT_RME is enabled. But it is less confusing in the
ptw.c code if we avoid the use of a bool secure that duplicates some
of the information in the ArmSecuritySpace argument.
Make arm_hcr_el2_eff_secstate() take an ARMSecuritySpace argument
instead.
Note that since arm_hcr_el2_eff() uses the return value from
arm_security_space_below_el3() for the 'space' argument, its
behaviour does not change even when at EL3 (Root security state) and
it continues to tell you what EL2 would be if you were in it.
Signed-off-by: Peter Maydell
---
target/arm/cpu.h| 2 +-
target/arm/helper.c | 7 ---
target/arm/ptw.c| 13 +
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 4d6c0f95d59..3743a9e2f8a 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2555,7 +2555,7 @@ static inline bool arm_is_el2_enabled(CPUARMState *env)
* "for all purposes other than a direct read or write access of HCR_EL2."
* Not included here is HCR_RW.
*/
-uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure);
+uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space);
uint64_t arm_hcr_el2_eff(CPUARMState *env);
uint64_t arm_hcrx_el2_eff(CPUARMState *env);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index d08c058e424..1e45fdb47c9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5731,11 +5731,12 @@ static void hcr_writelow(CPUARMState *env, const
ARMCPRegInfo *ri,
* Bits that are not included here:
* RW (read from SCR_EL3.RW as needed)
*/
-uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, bool secure)
+uint64_t arm_hcr_el2_eff_secstate(CPUARMState *env, ARMSecuritySpace space)
{
uint64_t ret = env->cp15.hcr_el2;
-if (!arm_is_el2_enabled_secstate(env, secure)) {
+if (space == ARMSS_Root ||
+!arm_is_el2_enabled_secstate(env, arm_space_is_secure(space))) {
/*
* "This register has no effect if EL2 is not enabled in the
* current Security state". This is ARMv8.4-SecEL2 speak for
@@ -5799,7 +5800,7 @@ uint64_t arm_hcr_el2_eff(CPUARMState *env)
if (arm_feature(env, ARM_FEATURE_M)) {
return 0;
}
-return arm_hcr_el2_eff_secstate(env, arm_is_secure_below_el3(env));
+return arm_hcr_el2_eff_secstate(env, arm_security_space_below_el3(env));
}
/*
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 63dd8e3cbe1..9e45160e1ba 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -209,9 +209,9 @@ static bool regime_translation_disabled(CPUARMState *env,
ARMMMUIdx mmu_idx,
ARMSecuritySpace space)
{
uint64_t hcr_el2;
-bool is_secure = arm_space_is_secure(space);
if (arm_feature(env, ARM_FEATURE_M)) {
+bool is_secure = arm_space_is_secure(space);
switch (env->v7m.mpu_ctrl[is_secure] &
(R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
case R_V7M_MPU_CTRL_ENABLE_MASK:
@@ -230,7 +230,7 @@ static bool regime_translation_disabled(CPUARMState *env,
ARMMMUIdx mmu_idx,
}
}
-hcr_el2 = arm_hcr_el2_eff_secstate(env, is_secure);
+hcr_el2 = arm_hcr_el2_eff_secstate(env, space);
switch (mmu_idx) {
case ARMMMUIdx_Stage2:
@@ -530,7 +530,6 @@ static bool fault_s1ns(ARMSecuritySpace space, ARMMMUIdx
s2_mmu_idx)
static bool S1_ptw_translate(CPUARMState *env, S1Translate *ptw,
hwaddr addr, ARMMMUFaultInfo *fi)
{
-bool is_secure = ptw->in_secure;
ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
ARMMMUIdx s2_mmu_idx = ptw->in_ptw_idx;
uint8_t pte_attrs;
@@ -587,7 +586,7 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
}
if (regime_is_stage2(s2_mmu_idx)) {
-uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
+uint64_t hcr = arm_hcr_el2_eff_secstate(env, ptw->in_space);
if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
/*
@@ -3066,7 +3065,6 @@ static bool get_phys_addr_disabled(CPUARMState *env,
ARMMMUFaultInfo *fi)
{
ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
-bool is_secure = arm_space_is_secure(ptw->in_space);
uint8_t memattr = 0x00;/* Device nGnRnE */
uint8_t shareability = 0; /* non-shareable */
int r_el;
@@ -3112,7 +3110,7 @@ static bool get_phys_addr_disabled(CPUARMState
[PATCH 09/14] target/arm/ptw: Remove S1Translate::in_secure
We no longer look at the in_secure field of the S1Translate struct
anyway, so we can remove it and all the code which sets it.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index bc834675fb2..77b8382ceff 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -51,13 +51,6 @@ typedef struct S1Translate {
*value being Stage2 vs Stage2_S distinguishes those.
*/
ARMSecuritySpace in_space;
-/*
- * in_secure: whether the translation regime is a Secure one.
- * This is always equal to arm_space_is_secure(in_space).
- * If a Secure ptw is "downgraded" to NonSecure by an NSTable bit,
- * this field is updated accordingly.
- */
-bool in_secure;
/*
* in_debug: is this a QEMU debug access (gdbstub, etc)? Debug
* accesses will not update the guest page table access flags
@@ -545,7 +538,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
S1Translate s2ptw = {
.in_mmu_idx = s2_mmu_idx,
.in_ptw_idx = ptw_idx_for_stage_2(env, s2_mmu_idx),
-.in_secure = arm_space_is_secure(s2_space),
.in_space = s2_space,
.in_debug = true,
};
@@ -1782,7 +1774,6 @@ static bool get_phys_addr_lpae(CPUARMState *env,
S1Translate *ptw,
QEMU_BUILD_BUG_ON(ARMMMUIdx_Phys_S + 1 != ARMMMUIdx_Phys_NS);
QEMU_BUILD_BUG_ON(ARMMMUIdx_Stage2_S + 1 != ARMMMUIdx_Stage2);
ptw->in_ptw_idx += 1;
-ptw->in_secure = false;
ptw->in_space = ARMSS_NonSecure;
}
@@ -3165,7 +3156,6 @@ static bool get_phys_addr_twostage(CPUARMState *env,
S1Translate *ptw,
ptw->in_s1_is_el0 = ptw->in_mmu_idx == ARMMMUIdx_Stage1_E0;
ptw->in_mmu_idx = ipa_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
-ptw->in_secure = ipa_secure;
ptw->in_space = ipa_space;
ptw->in_ptw_idx = ptw_idx_for_stage_2(env, ptw->in_mmu_idx);
@@ -3401,7 +3391,6 @@ bool get_phys_addr_with_secure(CPUARMState *env,
target_ulong address,
{
S1Translate ptw = {
.in_mmu_idx = mmu_idx,
-.in_secure = is_secure,
.in_space = arm_secure_to_space(is_secure),
};
return get_phys_addr_gpc(env, , address, access_type, result, fi);
@@ -3473,7 +3462,6 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
}
ptw.in_space = ss;
-ptw.in_secure = arm_space_is_secure(ss);
return get_phys_addr_gpc(env, , address, access_type, result, fi);
}
@@ -3487,7 +3475,6 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs,
vaddr addr,
S1Translate ptw = {
.in_mmu_idx = mmu_idx,
.in_space = ss,
-.in_secure = arm_space_is_secure(ss),
.in_debug = true,
};
GetPhysAddrResult res = {};
--
2.34.1
[PATCH 11/14] target/arm/ptw: Set attributes correctly for MMU disabled data accesses
When the MMU is disabled, data accesses should be Device nGnRnE,
Outer Shareable, Untagged. We handle the other cases from
AArch64.S1DisabledOutput() correctly but missed this one.
Device nGnRnE is memattr == 0, so the only part we were missing
was that shareability should be set to 2 for both insn fetches
and data accesses.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2be6bf302b0..e4210abc148 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3106,11 +3106,13 @@ static bool get_phys_addr_disabled(CPUARMState *env,
}
}
}
-if (memattr == 0 && access_type == MMU_INST_FETCH) {
-if (regime_sctlr(env, mmu_idx) & SCTLR_I) {
-memattr = 0xee; /* Normal, WT, RA, NT */
-} else {
-memattr = 0x44; /* Normal, NC, No */
+if (memattr == 0) {
+if (access_type == MMU_INST_FETCH) {
+if (regime_sctlr(env, mmu_idx) & SCTLR_I) {
+memattr = 0xee; /* Normal, WT, RA, NT */
+} else {
+memattr = 0x44; /* Normal, NC, No */
+}
}
shareability = 2; /* outer shareable */
}
--
2.34.1
[PATCH 10/14] target/arm/ptw: Drop S1Translate::out_secure
We only use S1Translate::out_secure in two places, where we are
setting up MemTxAttrs for a page table load. We can use
arm_space_is_secure(ptw->out_space) instead, which guarantees
that we're setting the MemTxAttrs secure and space fields
consistently, and allows us to drop the out_secure field in
S1Translate entirely.
Signed-off-by: Peter Maydell
---
target/arm/ptw.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 77b8382ceff..2be6bf302b0 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -63,7 +63,6 @@ typedef struct S1Translate {
* Stage 2 is indicated by in_mmu_idx set to ARMMMUIdx_Stage2{,_S}.
*/
bool in_s1_is_el0;
-bool out_secure;
bool out_rw;
bool out_be;
ARMSecuritySpace out_space;
@@ -551,7 +550,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
pte_attrs = s2.cacheattrs.attrs;
ptw->out_host = NULL;
ptw->out_rw = false;
-ptw->out_secure = s2.f.attrs.secure;
ptw->out_space = s2.f.attrs.space;
} else {
#ifdef CONFIG_TCG
@@ -570,7 +568,6 @@ static bool S1_ptw_translate(CPUARMState *env, S1Translate
*ptw,
ptw->out_phys = full->phys_addr | (addr & ~TARGET_PAGE_MASK);
ptw->out_rw = full->prot & PAGE_WRITE;
pte_attrs = full->pte_attrs;
-ptw->out_secure = full->attrs.secure;
ptw->out_space = full->attrs.space;
#else
g_assert_not_reached();
@@ -628,8 +625,8 @@ static uint32_t arm_ldl_ptw(CPUARMState *env, S1Translate
*ptw,
} else {
/* Page tables are in MMIO. */
MemTxAttrs attrs = {
-.secure = ptw->out_secure,
.space = ptw->out_space,
+.secure = arm_space_is_secure(ptw->out_space),
};
AddressSpace *as = arm_addressspace(cs, attrs);
MemTxResult result = MEMTX_OK;
@@ -674,8 +671,8 @@ static uint64_t arm_ldq_ptw(CPUARMState *env, S1Translate
*ptw,
} else {
/* Page tables are in MMIO. */
MemTxAttrs attrs = {
-.secure = ptw->out_secure,
.space = ptw->out_space,
+.secure = arm_space_is_secure(ptw->out_space),
};
AddressSpace *as = arm_addressspace(cs, attrs);
MemTxResult result = MEMTX_OK;
--
2.34.1
[PULL v1 1/1] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
The ppi command line option for the TIS device on sysbus never worked
and caused an immediate segfault. Remove support for it since it also
needs support in the firmware and needs testing inside the VM.
Reproducer with the ppi=on option passed:
qemu-system-aarch64 \
-machine virt,gic-version=3 \
-m 4G \
-nographic -no-acpi \
-chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm \
-device tpm-tis-device,tpmdev=tpm0,ppi=on
[...]
Segmentation fault (core dumped)
Signed-off-by: Stefan Berger
Reviewed-by: Eric Auger
Message-id: 20230713171955.149236-1-stef...@linux.ibm.com
---
hw/tpm/tpm_tis_sysbus.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index 45e63efd63..6724b3d4f6 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -93,7 +93,6 @@ static void tpm_tis_sysbus_reset(DeviceState *dev)
static Property tpm_tis_sysbus_properties[] = {
DEFINE_PROP_UINT32("irq", TPMStateSysBus, state.irq_num, TPM_TIS_IRQ),
DEFINE_PROP_TPMBE("tpmdev", TPMStateSysBus, state.be_driver),
-DEFINE_PROP_BOOL("ppi", TPMStateSysBus, state.ppi_enabled, false),
DEFINE_PROP_END_OF_LIST(),
};
--
2.41.0
[PULL v1 0/1] Merge tpm 2023/07/14 v1
Hello! This PR removes the 'ppi' boolean property from the tpm tis sysbus device. It could never be activated since it was leading to a segfault immediatley. Stefan The following changes since commit 3dd9e54703e6ae4f9ab3767f5cecc99edf08: Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-07-12 20:46:10 +0100) are available in the Git repository at: https://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2023-07-14-1 for you to fetch changes up to 4c46fe2ed492f35f411632c8b5a8442f322bc3f0: hw/tpm: TIS on sysbus: Remove unsupport ppi command line option (2023-07-14 11:31:54 -0400) Stefan Berger (1): hw/tpm: TIS on sysbus: Remove unsupport ppi command line option hw/tpm/tpm_tis_sysbus.c | 1 - 1 file changed, 1 deletion(-) -- 2.41.0
[PATCH 2/3] contrib/vhost-user-gpu: add support for sending dmabuf modifiers
virglrenderer recently added virgl_renderer_resource_get_info_ext as a
new api, which gets resource information, including dmabuf modifiers.
We have to support dmabuf modifiers since the driver may choose to
allocate buffers with these modifiers for efficiency, and importing
buffers without modifiers information may result in completely broken
rendering.
Signed-off-by: Erico Nunes
---
contrib/vhost-user-gpu/vhost-user-gpu.c | 5 ++-
contrib/vhost-user-gpu/virgl.c | 51 +++--
contrib/vhost-user-gpu/vugpu.h | 9 +
3 files changed, 61 insertions(+), 4 deletions(-)
diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 2e7815a7a3..aa304475a0 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1071,6 +1071,7 @@ static gboolean
protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
{
const uint64_t protocol_edid = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID);
+const uint64_t protocol_dmabuf2 = (1 << VHOST_USER_GPU_PROTOCOL_F_DMABUF2);
VuGpu *g = user_data;
uint64_t protocol_features;
VhostUserGpuMsg msg = {
@@ -1082,7 +1083,7 @@ protocol_features_cb(gint fd, GIOCondition condition,
gpointer user_data)
return G_SOURCE_CONTINUE;
}
-protocol_features &= protocol_edid;
+protocol_features &= (protocol_edid | protocol_dmabuf2);
msg = (VhostUserGpuMsg) {
.request = VHOST_USER_GPU_SET_PROTOCOL_FEATURES,
@@ -1100,6 +1101,8 @@ protocol_features_cb(gint fd, GIOCondition condition,
gpointer user_data)
exit(EXIT_FAILURE);
}
+g->use_modifiers = !!(protocol_features & protocol_dmabuf2);
+
return G_SOURCE_REMOVE;
}
diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 211aa110a9..1da6cc1588 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -318,6 +318,37 @@ virgl_resource_detach_backing(VuGpu *g,
vg_cleanup_mapping_iov(g, res_iovs, num_iovs);
}
+static int
+virgl_get_resource_info_modifiers(uint32_t resource_id,
+ struct virgl_renderer_resource_info *info,
+ uint64_t *modifiers)
+{
+int ret;
+#ifdef VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION
+struct virgl_renderer_resource_info_ext info_ext;
+ret = virgl_renderer_resource_get_info_ext(resource_id, _ext);
+if (ret < 0) {
+return ret;
+}
+
+*info = info_ext.base;
+*modifiers = info_ext.modifiers;
+#else
+ret = virgl_renderer_resource_get_info(resource_id, info);
+if (ret < 0) {
+return ret;
+}
+
+/*
+ * Before virgl_renderer_resource_get_info_ext,
+ * getting the modifiers was not possible.
+ */
+*modifiers = 0;
+#endif
+
+return 0;
+}
+
static void
virgl_cmd_set_scanout(VuGpu *g,
struct virtio_gpu_ctrl_command *cmd)
@@ -338,7 +369,9 @@ virgl_cmd_set_scanout(VuGpu *g,
memset(, 0, sizeof(info));
if (ss.resource_id && ss.r.width && ss.r.height) {
-ret = virgl_renderer_resource_get_info(ss.resource_id, );
+uint64_t modifiers = 0;
+ret = virgl_get_resource_info_modifiers(ss.resource_id, ,
+);
if (ret == -1) {
g_critical("%s: illegal resource specified %d\n",
__func__, ss.resource_id);
@@ -354,8 +387,6 @@ virgl_cmd_set_scanout(VuGpu *g,
}
assert(fd >= 0);
VhostUserGpuMsg msg = {
-.request = VHOST_USER_GPU_DMABUF_SCANOUT,
-.size = sizeof(VhostUserGpuDMABUFScanout),
.payload.dmabuf_scanout.scanout_id = ss.scanout_id,
.payload.dmabuf_scanout.x = ss.r.x,
.payload.dmabuf_scanout.y = ss.r.y,
@@ -367,6 +398,20 @@ virgl_cmd_set_scanout(VuGpu *g,
.payload.dmabuf_scanout.fd_flags = info.flags,
.payload.dmabuf_scanout.fd_drm_fourcc = info.drm_fourcc
};
+
+if (g->use_modifiers) {
+/*
+ * The mesage uses all the fields set in dmabuf_scanout plus
+ * modifiers which is appended after VhostUserGpuDMABUFScanout.
+ */
+msg.request = VHOST_USER_GPU_DMABUF_SCANOUT2;
+msg.size = sizeof(VhostUserGpuDMABUFScanout2);
+msg.payload.dmabuf_scanout2.modifier = modifiers;
+} else {
+msg.request = VHOST_USER_GPU_DMABUF_SCANOUT;
+msg.size = sizeof(VhostUserGpuDMABUFScanout);
+}
+
vg_send_msg(g, , fd);
close(fd);
} else {
diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
index f0f2069c47..509b679f03 100644
--- a/contrib/vhost-user-gpu/vugpu.h
+++ b/contrib/vhost-user-gpu/vugpu.h
@@ -37,6 +37,7 @@ typedef enum VhostUserGpuRequest {
VHOST_USER_GPU_DMABUF_SCANOUT,
[PATCH 1/3] docs: vhost-user-gpu: add protocol changes for dmabuf modifiers
VHOST_USER_GPU_DMABUF_SCANOUT2 is defined as a message with all the contents of VHOST_USER_GPU_DMABUF_SCANOUT plus the dmabuf modifiers which were ommitted. The VHOST_USER_GPU_PROTOCOL_F_DMABUF2 protocol feature is defined as a way to check whether this new message is supported or not. Signed-off-by: Erico Nunes --- docs/interop/vhost-user-gpu.rst | 26 +- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/interop/vhost-user-gpu.rst b/docs/interop/vhost-user-gpu.rst index b78806892d..3035822d05 100644 --- a/docs/interop/vhost-user-gpu.rst +++ b/docs/interop/vhost-user-gpu.rst @@ -134,6 +134,19 @@ VhostUserGpuEdidRequest :scanout-id: ``u32``, the scanout to get edid from +VhostUserGpuDMABUFScanout2 +^^ + +++--+ +| dmabuf_scanout | modifier | +++--+ + +:dmabuf_scanout: ``VhostUserGpuDMABUFScanout``, filled as described in the + VhostUserGpuDMABUFScanout structure. + +:modifier: ``u64``, the DMABUF modifiers + + C structure --- @@ -163,7 +176,8 @@ Protocol features .. code:: c - #define VHOST_USER_GPU_PROTOCOL_F_EDID 0 + #define VHOST_USER_GPU_PROTOCOL_F_EDID0 + #define VHOST_USER_GPU_PROTOCOL_F_DMABUF2 1 New messages and communication changes are negotiated thanks to the ``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` and @@ -263,3 +277,13 @@ Message types Retrieve the EDID data for a given scanout. This message requires the ``VHOST_USER_GPU_PROTOCOL_F_EDID`` protocol feature to be supported. + +``VHOST_USER_GPU_DMABUF_SCANOUT2`` + :id: 12 + :request payload: ``VhostUserGpuDMABUFScanout2`` + :reply payload: N/A + + Same as VHOST_USER_GPU_DMABUF_SCANOUT, but also sends the dmabuf modifiers + appended to the message, which were not provided in the other message. + This message requires the ``VHOST_USER_GPU_PROTOCOL_F_DMABUF2`` protocol + feature to be supported. -- 2.40.1
[PATCH 3/3] vhost-user-gpu: support dmabuf modifiers
When the backend sends VHOST_USER_GPU_DMABUF_SCANOUT2, handle it
by getting the modifiers information which is now available.
Signed-off-by: Erico Nunes
---
hw/display/vhost-user-gpu.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index e8ee03094e..1150521d9d 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -32,6 +32,7 @@ typedef enum VhostUserGpuRequest {
VHOST_USER_GPU_DMABUF_SCANOUT,
VHOST_USER_GPU_DMABUF_UPDATE,
VHOST_USER_GPU_GET_EDID,
+VHOST_USER_GPU_DMABUF_SCANOUT2,
} VhostUserGpuRequest;
typedef struct VhostUserGpuDisplayInfoReply {
@@ -79,6 +80,11 @@ typedef struct VhostUserGpuDMABUFScanout {
int fd_drm_fourcc;
} QEMU_PACKED VhostUserGpuDMABUFScanout;
+typedef struct VhostUserGpuDMABUFScanout2 {
+struct VhostUserGpuDMABUFScanout dmabuf_scanout;
+uint64_t modifier;
+} QEMU_PACKED VhostUserGpuDMABUFScanout2;
+
typedef struct VhostUserGpuEdidRequest {
uint32_t scanout_id;
} QEMU_PACKED VhostUserGpuEdidRequest;
@@ -93,6 +99,7 @@ typedef struct VhostUserGpuMsg {
VhostUserGpuScanout scanout;
VhostUserGpuUpdate update;
VhostUserGpuDMABUFScanout dmabuf_scanout;
+VhostUserGpuDMABUFScanout2 dmabuf_scanout2;
VhostUserGpuEdidRequest edid_req;
struct virtio_gpu_resp_edid resp_edid;
struct virtio_gpu_resp_display_info display_info;
@@ -107,6 +114,7 @@ static VhostUserGpuMsg m __attribute__ ((unused));
#define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
#define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+#define VHOST_USER_GPU_PROTOCOL_F_DMABUF2 1
static void vhost_user_gpu_update_blocked(VhostUserGPU *g, bool blocked);
@@ -171,7 +179,8 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
.flags = VHOST_USER_GPU_MSG_FLAG_REPLY,
.size = sizeof(uint64_t),
.payload = {
-.u64 = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID)
+.u64 = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID) |
+ (1 << VHOST_USER_GPU_PROTOCOL_F_DMABUF2)
}
};
@@ -236,6 +245,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
break;
}
+case VHOST_USER_GPU_DMABUF_SCANOUT2:
case VHOST_USER_GPU_DMABUF_SCANOUT: {
VhostUserGpuDMABUFScanout *m = >payload.dmabuf_scanout;
int fd = qemu_chr_fe_get_msgfd(>vhost_chr);
@@ -269,6 +279,11 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
VhostUserGpuMsg *msg)
.fourcc = m->fd_drm_fourcc,
.y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
};
+if (msg->request == VHOST_USER_GPU_DMABUF_SCANOUT2) {
+VhostUserGpuDMABUFScanout2 *m2 = >payload.dmabuf_scanout2;
+dmabuf->modifier = m2->modifier;
+}
+
dpy_gl_scanout_dmabuf(con, dmabuf);
break;
}
--
2.40.1
[PATCH 0/3] vhost-user-gpu: support dmabuf modifiers
virglrenderer recently added virgl_renderer_resource_get_info_ext as a new api, which gets resource information, including dmabuf modifiers. https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024 We have to support dmabuf modifiers since the driver may choose to allocate buffers with these modifiers to improve performance, and importing buffers without modifiers information may result in completely broken rendering. Currently trying to use vhost-user-gpu for rendering backend and using the qemu dbus ui as a ui backend results in a broken framebuffer with Intel GPUs as the buffer is allocated with a modifier. With this patchset, that is fixed. It is tricky to support since it requires to keep compatibility at the same time with: (1) build against older virglrenderer which do not provide virgl_renderer_resource_get_info_ext; (2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT message. I tried to reach a compromise here by not defining a completely new message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels like a bit of a hack, so I appreciate feedback if there is a better way (or naming) to handle it. Erico Nunes (3): docs: vhost-user-gpu: add protocol changes for dmabuf modifiers contrib/vhost-user-gpu: add support for sending dmabuf modifiers vhost-user-gpu: support dmabuf modifiers contrib/vhost-user-gpu/vhost-user-gpu.c | 5 ++- contrib/vhost-user-gpu/virgl.c | 51 +++-- contrib/vhost-user-gpu/vugpu.h | 9 + docs/interop/vhost-user-gpu.rst | 26 - hw/display/vhost-user-gpu.c | 17 - 5 files changed, 102 insertions(+), 6 deletions(-) -- 2.40.1
Re: [PATCH 1/1] qemu-nbd: fix regression with qemu-nbd --fork run over ssh
On Thu, Jul 06, 2023 at 09:15:45PM +0200, Denis V. Lunev wrote:
> Commit e6df58a5578fee7a50bbf36f4a50a2781cff855d
> Author: Hanna Reitz
> Date: Wed May 8 23:18:18 2019 +0200
> qemu-nbd: Do not close stderr
> has introduced an interesting regression. Original behavior of
> ssh somehost qemu-nbd /home/den/tmp/file -f raw --fork
> was the following:
> * qemu-nbd was started as a daemon
> * the command execution is done and ssh exited with success
>
> The patch has changed this behavior and 'ssh' command now hangs forever.
>
> According to the normal specification of the daemon() call, we should
> endup with STDERR pointing to /dev/null. That should be done at the
> very end of the successful startup sequence when the pipe to the
> bootstrap process (used for diagnostics) is no longer needed.
>
> This could be achived in the same way as done for 'qemu-nbd -c' case.
That was commit 0eaf453e, also fixing up e6df58a5.
> STDOUT copying to STDERR does the trick.
It took me a while to see how this worked (the double-negative of
passing 0 to the 'noclose' parameter of daemon() doesn't make it easy
to track what is supposed to happen), but I agree that our goal is
that after daemon()izing, we temporarily set stderr to the inherited
pipe for passing back any startup error messages, then usually want to
restore it to /dev/null for the remainder of the process.
>
> This also leads to proper 'ssh' connection closing which fixes my
> original problem.
>
> Signed-off-by: Denis V. Lunev
> CC: Eric Blake
> CC: Vladimir Sementsov-Ogievskiy
> CC: Hanna Reitz
> ---
> qemu-nbd.c | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
I indeed see how this fixes a regression under 'fork_process'.
However, the code that calls qemu_daemon() is also reachable under the
condition 'device && !verbose'. Does it make sense for either:
qemu-nbd -v -c /dev/nbd0
qemu-nbd -f -v -c /dev/nbd0
as a way to connect to the kernel device, but explicitly ask to remain
verbose, as a way to debug issues in connecting to the kernel via
stderr?
Going back to the emails at the time of Hanna's original commit...
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01872.html
I don't see any consideration about that case; capturing the original
stderr was done to fix what looked like an easy bug fix to a botched
implementation of old_stderr in commit ffb31e1d without considering
the ramifications.
But seeing as how pre-existing code DID want at least 'qemu-nbd -v -c
/dev/nbd0' to log indefinitely, I think we need to squash in:
diff --git i/qemu-nbd.c w/qemu-nbd.c
index 4276163564b..4d037798b9b 100644
--- i/qemu-nbd.c
+++ w/qemu-nbd.c
@@ -313,7 +313,7 @@ static void *nbd_client_thread(void *arg)
/* update partition table */
pthread_create(_parts_thread, NULL, show_parts, device);
-if (verbose) {
+if (verbose && !fork_process) {
fprintf(stderr, "NBD device %s is now connected to %s\n",
device, srcpath);
} else {
With that tweak, I'm fine with adding:
Reviewed-by: Eric Blake
Do you agree with my tweak? If so, I can queue this through my NBD
tree without needing to see a v2 post. However...
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 4276163564..e9e118dfdb 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -575,7 +575,6 @@ int main(int argc, char **argv)
> bool writethrough = false; /* Client will flush as needed. */
> bool fork_process = false;
> bool list = false;
> -int old_stderr = -1;
> unsigned socket_activation;
> const char *pid_file_name = NULL;
> const char *selinux_label = NULL;
> @@ -930,11 +929,6 @@ int main(int argc, char **argv)
> } else if (pid == 0) {
> close(stderr_fd[0]);
>
> -/* Remember parent's stderr if we will be restoring it. */
> -if (fork_process) {
> -old_stderr = dup(STDERR_FILENO);
> -}
> -
> ret = qemu_daemon(1, 0);
>
> /* Temporarily redirect stderr to the parent's pipe... */
> @@ -1152,8 +1146,7 @@ int main(int argc, char **argv)
Pre-existing, but your patch made me notice nearby (minor corner-case)
bugs:
ret = qemu_daemon(1, 0);
/* Temporarily redirect stderr to the parent's pipe... */
dup2(stderr_fd[1], STDERR_FILENO);
We aren't checking for dup2() failure. But even if it succeeds (which
we expect), it could have modified the contents of errno compared to
what they were after qemu_daemon()...
if (ret < 0) {
error_report("Failed to daemonize: %s", strerror(errno));
...so this could be reporting garbage. I wouldn't mind additional
patches to make the code more robust against unlikely failure
scenarios.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: QEMU assert (was: [xen-unstable test] 181558: regressions - FAIL)
On Tue, Jul 04, 2023 at 11:56:54AM +0200, Roger Pau Monné wrote: > On Tue, Jul 04, 2023 at 10:37:38AM +0100, Anthony PERARD wrote: > > On Wed, Jun 28, 2023 at 02:31:39PM +0200, Roger Pau Monné wrote: > > > On Fri, Jun 23, 2023 at 03:04:21PM +, osstest service owner wrote: > > > > flight 181558 xen-unstable real [real] > > > > http://logs.test-lab.xenproject.org/osstest/logs/181558/ > > > > > > > > Regressions :-( > > > > > > > > Tests which did not succeed and are blocking, > > > > including tests which could not be run: > > > > test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. > > > > vs. 181545 > > > > > > The test failing here is hitting the assert in qemu_cond_signal() as > > > called by worker_thread(): > > > > > > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > > > #1 0x7740b535 in __GI_abort () at abort.c:79 > > > #2 0x7740b40f in __assert_fail_base (fmt=0x7756cef0 > > > "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5614abcb > > > "cond->initialized", > > > file=0x5614ab88 > > > "../qemu-xen-dir-remote/util/qemu-thread-posix.c", line=198, > > > function=) at assert.c:92 > > > #3 0x774191a2 in __GI___assert_fail (assertion=0x5614abcb > > > "cond->initialized", file=0x5614ab88 > > > "../qemu-xen-dir-remote/util/qemu-thread-posix.c", line=198, > > > function=0x5614ad80 <__PRETTY_FUNCTION__.17104> > > > "qemu_cond_signal") at assert.c:101 > > > #4 0x55f1c8d2 in qemu_cond_signal (cond=0x7fffb800db30) at > > > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:198 > > > #5 0x55f36973 in worker_thread (opaque=0x7fffb800dab0) at > > > ../qemu-xen-dir-remote/util/thread-pool.c:129 > > > #6 0x55f1d1d2 in qemu_thread_start (args=0x7fffb8000b20) at > > > ../qemu-xen-dir-remote/util/qemu-thread-posix.c:505 > > > #7 0x775b0fa3 in start_thread (arg=) at > > > pthread_create.c:486 > > > #8 0x774e206f in clone () at > > > ../sysdeps/unix/sysv/linux/x86_64/clone.S:95 > > > > > > I've been trying to figure out how it can get in such state, but so > > > far I had no luck. I'm not a QEMU expert, so it's probably better if > > > someone else could handle this. > > > > > > In the failures I've seen, and the reproduction I have, the assert > > > triggers in the QEMU dom0 instance responsible for locally-attaching > > > the disk to dom0 in order to run pygrub. > > > > > > This is also with QEMU 7.2, as testing with upstream QEMU is blocked > > > ATM, so there's a chance it has already been fixed upstream. > > > > > > Thanks, Roger. > > > > So, I've run a test with the latest QEMU and I can still reproduce the > > issue. The test also fails with QEMU 7.1.0. > > > > But, QEMU 7.0 seems to pass the test, even with a start-stop loop of 200 > > iteration. So I'll try to find out if something change in that range. > > Or try to find out why would the thread pool be not initialised > > properly. > > Thanks for looking into this. > > There are a set of changes from Paolo Bonzini: > > 232e9255478f thread-pool: remove stopping variable > 900fa208f506 thread-pool: replace semaphore with condition variable > 3c7b72ddca9c thread-pool: optimize scheduling of completion bottom half > > That landed in 7.1 that seem like possible candidates. I think I've figured out the issue. I've sent a patch: https://lore.kernel.org/qemu-devel/20230714152720.5077-1-anthony.per...@citrix.com/ I did run osstest with this patch, and 200 iteration of stop/start, no more issue of qemu for dom0 disapearing. The issue I've found is osstest not able to ssh to the guest, which seems to be started. And qemu for dom0 is still running. While the report exist: http://logs.test-lab.xenproject.org/osstest/logs/181785/ Cheers, -- Anthony PERARD
[PATCH] thread-pool: signal "request_cond" while locked
From: Anthony PERARD
thread_pool_free() might have been called on the `pool`, which would
be a reason for worker_thread() to quit. In this case,
`pool->request_cond` is been destroyed.
If worker_thread() didn't managed to signal `request_cond` before it
been destroyed by thread_pool_free(), we got:
util/qemu-thread-posix.c:198: qemu_cond_signal: Assertion
`cond->initialized' failed.
One backtrace:
__GI___assert_fail (assertion=0x5614abcb "cond->initialized",
file=0x5614ab88 "util/qemu-thread-posix.c", line=198,
function=0x5614ad80 <__PRETTY_FUNCTION__.17104> "qemu_cond_signal")
at assert.c:101
qemu_cond_signal (cond=0x7fffb800db30) at util/qemu-thread-posix.c:198
worker_thread (opaque=0x7fffb800dab0) at util/thread-pool.c:129
qemu_thread_start (args=0x7fffb8000b20) at util/qemu-thread-posix.c:505
start_thread (arg=) at pthread_create.c:486
Reported here:
https://lore.kernel.org/all/ZJwoK50FcnTSfFZ8@MacBook-Air-de-Roger.local/T/#u
To avoid issue, keep lock while sending a signal to `request_cond`.
Fixes: 900fa208f506 ("thread-pool: replace semaphore with condition variable")
Signed-off-by: Anthony PERARD
---
There's maybe an issue in thread_pool_submit_aio() as well with
signalling `request_cond`, but maybe it's much less likely to be an
issue?
---
util/thread-pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/util/thread-pool.c b/util/thread-pool.c
index 0d97888df0..e3d8292d14 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -120,13 +120,13 @@ static void *worker_thread(void *opaque)
pool->cur_threads--;
qemu_cond_signal(>worker_stopped);
-qemu_mutex_unlock(>lock);
/*
* Wake up another thread, in case we got a wakeup but decided
* to exit due to pool->cur_threads > pool->max_threads.
*/
qemu_cond_signal(>request_cond);
+qemu_mutex_unlock(>lock);
return NULL;
}
--
Anthony PERARD
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 10:05, Stefan Berger wrote: On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though. After applying the whole series and trying to resume state taken with current git master I cannot restore it but it leads to this error here. I would just leave it completely untouched in 4/11. 2023-07-14T14:46:34.547550Z qemu-system-x86_64: Unknown ramblock "tpm-crb-cmd", cannot accept migration 2023-07-14T14:46:34.547799Z qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram' 2023-07-14T14:46:34.547835Z qemu-system-x86_64: load of migration failed: Invalid argument Stefan
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
On 7/14/23 09:51, Eric Auger wrote: Hi Stefan, On 7/14/23 13:51, Stefan Berger wrote: On 7/14/23 02:07, Joelle van Dyne wrote: On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger wrote: The ppi command line option for the TIS device on sysbus never worked and caused an immediate segfault. Remove support for it since it also needs support in the firmware and needs testing inside the VM. Reproducer with the ppi=on option passed: qemu-system-aarch64 \ -machine virt,gic-version=3 \ -m 4G \ -nographic -no-acpi \ -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ -tpmdev emulator,id=tpm0,chardev=chrtpm \ -device tpm-tis-device,tpmdev=tpm0,ppi=on [...] Segmentation fault (core dumped) Signed-off-by: Stefan Berger Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version introduces a new field in the same position which will cause an issue when restoring from an older version? Hm, you got a point there. We will have to error-out in case someone sets ppi=on instead since the expectation that PPI would work is simply not there. v2 coming soon. as Joelle pointed it out ppi_enabled is not part of vmstate_tpm_tis_sysbus fields. And since it has never worked I suspect we cannot have any existing VM enabling it. So I don't get the issue with this 1st version? You are right. I repeated my test with restoring state of a VM taken before the removal of this field and it restored it. So that other patch is good and I am withdrawing this patch here. Stefan Thanks Eric Stefan
Re: [PATCH v3] hw/mips: Improve the default USB settings in the loongson3-virt machine
On 14/7/23 12:49, Thomas Huth wrote: It's possible to compile QEMU without the USB devices (e.g. when using "--without-default-devices" as option for the "configure" script). To be still able to run the loongson3-virt machine in default mode with such a QEMU binary, we have to check here for the availability of the OHCI controller first before instantiating the USB devices. Signed-off-by: Thomas Huth --- v3: Back to runtime detection, but more simple this time compared to v1 (checking for OHCI should be enough, since this implies CONFIG_USB which is the switch that usb-kbd and usb-tablet are depending on) hw/mips/loongson3_virt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé And queued to mips-fixes, thanks!
Re: [PATCH] target/i386: Check CR0.TS before enter_mmx
Queued, thanks. Paolo
Re: [PATCH v2 10/11] tpm_crb_sysbus: introduce TPM CRB SysBus device
On 7/14/23 03:09, Joelle van Dyne wrote:
This SysBus variant of the CRB interface supports dynamically locating
the MMIO interface so that Virt machines can use it. This interface
is currently the only one supported by QEMU that works on Windows 11
ARM64. We largely follow the TPM TIS SysBus device as a template.
To try out this device with Windows 11 before OVMF is updated, you
will need to modify `sysbud-fdt.c` and change the added line from:
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, no_fdt_node),
```
to
```c
TYPE_BINDING(TYPE_TPM_CRB_SYSBUS, add_tpm_tis_fdt_node),
```
This change was not included because it can confuse Linux (although
from testing, it seems like Linux is able to properly ignore the
device from the TPM TIS driver and recognize it from the ACPI device
in the TPM CRB driver). A proper fix would require OVMF to recognize
the ACPI device and not depend on the FDT node for recognizing TPM.
The command line to try out this device with SWTPM is:
```
$ qemu-system-aarch64 \
-chardev socket,id=chrtpm0,path=tpm.sock \
-tpmdev emulator,id=tpm0,chardev=chrtpm0 \
-device tpm-crb-device,tpmdev=tpm0
```
along with SWTPM:
```
$ swtpm \
--ctrl type=unixio,path=tpm.sock,terminate \
--tpmstate backend-uri=file://tpm.data \
--tpm2
```
Signed-off-by: Joelle van Dyne
---
docs/specs/tpm.rst | 1 +
include/hw/acpi/aml-build.h | 1 +
include/sysemu/tpm.h| 3 +
hw/acpi/aml-build.c | 7 +-
hw/arm/virt.c | 1 +
hw/core/sysbus-fdt.c| 1 +
hw/loongarch/virt.c | 1 +
hw/riscv/virt.c | 1 +
hw/tpm/tpm_crb_sysbus.c | 170
hw/arm/Kconfig | 1 +
hw/riscv/Kconfig| 1 +
hw/tpm/Kconfig | 5 ++
hw/tpm/meson.build | 2 +
13 files changed, 194 insertions(+), 1 deletion(-)
create mode 100644 hw/tpm/tpm_crb_sysbus.c
diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 2bc29c9804..95aeb49220 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -46,6 +46,7 @@ operating system.
QEMU files related to TPM CRB interface:
- ``hw/tpm/tpm_crb.c``
- ``hw/tpm/tpm_crb_common.c``
+ - ``hw/tpm/tpm_crb_sysbus.c``
If you added the command line to use for Windows guests to this document
I think this would be helpful. And also mention that this must be used for
Windows
since the other ones don't work.
SPAPR interface
---
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d1fb08514b..9660e16148 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -3,6 +3,7 @@
#include "hw/acpi/acpi-defs.h"
#include "hw/acpi/bios-linker-loader.h"
+#include "exec/hwaddr.h"
#define ACPI_BUILD_APPNAME6 "BOCHS "
#define ACPI_BUILD_APPNAME8 "BXPC"
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 66e3b45f30..f79c8f3575 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -47,6 +47,7 @@ struct TPMIfClass {
#define TYPE_TPM_TIS_ISA"tpm-tis"
#define TYPE_TPM_TIS_SYSBUS "tpm-tis-device"
#define TYPE_TPM_CRB"tpm-crb"
+#define TYPE_TPM_CRB_SYSBUS "tpm-crb-device"
#define TYPE_TPM_SPAPR "tpm-spapr"
#define TYPE_TPM_TIS_I2C"tpm-tis-i2c"
@@ -56,6 +57,8 @@ struct TPMIfClass {
object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_SYSBUS)
#define TPM_IS_CRB(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
+#define TPM_IS_CRB_SYSBUS(chr) \
+object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB_SYSBUS)
#define TPM_IS_SPAPR(chr) \
object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
#define TPM_IS_TIS_I2C(chr) \
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ea331a20d1..f809137fc9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -31,6 +31,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci/pci_bridge.h"
#include "qemu/cutils.h"
+#include "qom/object.h"
static GArray *build_alloc_array(void)
{
@@ -2218,7 +2219,7 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
{
uint8_t start_method_params[12] = {};
unsigned log_addr_offset;
-uint64_t control_area_start_address;
+uint64_t baseaddr, control_area_start_address;
TPMIf *tpmif = tpm_find();
uint32_t start_method;
AcpiTable table = { .sig = "TPM2", .rev = 4,
@@ -2236,6 +2237,10 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker,
GArray *tcpalog,
} else if (TPM_IS_CRB(tpmif)) {
control_area_start_address = TPM_CRB_ADDR_CTRL;
start_method = TPM2_START_METHOD_CRB;
+} else if (TPM_IS_CRB_SYSBUS(tpmif)) {
+baseaddr = object_property_get_uint(OBJECT(tpmif), "baseaddr", NULL);
+control_area_start_address = baseaddr + A_CRB_CTRL_REQ;
Re: [PATCH v2 11/11] tpm_crb: support restoring older vmstate
On 7/14/23 03:09, Joelle van Dyne wrote: When we moved to a single mapping and modified TPM CRB's VMState, it broke restoring of VMs that were saved on an older version. This change allows those VMs to gracefully migrate to the new memory mapping. Thanks. This has to be in 4/11 though.
Re: [Qemu-devel] [PATCH] acpi: Add emulated sleep button
On Fri, 7 Jul 2023 13:43:36 -0400 "Annie.li" wrote: > Hi Igor, > > Revisiting this thread and have more questions, please clarify, thank you! > > On 9/20/2021 3:53 AM, Igor Mammedov wrote: > > On Fri, 6 Aug 2021 16:18:09 -0400 > > "Annie.li" wrote: > > > >> Hello Igor, > >> > >> This is an old patch, but it does what we need. > >> I am getting a little bit lost about not implementing fixed hardware > >> sleep button, can you please clarify? thank you! > >> > >> On 7/20/2017 10:59 AM, Igor Mammedov wrote: > >>> On Thu, 20 Jul 2017 11:31:26 +0200 > >>> Stefan Fritsch wrote: > >>> > From: Stefan Fritsch > > Add an ACPI sleep button and QMP/HMP commands to trigger it. A sleep > button is a so called "fixed hardware feature", which makes it more > suitable for putting the system to sleep than a laptop lid, for example. > > The sleep button is disabled by default (Bit 5 in the FACP flags > register set and no button "device" present in SSDT/DSDT). Clearing said > bit enables it as a fixed feature device. > >>> per spec sleep button is used for both putting system into > >>> sleep and for waking it up. > >>> > >>> Reusing system_wakeup 'button' to behave as per spec would > >>> make this patch significantly smaller. > Current 'system_wakeup' sets the WAK_STS bit and PWRBTN_STS to wake up > the system, the system_wakeup 'button' is the power button. So(Correct me > if I am wrong) reusing the system_wakeup 'button' means reusing the power > button for sleep. See the following code of setting WAK_STS and PWRBTN_STS > for 'system_wakeup', > case QEMU_WAKEUP_REASON_OTHER: > /* ACPI_BITMASK_WAKE_STATUS should be set on resume. > Pretend that resume was caused by power button */ > ar->pm1.evt.sts |= > (ACPI_BITMASK_WAKE_STATUS | ACPI_BITMASK_POWER_BUTTON_STATUS); (that's quite ancient code (0bacd1300d) and I couldn't find a reason why power button was chosen. https://www.mail-archive.com/kvm@vger.kernel.org/msg05742.html that was tested with WinXP so would be wise to check if SLEEP button works there. (though I'm not sure if we still care for XP being runnable on QEMU) ) it doesn't have to be ACPI_BITMASK_POWER_BUTTON_STATUS, you can convert it to sleep button by using ACPI_BITMASK_SLEEP_BUTTON_STATUS. However you'll have to enable sleep button (which is disabled currently) in FADT (ACPI_FADT_F_SLP_BUTTON), for guest to see the button. > break; > > Per the ACPI spec, the power button can be used in single button model, i.e. > it can be used to either shut down the system or put the system into sleep. > However, this depends on the software policy and user's setting of the > system. > Sleep/shutdown can not be done through the power button in the same > scenario. > If the system has configured pressing the power button to put the system > into sleep, > system_sleep will put the system into sleep state through the power > button, as well > as system_powerdown. Pressing the power button will not shut down the > system. > In this case, system_powerdown has its own issue, but this is different > story and > let's just focus on things related to system_sleep here. regardless of what button is used OSPM is free to enable or disable it using FOO_EN bits. > >> About reusing "system_wakeup", does it mean the following? > >> > >> 1. when guest is in sleep state, "system_wakeup" wakes up the guest > >> 2. when guest is running, "system_wakeup" puts the guest into sleep > > yes, it could be something like this > > > > > >> "system_wakeup" sets WAK_STS and then system transitions to the > >> working state. Correspondingly, I suppose both SLPBTN_STS and > >> SLPBTN_EN need to be set for sleeping, and this is what fixed > >> hardware sleep button requires? > > yep > > > >> I have combined the sleep and wakeup together, share the > >> code between. But Xen also registers the wakeup notifier, and > >> this makes things more complicated if this notifier is shared > >> between sleep and wakeup. Or this is my misunderstanding? > >> please correct me if I am wrong. > > you'd have to fixup xen notifier to handle that > > > >>> If you like idea of separate command/button better, then > >>> I'd go generic control sleep button way instead of fixed > >>> hardware, it would allow us to reuse most of the code in > >>> ARM target, plus I'd avoid notifiers and use acpi device > >>> lookup instead (see: qmp_query_acpi_ospm_status as example) > Implementing the generic control sleep button for x86 does align > to the generic control power button implementation in ARM, but > it doesn't align to the current fixed hardware power button for x86. > Should sleep button be implemented as generic control sleep button > to reuse code in ARM or fixed hardware button to align to the fixed > power button for x86? sleep control button device was present in ACPI 1.0b so it might be supported by x86 as well
Re: [PATCH] hw/tpm: TIS on sysbus: Remove unsupport ppi command line option
Hi Stefan, On 7/14/23 13:51, Stefan Berger wrote: > > > On 7/14/23 02:07, Joelle van Dyne wrote: >> On Thu, Jul 13, 2023 at 10:20 AM Stefan Berger >> wrote: >>> >>> The ppi command line option for the TIS device on sysbus never worked >>> and caused an immediate segfault. Remove support for it since it also >>> needs support in the firmware and needs testing inside the VM. >>> >>> Reproducer with the ppi=on option passed: >>> >>> qemu-system-aarch64 \ >>> -machine virt,gic-version=3 \ >>> -m 4G \ >>> -nographic -no-acpi \ >>> -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \ >>> -tpmdev emulator,id=tpm0,chardev=chrtpm \ >>> -device tpm-tis-device,tpmdev=tpm0,ppi=on >>> [...] >>> Segmentation fault (core dumped) >>> >>> Signed-off-by: Stefan Berger >> >> Do you need to add a VMSTATE_UNUSED_TEST in case a future QEMU version >> introduces a new field in the same position which will cause an issue >> when restoring from an older version? > > Hm, you got a point there. We will have to error-out in case someone > sets ppi=on instead since the expectation that PPI would work is > simply not there. v2 coming soon. as Joelle pointed it out ppi_enabled is not part of vmstate_tpm_tis_sysbus fields. And since it has never worked I suspect we cannot have any existing VM enabling it. So I don't get the issue with this 1st version? Thanks Eric > > Stefan >
Re: [PATCH, trivial 15/29] tree-wide spelling fixes in comments and some messages: other architectures
14.07.2023 15:49, Peter Maydell wrote: On Fri, 14 Jul 2023 at 12:40, Michael Tokarev wrote: Signed-off-by: Michael Tokarev --- host/include/aarch64/host/cpuinfo.h | 2 +- This... hw/misc/allwinner-r40-dramc.c| 2 +- hw/misc/exynos4210_rng.c | 2 +- ...these... tests/tcg/aarch64/gdbstub/test-sve.py| 2 +- tests/tcg/aarch64/sme-outprod1.c | 2 +- tests/tcg/aarch64/system/boot.S | 6 +++--- tests/tcg/aarch64/system/semiheap.c | 2 +- ...and these are all arm, not "other". I'll move these 7 files to the "arm" part, and both can now have your r-b. But anyway, for the whole patch Reviewed-by: Peter Maydell Thank you! /mjt
Re: [PATCH, trivial 24/29] tree-wide spelling fixes in comments and some messages: migration/
14.07.2023 16:05, Fabiano Rosas wrote: --- a/migration/migration-stats.c +++ b/migration/migration-stats.c @@ -50,3 +50,3 @@ void migration_rate_set(uint64_t limit) /* - * 'limit' is per second. But we check it each BUFER_DELAY miliseconds. + * 'limit' is per second. But we check it each BUFER_DELAY milliseconds. BUFER is a typo here as well. Fixed this one too, thank you! /mjt
Re: [PATCH, trivial 01/29] tree-wide spelling fixes in comments and some messages: block
14.07.2023 14:38, Michael Tokarev:
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -69,3 +69,3 @@ typedef struct BlockCopyCallState {
/*
- * Fields that report information about return values and erros.
+ * Fields that report information about return values and errors.
* Protected by lock in BlockCopyState.
@@ -464,3 +464,3 @@ static coroutine_fn int block_copy_task_run(AioTaskPool
*pool,
*
- * No sync here: nor bitmap neighter intersecting requests handling, only copy.
+ * No sync here: nor bitmap neither intersecting requests handling, only copy.
This should be reworded too. Fixed in git:
No sync here: neither bitmap nor intersecting requests handling, only copy.
/mjt
Re: [PATCH, trivial 24/29] tree-wide spelling fixes in comments and some messages: migration/
Michael Tokarev writes: > Signed-off-by: Michael Tokarev > --- > migration/migration-stats.c | 2 +- > migration/migration.h | 4 ++-- > migration/multifd-zlib.c| 2 +- > migration/multifd-zstd.c| 2 +- > migration/multifd.c | 2 +- > migration/savevm.c | 2 +- > migration/trace-events | 2 +- > 7 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/migration/migration-stats.c b/migration/migration-stats.c > index f98c8260be..f195e89732 100644 > --- a/migration/migration-stats.c > +++ b/migration/migration-stats.c > @@ -50,3 +50,3 @@ void migration_rate_set(uint64_t limit) > /* > - * 'limit' is per second. But we check it each BUFER_DELAY miliseconds. > + * 'limit' is per second. But we check it each BUFER_DELAY milliseconds. BUFER is a typo here as well. Aside from that: Reviewed-by: Fabiano Rosas
Re: [PATCH V2 06/10] tests/qtest: migration events
Steve Sistare writes:
> Define a state object to capture events seen by migration tests, to allow
> more events to be captured in a subsequent patch, and simplify event
> checking in wait_for_migration_pass. No functional change.
>
> Signed-off-by: Steve Sistare
I'm working on top of this patch in another series and it works
fine. I have a patch adding the migration events to it (setup, active,
completed, etc).
> ---
> tests/qtest/migration-helpers.c | 24 +--
> tests/qtest/migration-helpers.h | 8 +++--
> tests/qtest/migration-test.c| 68
> +++--
> 3 files changed, 44 insertions(+), 56 deletions(-)
>
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index be00c52..b541108 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -23,26 +23,16 @@
> */
> #define MIGRATION_STATUS_WAIT_TIMEOUT 120
>
> -bool migrate_watch_for_stop(QTestState *who, const char *name,
> -QDict *event, void *opaque)
> -{
> -bool *seen = opaque;
> -
> -if (g_str_equal(name, "STOP")) {
> -*seen = true;
> -return true;
> -}
> -
> -return false;
> -}
> -
> -bool migrate_watch_for_resume(QTestState *who, const char *name,
> +bool migrate_watch_for_events(QTestState *who, const char *name,
>QDict *event, void *opaque)
> {
> -bool *seen = opaque;
> +QTestMigrationState *state = opaque;
>
> -if (g_str_equal(name, "RESUME")) {
> -*seen = true;
> +if (g_str_equal(name, "STOP")) {
> +state->stop_seen = true;
> +return true;
> +} else if (g_str_equal(name, "RESUME")) {
> +state->resume_seen = true;
> return true;
> }
>
> diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
> index 009e250..59fbb83 100644
> --- a/tests/qtest/migration-helpers.h
> +++ b/tests/qtest/migration-helpers.h
> @@ -15,9 +15,11 @@
>
> #include "libqtest.h"
>
> -bool migrate_watch_for_stop(QTestState *who, const char *name,
> -QDict *event, void *opaque);
> -bool migrate_watch_for_resume(QTestState *who, const char *name,
> +typedef struct QTestMigrationState {
> +bool stop_seen, resume_seen;
> +} QTestMigrationState;
> +
> +bool migrate_watch_for_events(QTestState *who, const char *name,
>QDict *event, void *opaque);
>
> G_GNUC_PRINTF(3, 4)
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index b0c355b..2dde3af 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -43,8 +43,8 @@
> unsigned start_address;
> unsigned end_address;
> static bool uffd_feature_thread_id;
> -static bool got_src_stop;
> -static bool got_dst_resume;
> +static QTestMigrationState src_state;
> +static QTestMigrationState dst_state;
It's a bit cumbersome though to have the QTestMigrationState in
migration-test.c and having to pass it around. Could we maybe move it
inside QTestState? That way it is easily reachable from
migrate-helpers.c and we could move all the wait* functions there.
Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation
On Fri, 14 Jul 2023 at 12:50, Igor Mammedov wrote:
>
> On Thu, 13 Jul 2023 12:59:55 +0100
> Peter Maydell wrote:
>
> > On Thu, 13 Jul 2023 at 12:52, Marcin Juszkiewicz
> > wrote:
> > >
> > > W dniu 13.07.2023 o 13:44, Peter Maydell pisze:
> > >
> > > > I see this isn't a change in this patch, but given that
> > > > what the user specifies is not "cortex-a8-arm-cpu" but
> > > > "cortex-a8", why do we include the "-arm-cpu" suffix in
> > > > the error messages? It's not valid syntax to say
> > > > "-cpu cortex-a8-arm-cpu", so it's a bit misleading...
> > >
> > > Internally those cpu names are "max-{TYPE_ARM_CPU}" and similar for
> > > other architectures.
> >
> > Yes; my question is "why are we not using the user-facing
> > string rather than the internal type name?".
>
> With other targets full CPU type name can also be valid
> user-facing string. Namely we use it with -device/device_add
> interface. Considering we would like to have CPU hotplug
> on ARM as well, we shouldn't not outlaw full type name.
> (QMP/monitor interface also mostly uses full type names)
You don't seem to be able to use the full type name on
x86-64 either:
$ ./build/all/qemu-system-x86_64 -cpu pentium-x86_64-cpu
qemu-system-x86_64: unable to find CPU model 'pentium-x86_64-cpu'
and '-cpu help' does not list them with the suffix.
> Instead it might be better to consolidate on what has
> been done on making CPU '-device' compatible and
> allow to use full CPU type name with '-cpu' on arm machines.
>
> Then later call suffix-less legacy => deprecate/drop it from
> user-facing side including cleanup of all the infra we've
> invented to keep mapping between cpu_model and typename.
This seems to me like a worsening of the user interface,
and in practice there is not much likelihood of being
able to deprecate-and-drop the nicer user-facing names,
because they are baked into so many command lines and
scripts.
thanks
-- PMM
Re: [PATCH, trivial 00/29] tree-wide spelling fixes in comments and some messages
14.07.2023 15:44, Peter Maydell wrote: .. Michael Tokarev (29): tree-wide spelling fixes in comments and some messages: block tree-wide spelling fixes in comments and some messages: bsd-user This would be easier to deal with if it followed our standard convention of putting the subsystem name at the front of the subject line, not the end. Well, as usual, good thoughts come after the thing's done already :) I pushed whole series to my qemu tree clone on gitlab: https://gitlab.com/mjt0k/qemu/-/commits/spelling Here, it is actually easier to review because gitlab diff highlights actually changed words in addition to lines. and it's not possible to tell which I might care about reviewing from the list-of-emails pane without clicking through each time... I'm sorry about that. Here I have such window layout so whole subject fits on the screen, and actual message is below the list of messages. I don't think re-sending it is a good idea though.. ;) Thank you! /mjt
Re: [PATCH, trivial 08/29] tree-wide spelling fixes in comments and some messages: arm
On Fri, 14 Jul 2023 at 12:44, Michael Tokarev wrote: > > Signed-off-by: Michael Tokarev > --- > hw/arm/aspeed.c| 2 +- > hw/arm/mps2-tz.c | 2 +- > hw/intc/arm_gic.c | 4 ++-- > hw/intc/arm_gicv3_redist.c | 2 +- > hw/intc/armv7m_nvic.c | 2 +- > include/hw/arm/fsl-imx7.h | 2 +- > include/hw/intc/armv7m_nvic.h | 2 +- > target/arm/cpu.c | 2 +- > target/arm/cpu.h | 2 +- > target/arm/cpu64.c | 2 +- > target/arm/helper.c| 4 ++-- > target/arm/tcg/m_helper.c | 2 +- > target/arm/tcg/translate-a64.c | 4 ++-- > target/arm/tcg/translate-mve.c | 4 ++-- > target/arm/tcg/translate-sve.c | 2 +- > target/arm/tcg/translate-vfp.c | 2 +- > target/arm/tcg/vec_helper.c| 2 +- > 17 files changed, 21 insertions(+), 21 deletions(-) Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH, trivial 15/29] tree-wide spelling fixes in comments and some messages: other architectures
On Fri, 14 Jul 2023 at 12:40, Michael Tokarev wrote: > > Signed-off-by: Michael Tokarev > --- > host/include/aarch64/host/cpuinfo.h | 2 +- This... > hw/misc/allwinner-r40-dramc.c| 2 +- > hw/misc/exynos4210_rng.c | 2 +- ...these... > tests/tcg/aarch64/gdbstub/test-sve.py| 2 +- > tests/tcg/aarch64/sme-outprod1.c | 2 +- > tests/tcg/aarch64/system/boot.S | 6 +++--- > tests/tcg/aarch64/system/semiheap.c | 2 +- ...and these are all arm, not "other". But anyway, for the whole patch Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH V2 03/10] migration: add runstate function
Steve Sistare writes: > Create a subroutine for preserving the runstate after migration, > to be used in a subsequent patch. No functional change. > > Signed-off-by: Steve Sistare Reviewed-by: Fabiano Rosas
[PATCH v1] migration: refactor migration_completion
Current migration_completion function is a bit long. Refactor the long
implementation into different subfunctions:
- migration_completion_precopy: completion code related to precopy
- migration_completion_postcopy: completion code related to postcopy
- close_return_path_on_source: rp thread related cleanup on migration
completion. It is named to match with open_return_path_on_source.
This improves readability and is easier for future updates (e.g. add new
subfunctions when completion code related to new features are needed). No
functional changes intended.
Signed-off-by: Wei Wang
---
migration/migration.c | 181 +-
1 file changed, 106 insertions(+), 75 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 91bba630a8..83f1c74f99 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2058,6 +2058,21 @@ static int
await_return_path_close_on_source(MigrationState *ms)
return ms->rp_state.error;
}
+static int close_return_path_on_source(MigrationState *ms)
+{
+int ret;
+
+if (!ms->rp_state.rp_thread_created) {
+return 0;
+}
+
+trace_migration_return_path_end_before();
+ret = await_return_path_close_on_source(ms);
+trace_migration_return_path_end_after(ret);
+
+return ret;
+}
+
static inline void
migration_wait_main_channel(MigrationState *ms)
{
@@ -2288,66 +2303,107 @@ static int migration_maybe_pause(MigrationState *s,
return s->state == new_state ? 0 : -EINVAL;
}
-/**
- * migration_completion: Used by migration_thread when there's not much left.
- * The caller 'breaks' the loop when this returns.
- *
- * @s: Current migration state
- */
-static void migration_completion(MigrationState *s)
+static int migration_completion_precopy(MigrationState *s,
+int *current_active_state)
{
int ret;
-int current_active_state = s->state;
-if (s->state == MIGRATION_STATUS_ACTIVE) {
-qemu_mutex_lock_iothread();
-s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
+qemu_mutex_lock_iothread();
+s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
-s->vm_old_state = runstate_get();
-global_state_store();
+s->vm_old_state = runstate_get();
+global_state_store();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-trace_migration_completion_vm_stop(ret);
-if (ret >= 0) {
-ret = migration_maybe_pause(s, _active_state,
-MIGRATION_STATUS_DEVICE);
-}
-if (ret >= 0) {
-/*
- * Inactivate disks except in COLO, and track that we
- * have done so in order to remember to reactivate
- * them if migration fails or is cancelled.
- */
-s->block_inactive = !migrate_colo();
-migration_rate_set(RATE_LIMIT_DISABLED);
-ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
- s->block_inactive);
-}
+ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+trace_migration_completion_vm_stop(ret);
+if (ret < 0) {
+goto out_unlock;
+}
-qemu_mutex_unlock_iothread();
+ret = migration_maybe_pause(s, current_active_state,
+MIGRATION_STATUS_DEVICE);
+if (ret < 0) {
+goto out_unlock;
+}
-if (ret < 0) {
-goto fail;
-}
-} else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
-trace_migration_completion_postcopy_end();
+/*
+ * Inactivate disks except in COLO, and track that we have done so in order
+ * to remember to reactivate them if migration fails or is cancelled.
+ */
+s->block_inactive = !migrate_colo();
+migration_rate_set(RATE_LIMIT_DISABLED);
+ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
+ s->block_inactive);
+out_unlock:
+qemu_mutex_unlock_iothread();
+return ret;
+}
-qemu_mutex_lock_iothread();
-qemu_savevm_state_complete_postcopy(s->to_dst_file);
-qemu_mutex_unlock_iothread();
+static int migration_completion_postcopy(MigrationState *s)
+{
+trace_migration_completion_postcopy_end();
+
+qemu_mutex_lock_iothread();
+qemu_savevm_state_complete_postcopy(s->to_dst_file);
+qemu_mutex_unlock_iothread();
+
+/*
+ * Shutdown the postcopy fast path thread. This is only needed when dest
+ * QEMU binary is old (7.1/7.2). QEMU 8.0+ doesn't need this.
+ */
+if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
+postcopy_preempt_shutdown_file(s);
+}
+
+trace_migration_completion_postcopy_end_after_complete();
+
+
Re: [PATCH V2 02/10] migration: preserve suspended runstate
Steve Sistare writes: > A guest that is migrated in the suspended state automaticaly wakes and > continues execution. This is wrong; the guest should end migration in > the same state it started. The root causes is that the outgoing migration > code automatically wakes the guest, then saves the RUNNING runstate in > global_state_store(), hence the incoming migration code thinks the guest is > running and continues the guest if autostart is true. > > On the outgoing side, do not call qemu_system_wakeup_request(). That > alone fixes precopy migration, as process_incoming_migration_bh correctly > sets runstate from global_state_get_runstate(). > > On the incoming side for postcopy, do not wake the guest, and apply the > the same logic as found in precopy: if autostart and the runstate is > RUNNING, then vm_start, else merely restore the runstate. > > In both cases, if the restored state is SUSPENDED, then a later wakeup > request will resume the guest, courtesy of the previous "start on wakeup" > patch. > > Signed-off-by: Steve Sistare Reviewed-by: Fabiano Rosas
